Skip to contentSkip to conversation

iio: adc: mcp3911: correct "microchip,device-addr" property

PATCH Superseded Marcus Folkesson 5 participants 29 messages Jun 23, 2022 – Jun 28, 2022

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Marcus Folkesson

author maintainer
#0 lore raw

PATCH 1/10 iio: adc: mcp3911: correct "microchip,device-addr" property

Go for the right property name that is documented in the bindings.

---
 drivers/iio/adc/mcp3911.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/adc/mcp3911.c +1 -1
index 1cb4590fe412..4338552cd0ca 100644
@@ -208,7 +208,7 @@ static int mcp3911_config(struct mcp3911 *adc)
 	u32 configreg;
 	int ret;
 
-	device_property_read_u32(dev, "device-addr", &adc->dev_addr);
+	device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);
 	if (adc->dev_addr > 3) {
 		dev_err(&adc->spi->dev,
 			"invalid device address (%i). Must be in range 0-3.\n",
Signature · 1 line
2.36.1
author maintainer
#1 lore raw

PATCH 2/10 iio: adc: mcp3911: use resource-managed version of iio_device_register

Keep using managed resources as much as possible.

---
 drivers/iio/adc/mcp3911.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/iio/adc/mcp3911.c +1 -3
index 4338552cd0ca..3d9e8ed10874 100644
@@ -306,7 +306,7 @@ static int mcp3911_probe(struct spi_device *spi)
 
 	mutex_init(&adc->lock);
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(&adc->spi->dev, indio_dev);
 	if (ret)
 		goto clk_disable;
 
@@ -326,8 +326,6 @@ static void mcp3911_remove(struct spi_device *spi)
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
 	struct mcp3911 *adc = iio_priv(indio_dev);
 
-	iio_device_unregister(indio_dev);
-
 	clk_disable_unprepare(adc->clki);
 	if (adc->vref)
 		regulator_disable(adc->vref);
Signature · 1 line
2.36.1
#2 lore raw

Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register

Quoted 2 lines · Marcus Folkesson

Keep using managed resources as much as possible.

You may not mix devm_ and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong after devm_ call.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#3 lore raw

Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register

Thank you for your comments (all of them) Andy!

Quoted 7 lines · Andy Shevchenko
Quoted 2 lines · Marcus Folkesson

Keep using managed resources as much as possible.

You may not mix devm_ and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong after devm_ call.

Can you please confirm that clocks and regulators are disabled when the resources are handed back?
I cannot see where when I'm trying to follow the code.

Best regards
Marcus Folkesson

maintainer
#4 lore raw

Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register

Quoted 6 lines · On Fri, 24 Jun 2022 08:29:20 +0200 Marcus Folkesson…

Thank you for your comments (all of them) Andy!

Quoted 3 lines · Andy Shevchenko
Quoted 1 line · Marcus Folkesson
Quoted 8 lines
Quoted 4 lines
Quoted 1 line

Keep using managed resources as much as possible.

You may not mix devm_ and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong after devm_ call.

Can you please confirm that clocks and regulators are disabled when the resources are handed back?
I cannot see where when I'm trying to follow the code.

Andy isn't arguing that the goto is wrong but rather that you cannot in general safely use devm_* calls if their failure leads to having to do any cleanup. The reason is the ordering is hard to reason about. Sometimes it's safe, but often enough causes problems that we basically refuse to think hard enough to figure out if it is. Hence basic rule is don't do it.

The issue is this.
probe() {

	non_devm_call_1();
	ret = devm_call_2()
	if (ret)
		goto err;

	return 0;
err:
	unwind_non_devm_call_1()
}
remove() {
	unwind_non_devm_call_1()
}

remove or error path should unwind in opposite order of what happens in probe. On the rare occasion where that isn't the right choice, there should be very clear comments to say why.

Order is

remove() -> unwind_non_devm_call_1()
devm_managed_cleanup() -> unwind_devm_call_2()

Whereas should be

remove()-> unwind_call_2() then unwind_call_1()

There are two ways to solve this. Either only use devm for those elements in probe() that happen before the first thing you need to unwind manually or make everything devm managed (it unwinds in reverse order of setup) devm_add_action_or_reset() allows you to use your own devm_ managed callbacks if there isn't a standard one available.

Jonathan

Quoted 3 lines

Best regards
Marcus Folkesson

#5 lore raw

Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register

Quoted 52 lines · Jonathan Cameron
Quoted 6 lines · On Fri, 24 Jun 2022 08:29:20 +0200 Marcus Folkesson…

Thank you for your comments (all of them) Andy!

Quoted 3 lines · Andy Shevchenko
Quoted 1 line · Marcus Folkesson
Quoted 8 lines
Quoted 4 lines
Quoted 1 line

Keep using managed resources as much as possible.

You may not mix devm_ and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong after devm_ call.

Can you please confirm that clocks and regulators are disabled when the resources are handed back?
I cannot see where when I'm trying to follow the code.

Andy isn't arguing that the goto is wrong but rather that you cannot in general safely use devm_* calls if their failure leads to having to do any cleanup. The reason is the ordering is hard to reason about. Sometimes it's safe, but often enough causes problems that we basically refuse to think hard enough to figure out if it is. Hence basic rule is don't do it.

The issue is this.
probe() {

        non_devm_call_1();
        ret = devm_call_2()
        if (ret)
                goto err;

        return 0;
err:
        unwind_non_devm_call_1()
}
remove() {
        unwind_non_devm_call_1()
}

remove or error path should unwind in opposite order of what happens in probe. On the rare occasion where that isn't the right choice, there should be very clear comments to say why.

Order is

remove() -> unwind_non_devm_call_1()
devm_managed_cleanup() -> unwind_devm_call_2()

Whereas should be

Quoted 8 lines

remove()-> unwind_call_2() then unwind_call_1()

There are two ways to solve this. Either only use devm for those elements in probe() that happen before the first thing you need to unwind manually or make everything devm managed (it unwinds in reverse order of setup) devm_add_action_or_reset() allows you to use your own devm_ managed callbacks if there isn't a standard one available.

Thanks, Jonathan, that's exactly what I meant!

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#6 lore raw

PATCH 3/10 iio: adc: mcp3911: add support for buffers

In reply to Marcus Folkesson

Add support for buffers to make the driver fit for more usecases.

---
 drivers/iio/adc/mcp3911.c | 58 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)
drivers/iio/adc/mcp3911.c +57 -1
index 3d9e8ed10874..768cb0203f52 100644
@@ -9,6 +9,10 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/trigger.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/property.h>
@@ -54,6 +58,10 @@ struct mcp3911 {
 	struct regulator *vref;
 	struct clk *clki;
 	u32 dev_addr;
+	struct {
+		u32 channels[2];
+		s64 ts __aligned(8);
+	} scan;
 };
 
 static int mcp3911_read(struct mcp3911 *adc, u8 reg, u32 *val, u8 len)
@@ -187,16 +195,58 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 		.type = IIO_VOLTAGE,				\
 		.indexed = 1,					\
 		.channel = idx,					\
+		.scan_index = idx,				\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 			BIT(IIO_CHAN_INFO_OFFSET) |		\
 			BIT(IIO_CHAN_INFO_SCALE),		\
+		.scan_type = {					\
+			.sign = 's',				\
+			.realbits = 24,				\
+			.storagebits = 32,			\
+		},						\
 }
 
 static const struct iio_chan_spec mcp3911_channels[] = {
 	MCP3911_CHAN(0),
 	MCP3911_CHAN(1),
+	IIO_CHAN_SOFT_TIMESTAMP(2),
 };
 
+static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct mcp3911 *adc = iio_priv(indio_dev);
+	int scan_index;
+	int i = 0;
+	u32 val;
+
+	mutex_lock(&adc->lock);
+	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
+			indio_dev->masklength) {
+		const struct iio_chan_spec *scan_chan =
+			&indio_dev->channels[scan_index];
+		int ret = mcp3911_read(adc,
+				MCP3911_CHANNEL(scan_chan->channel), &val, 3);
+
+		if (ret < 0) {
+			dev_warn(&adc->spi->dev,
+					"failed to get conversion data\n");
+			goto out;
+		}
+
+		adc->scan.channels[i] = val;
+		i++;
+	}
+	iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan,
+			iio_get_time_ns(indio_dev));
+out:
+	mutex_unlock(&adc->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static const struct iio_info mcp3911_info = {
 	.read_raw = mcp3911_read_raw,
 	.write_raw = mcp3911_write_raw,
@@ -297,7 +347,7 @@ static int mcp3911_probe(struct spi_device *spi)
 		goto clk_disable;
 
 	indio_dev->name = spi_get_device_id(spi)->name;
-	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
 	indio_dev->info = &mcp3911_info;
 	spi_set_drvdata(spi, indio_dev);
 
@@ -306,6 +356,12 @@ static int mcp3911_probe(struct spi_device *spi)
 
 	mutex_init(&adc->lock);
 
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+			NULL,
+			mcp3911_trigger_handler, NULL);
+	if (ret)
+		goto clk_disable;
+
 	ret = devm_iio_device_register(&adc->spi->dev, indio_dev);
 	if (ret)
 		goto clk_disable;
Signature · 1 line
2.36.1
author maintainer
#7 lore raw

PATCH 4/10 iio: adc: mcp3911: add support for interrupts

In reply to Marcus Folkesson

Make it possible to read values upon interrupts.
Configure Data Ready Signal Output Pin to either HiZ or push-pull and use it as interrupt source.

---
 drivers/iio/adc/mcp3911.c | 69 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)
drivers/iio/adc/mcp3911.c +69 -0
index 768cb0203f52..e761feed5303 100644
@@ -26,6 +26,7 @@
 #define MCP3911_REG_GAIN		0x09
 
 #define MCP3911_REG_STATUSCOM		0x0a
+#define MCP3911_STATUSCOM_DRHIZ         BIT(12)
 #define MCP3911_STATUSCOM_CH1_24WIDTH	BIT(4)
 #define MCP3911_STATUSCOM_CH0_24WIDTH	BIT(3)
 #define MCP3911_STATUSCOM_EN_OFFCAL	BIT(2)
@@ -58,6 +59,7 @@ struct mcp3911 {
 	struct regulator *vref;
 	struct clk *clki;
 	u32 dev_addr;
+	struct iio_trigger *trig;
 	struct {
 		u32 channels[2];
 		s64 ts __aligned(8);
@@ -252,6 +254,17 @@ static const struct iio_info mcp3911_info = {
 	.write_raw = mcp3911_write_raw,
 };
 
+static irqreturn_t mcp3911_interrupt(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct mcp3911 *adc = iio_priv(indio_dev);
+
+	if (iio_buffer_enabled(indio_dev))
+		iio_trigger_poll(adc->trig);
+
+	return IRQ_HANDLED;
+};
+
 static int mcp3911_config(struct mcp3911 *adc)
 {
 	struct device *dev = &adc->spi->dev;
@@ -292,11 +305,30 @@ static int mcp3911_config(struct mcp3911 *adc)
 	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
 }
 
+
+static int mcp3911_set_trigger_state(struct iio_trigger *trig, bool enable)
+{
+	struct mcp3911 *adc = iio_trigger_get_drvdata(trig);
+
+	if (enable)
+		enable_irq(adc->spi->irq);
+	else
+		disable_irq(adc->spi->irq);
+
+	return 0;
+}
+
+static const struct iio_trigger_ops mcp3911_trigger_ops = {
+	.validate_device = iio_trigger_validate_own_device,
+	.set_trigger_state = mcp3911_set_trigger_state,
+};
+
 static int mcp3911_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
 	struct mcp3911 *adc;
 	int ret;
+	bool dr_hiz;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
 	if (!indio_dev)
@@ -346,6 +378,17 @@ static int mcp3911_probe(struct spi_device *spi)
 	if (ret)
 		goto clk_disable;
 
+	dr_hiz = device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz");
+	if (dr_hiz)
+		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
+				0, 2);
+	else
+		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
+				MCP3911_STATUSCOM_DRHIZ, 2);
+
+	if (ret < 0)
+		goto clk_disable;
+
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
 	indio_dev->info = &mcp3911_info;
@@ -356,6 +399,32 @@ static int mcp3911_probe(struct spi_device *spi)
 
 	mutex_init(&adc->lock);
 
+	if (spi->irq > 0) {
+		adc->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
+				indio_dev->name,
+				iio_device_id(indio_dev));
+		if (!adc->trig)
+			goto clk_disable;
+
+		adc->trig->ops = &mcp3911_trigger_ops;
+		iio_trigger_set_drvdata(adc->trig, adc);
+		ret = devm_iio_trigger_register(&spi->dev, adc->trig);
+		if (ret)
+			goto clk_disable;
+
+		/*
+		 * The device generates interrupts as long as it is powered up.
+		 * Some platforms might not allow the option to power it down so
+		 * don't enable the interrupt to avoid extra load on the system
+		 */
+		ret = devm_request_irq(&spi->dev, spi->irq,
+				&mcp3911_interrupt,
+				IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN | IRQF_ONESHOT,
+				indio_dev->name, indio_dev);
+		if (ret)
+			goto clk_disable;
+	}
+
 	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
 			NULL,
 			mcp3911_trigger_handler, NULL);
Signature · 1 line
2.36.1
#8 lore raw

Re: [PATCH 04/10] iio: adc: mcp3911: add support for interrupts

Quoted 4 lines · Marcus Folkesson

Make it possible to read values upon interrupts.
Configure Data Ready Signal Output Pin to either HiZ or push-pull and use it as interrupt source.

...

Quoted 3 lines

}

+

Unwanted blank line.

drivers/iio/adc/mcp3911.c static int mcp3911_config(struct mcp3911 *adc)

Quoted 4 lines
drivers/iio/adc/mcp3911.c +3 -0
+static int mcp3911_set_trigger_state(struct iio_trigger *trig, bool enable)
+{
 
+       bool dr_hiz;

As far as I can see you don't need this variable, just call if (device_property_...(...)).

drivers/iio/adc/mcp3911.c static int mcp3911_probe(struct spi_device *spi) · line 388

Quoted 9 lines
drivers/iio/adc/mcp3911.c +8 -0
+       dr_hiz = device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz");
+       if (dr_hiz)
+               ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
+                               0, 2);
+       else
+               ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
+                               MCP3911_STATUSCOM_DRHIZ, 2);
 
+

Unneeded blank line.

Quoted 2 lines
drivers/iio/adc/mcp3911.c +2 -0
+       if (ret < 0)
+               goto clk_disable;
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#9 lore raw

Re: [PATCH 04/10] iio: adc: mcp3911: add support for interrupts

Quoted 2 lines · Andy Shevchenko

Unwanted blank line.

Removed

Quoted 7 lines

drivers/iio/adc/mcp3911.c static int mcp3911_config(struct mcp3911 *adc)

Quoted 4 lines
drivers/iio/adc/mcp3911.c +3 -0
+static int mcp3911_set_trigger_state(struct iio_trigger *trig, bool enable)
+{
 
+       bool dr_hiz;

As far as I can see you don't need this variable, just call if (device_property_...(...)).

Fixed

Quoted 2 lines

Unneeded blank line.

Removed

Thanks,
Marcus Folkesson

author maintainer
#10 lore raw

PATCH 5/10 dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry

In reply to Marcus Folkesson

The Data Ready Output Pin is either hard wired to work as high impedance or push-pull. Make it configurable.

---
 .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml    | 8 ++++++++
 1 file changed, 8 insertions(+)
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +8 -0
index 95ab285f4eba..74b333e44bfd 100644
@@ -36,6 +36,14 @@ properties:
     description: IRQ line of the ADC
     maxItems: 1
 
+  microchip,data-ready-hiz:
+    description:
+      Data Ready Pin Inactive State Control bit
+      true = The DR pin state is high-impedance when data are NOT ready
+      false = The DR pin state is a logic high when data are NOT ready
+    type: boolean
+    default: false
+
   microchip,device-addr:
     description: Device address when multiple MCP3911 chips are present on the same SPI bus.
     $ref: /schemas/types.yaml#/definitions/uint32
Signature · 1 line
2.36.1
#11 lore raw

Re: [PATCH 05/10] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry

Quoted 13 lines · Marcus Folkesson

The Data Ready Output Pin is either hard wired to work as high impedance or push-pull. Make it configurable.

---
 .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml    | 8 ++++++++
 1 file changed, 8 insertions(+)
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +0 -0
index 95ab285f4eba..74b333e44bfd 100644

Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 41

Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +3 -0
@@ -36,6 +36,14 @@ properties:
     description: IRQ line of the ADC
     maxItems: 1
 
+  microchip,data-ready-hiz:
+    description:
+      Data Ready Pin Inactive State Control bit

"Bit" of what? Do not describe the programming model but the actual feature.

Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 45

Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +4 -0
+      true = The DR pin state is high-impedance when data are NOT ready
+      false = The DR pin state is a logic high when data are NOT ready
+    type: boolean
+    default: false

You do not need default for bools.

Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 49

Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +1 -0
+
   microchip,device-addr:
     description: Device address when multiple MCP3911 chips are present on the same SPI bus.
     $ref: /schemas/types.yaml#/definitions/uint32

Best regards,
Krzysztof

author maintainer
#12 lore raw

Re: [PATCH 05/10] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry

Quoted 6 lines · Krzysztof Kozlowski

Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 41

Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +3 -0
+  microchip,data-ready-hiz:
+    description:
+      Data Ready Pin Inactive State Control bit

"Bit" of what? Do not describe the programming model but the actual feature.

You are right. I will remove "bit".

Quoted 6 lines

Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 45

Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +4 -0
+      true = The DR pin state is high-impedance when data are NOT ready
+      false = The DR pin state is a logic high when data are NOT ready
+    type: boolean
+    default: false

You do not need default for bools.

OK

Quoted 9 lines
Quoted 5 lines

+
microchip,device-addr:
description: Device address when multiple MCP3911 chips are present on the same SPI bus. $ref: /schemas/types.yaml#/definitions/uint32

Best regards,
Krzysztof

Thanks,
Marcus Folkesson

maintainer
#13 lore raw

Re: [PATCH 05/10] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry

In reply to Marcus Folkesson
Quoted 8 lines · Marcus Folkesson

The Data Ready Output Pin is either hard wired to work as high impedance or push-pull. Make it configurable.

---
 .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml    | 8 ++++++++
 1 file changed, 8 insertions(+)

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: properties:microchip,data-ready-hiz: 'oneOf' conditional failed, one must be fixed:
Additional properties are not allowed ('default' was unexpected)
hint: A vendor boolean property can use "type: boolean"
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: properties:microchip,data-ready-hiz: 'oneOf' conditional failed, one must be fixed:
'enum' is a required property
'const' is a required property
hint: A vendor string property with exact values has an implicit type
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
Additional properties are not allowed ('default', 'type' were unexpected)
hint: A vendor string property with exact values has an implicit type
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: properties:microchip,data-ready-hiz: 'oneOf' conditional failed, one must be fixed:
'$ref' is a required property
'allOf' is a required property
hint: A vendor property needs a $ref to types.yaml
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: ignoring, error in schema: properties: microchip,data-ready-hiz Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.example.dtb:0:0: /example-0/spi/adc@0: failed to match any schema with compatible: ['microchip,mcp3911']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit.

author maintainer
#14 lore raw

PATCH 6/10 iio: adc: mcp3911: add support for oversampling ratio

In reply to Marcus Folkesson

The chip support oversampling ratio so expose it to userspace.

---
 drivers/iio/adc/mcp3911.c | 72 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
drivers/iio/adc/mcp3911.c +72 -0
index e761feed5303..65831bef12f6 100644
@@ -5,6 +5,8 @@
  * Copyright (C) 2018 Marcus Folkesson <[email protected]>
  * Copyright (C) 2018 Kent Gustavsson <[email protected]>
  */
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
@@ -35,6 +37,7 @@
 #define MCP3911_REG_CONFIG		0x0c
 #define MCP3911_CONFIG_CLKEXT		BIT(1)
 #define MCP3911_CONFIG_VREFEXT		BIT(2)
+#define MCP3911_CONFIG_OSR		GENMASK(13, 11)
 
 #define MCP3911_REG_OFFCAL_CH0		0x0e
 #define MCP3911_REG_GAINCAL_CH0		0x11
@@ -53,6 +56,8 @@
 
 #define MCP3911_NUM_CHANNELS		2
 
+static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};
+
 struct mcp3911 {
 	struct spi_device *spi;
 	struct mutex lock;
@@ -108,6 +113,22 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
 	return mcp3911_write(adc, reg, val, len);
 }
 
+
+static int mcp3911_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long info)
+{
+	switch (info) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*type = IIO_VAL_INT;
+		*vals = mcp3911_osr_table;
+		*length = ARRAY_SIZE(mcp3911_osr_table);
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
 static int mcp3911_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *channel, int *val,
 			    int *val2, long mask)
@@ -134,6 +155,16 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 
 		ret = IIO_VAL_INT;
 		break;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = mcp3911_read(adc,
+				MCP3911_REG_CONFIG, val, 2);
+		if (ret)
+			goto out;
+
+		*val = FIELD_GET(MCP3911_CONFIG_OSR, *val);
+		*val = 32 << *val;
+		ret = IIO_VAL_INT;
+		break;
 
 	case IIO_CHAN_INFO_SCALE:
 		if (adc->vref) {
@@ -186,6 +217,42 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 				MCP3911_STATUSCOM_EN_OFFCAL,
 				MCP3911_STATUSCOM_EN_OFFCAL, 2);
 		break;
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		switch (val) {
+		case 4096:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x07);
+			break;
+		case 2048:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x06);
+			break;
+		case 1024:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x05);
+			break;
+		case 512:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x04);
+			break;
+		case 256:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x03);
+			break;
+		case 128:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x02);
+			break;
+		case 64:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x01);
+			break;
+		case 32:
+			val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x00);
+			break;
+		default:
+			ret = -EINVAL;
+			goto out;
+		}
+
+		ret = mcp3911_update(adc, MCP3911_REG_CONFIG,
+				MCP3911_CONFIG_OSR,
+				val, 2);
+		break;
 	}
 
 out:
@@ -198,9 +265,13 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 		.indexed = 1,					\
 		.channel = idx,					\
 		.scan_index = idx,				\
+		.scan_index = idx,				\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 			BIT(IIO_CHAN_INFO_OFFSET) |		\
 			BIT(IIO_CHAN_INFO_SCALE),		\
+		.info_mask_shared_by_type_available =		\
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
 		.scan_type = {					\
 			.sign = 's',				\
 			.realbits = 24,				\
@@ -252,6 +323,7 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p)
 static const struct iio_info mcp3911_info = {
 	.read_raw = mcp3911_read_raw,
 	.write_raw = mcp3911_write_raw,
+	.read_avail = mcp3911_read_avail,
 };
 
 static irqreturn_t mcp3911_interrupt(int irq, void *dev_id)
Signature · 1 line
2.36.1
#15 lore raw

Re: [PATCH 06/10] iio: adc: mcp3911: add support for oversampling ratio

Quoted 2 lines · Marcus Folkesson

The chip support oversampling ratio so expose it to userspace.

supports

ratio, so

...

Quoted 1 line

+static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};

Spaces inside {}.

...

Quoted 3 lines

}

+

Unwanted blank line.

...

Quoted 19 lines
drivers/iio/adc/mcp3911.c +19 -0
+               switch (val) {
+               case 4096:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x07);
+                       break;
+               case 2048:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x06);
+                       break;
+               case 1024:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x05);
+                       break;
+               case 512:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x04);
+                       break;
+               case 256:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x03);
+                       break;
+               case 128:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x02);
+                       break;

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 250

Quoted 10 lines
drivers/iio/adc/mcp3911.c +10 -0
+               case 64:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x01);
+                       break;
+               case 32:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x00);
+                       break;
+               default:
+                       ret = -EINVAL;
+                       goto out;
+               }

I understood why the table above, but this is a waste of resources. Use that table

...

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 254

Quoted 3 lines
drivers/iio/adc/mcp3911.c +3 -0
+               ret = mcp3911_update(adc, MCP3911_REG_CONFIG,
+                               MCP3911_CONFIG_OSR,
+                               val, 2);

sizeof() ?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#16 lore raw

Re: [PATCH 06/10] iio: adc: mcp3911: add support for oversampling ratio

Quoted 10 lines · Andy Shevchenko
Quoted 2 lines · Marcus Folkesson

The chip support oversampling ratio so expose it to userspace.

supports

ratio, so

...

Thanks

Quoted 4 lines
Quoted 2 lines

+static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};

Spaces inside {}.

Not sure what you mean?

Quoted 10 lines

...

Quoted 3 lines

}

+

Unwanted blank line.

...

Removed

Quoted 31 lines
Quoted 20 lines
drivers/iio/adc/mcp3911.c +19 -0
+               switch (val) {
+               case 4096:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x07);
+                       break;
+               case 2048:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x06);
+                       break;
+               case 1024:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x05);
+                       break;
+               case 512:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x04);
+                       break;
+               case 256:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x03);
+                       break;
+               case 128:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x02);
+                       break;

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 250

Quoted 10 lines
drivers/iio/adc/mcp3911.c +10 -0
+               case 64:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x01);
+                       break;
+               case 32:
+                       val = FIELD_PREP(MCP3911_CONFIG_OSR, 0x00);
+                       break;
+               default:
+                       ret = -EINVAL;
+                       goto out;
+               }
Quoted 2 lines

I understood why the table above, but this is a waste of resources. Use that table

Yep, I will use the table instead

Quoted 8 lines

...

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 254

Quoted 3 lines
drivers/iio/adc/mcp3911.c +3 -0
+               ret = mcp3911_update(adc, MCP3911_REG_CONFIG,
+                               MCP3911_CONFIG_OSR,
+                               val, 2);

sizeof() ?

sizeof() what?

Thanks,
Marcus Folkesson

#17 lore raw

Re: [PATCH 06/10] iio: adc: mcp3911: add support for oversampling ratio

Quoted 3 lines · Marcus Folkesson
Quoted 2 lines · Andy Shevchenko

On Thu, Jun 23, 2022 at 9:08 PM Marcus Folkesson
<[email protected]> wrote:

...

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

+static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};

Spaces inside {}.

Not sure what you mean?

   int foo = { 1, 2, 3 };

...

Quoted 7 lines
Quoted 5 lines

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 254

Quoted 3 lines
drivers/iio/adc/mcp3911.c +3 -0
+               ret = mcp3911_update(adc, MCP3911_REG_CONFIG,
+                               MCP3911_CONFIG_OSR,
+                               val, 2);

sizeof() ?

sizeof() what?

sizeof(val) ?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#18 lore raw

PATCH 7/10 iio: adc: mcp3911: use correct formula for AD conversion

In reply to Marcus Folkesson

The ADC conversion is actually not rail-to-rail but with a factor 1.5. Make use of this factor when calculating actual voltage.

---
 drivers/iio/adc/mcp3911.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/iio/adc/mcp3911.c +12 -5
index 65831bef12f6..9e365947d285 100644
@@ -48,8 +48,8 @@
 #define MCP3911_CHANNEL(x)		(MCP3911_REG_CHANNEL0 + x * 3)
 #define MCP3911_OFFCAL(x)		(MCP3911_REG_OFFCAL_CH0 + x * 6)
 
-/* Internal voltage reference in uV */
-#define MCP3911_INT_VREF_UV		1200000
+/* Internal voltage reference in mV */
+#define MCP3911_INT_VREF_MV		1200
 
 #define MCP3911_REG_READ(reg, id)	((((reg) << 1) | ((id) << 5) | (1 << 0)) & 0xff)
 #define MCP3911_REG_WRITE(reg, id)	((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
@@ -178,11 +178,18 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 
 			*val = ret / 1000;
 		} else {
-			*val = MCP3911_INT_VREF_UV;
+			*val = MCP3911_INT_VREF_MV;
 		}
 
-		*val2 = 24;
-		ret = IIO_VAL_FRACTIONAL_LOG2;
+		/*
+		 * For 24bit Conversion
+		 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
+		 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
+		 */
+
+		/* val2 = (2^23 * 1.5) */
+		*val2 = 12582912;
+		ret = IIO_VAL_FRACTIONAL;
 		break;
 	}
 
Signature · 1 line
2.36.1
author maintainer
#19 lore raw

PATCH 8/10 iio: adc: mcp3911: add support for phase

In reply to Marcus Folkesson

The MCP3911 incorporates a phase delay generator, which ensures that the two ADCs are converting the inputs with a fixed delay between them. Expose it to userspace.

---
 drivers/iio/adc/mcp3911.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)
drivers/iio/adc/mcp3911.c +24 -1
index 9e365947d285..aefc5eac874c 100644
@@ -155,6 +155,17 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 
 		ret = IIO_VAL_INT;
 		break;
+
+	case IIO_CHAN_INFO_PHASE:
+		ret = mcp3911_read(adc,
+				   MCP3911_REG_PHASE, val, 2);
+		if (ret)
+			goto out;
+
+		*val = sign_extend32(*val, 12);
+		ret = IIO_VAL_INT;
+		break;
+
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		ret = mcp3911_read(adc,
 				MCP3911_REG_CONFIG, val, 2);
@@ -225,6 +236,16 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 				MCP3911_STATUSCOM_EN_OFFCAL, 2);
 		break;
 
+	case IIO_CHAN_INFO_PHASE:
+		if (val2 != 0 || val > 0xfff) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/* Write phase */
+		ret = mcp3911_write(adc, MCP3911_REG_PHASE, val,
+				    2);
+		break;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		switch (val) {
 		case 4096:
@@ -273,7 +294,9 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 		.channel = idx,					\
 		.scan_index = idx,				\
 		.scan_index = idx,				\
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+		.info_mask_shared_by_type =			\
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)|	\
+			BIT(IIO_CHAN_INFO_PHASE),		\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 			BIT(IIO_CHAN_INFO_OFFSET) |		\
 			BIT(IIO_CHAN_INFO_SCALE),		\
Signature · 1 line
2.36.1
#20 lore raw

Re: [PATCH 08/10] iio: adc: mcp3911: add support for phase

Quoted 5 lines · Marcus Folkesson

The MCP3911 incorporates a phase delay generator, which ensures that the two ADCs are converting the inputs with a fixed delay between them. Expose it to userspace.

...

Quoted 8 lines
drivers/iio/adc/mcp3911.c +2 -0
+       case IIO_CHAN_INFO_PHASE:
+               if (val2 != 0 || val > 0xfff) {

= BIT(12) will show better the intention

drivers/iio/adc/mcp3911.c +3 -0
+                       ret = -EINVAL;
+                       goto out;
+               }

...

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 247

Quoted 2 lines
drivers/iio/adc/mcp3911.c +2 -0
+               ret = mcp3911_write(adc, MCP3911_REG_PHASE, val,
+                                   2);

One line.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#21 lore raw

Re: [PATCH 08/10] iio: adc: mcp3911: add support for phase

Quoted 4 lines · Andy Shevchenko

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 247

Quoted 2 lines
drivers/iio/adc/mcp3911.c +2 -0
+               ret = mcp3911_write(adc, MCP3911_REG_PHASE, val,
+                                   2);

One line.

Fixed

Quoted 4 lines
Signature · 3 lines
-- 
With Best Regards,
Andy Shevchenko

Thanks,
Marcus Folkesson

maintainer
#22 lore raw

Re: [PATCH 08/10] iio: adc: mcp3911: add support for phase

Quoted 7 lines · On Fri, 24 Jun 2022 14:02:22 +0200 Marcus Folkesson…
Quoted 4 lines · Andy Shevchenko

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 247

Quoted 2 lines
drivers/iio/adc/mcp3911.c +2 -0
+               ret = mcp3911_write(adc, MCP3911_REG_PHASE, val,
+                                   2);  

One line.

Fixed

Hi Marcus,

Grumpy maintainer moment: If you've accepted feedback don't send yet more email. Just have it incorporated in next version + in changes logs. It's on a few seconds for people to open each email, but it adds up over time!

Jonathan

Quoted 9 lines
Quoted 5 lines
Signature · 3 lines
-- 
With Best Regards,
Andy Shevchenko  

Thanks,
Marcus Folkesson

author maintainer
#23 lore raw

PATCH 9/10 iio: adc: mcp3911: make use of the sign bit

In reply to Marcus Folkesson

The device supports negative values as well.

---
 drivers/iio/adc/mcp3911.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/adc/mcp3911.c +2 -0
index aefc5eac874c..084f6d1aa6d1 100644
@@ -144,6 +144,8 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			goto out;
 
+		*val = sign_extend32(*val, 23);
+
 		ret = IIO_VAL_INT;
 		break;
 
Signature · 1 line
2.36.1
author maintainer
#24 lore raw

PATCH 10/10 iio: adc: mcp3911: add support to set PGA

In reply to Marcus Folkesson

Add support for setting the Programmable Gain Amplifiers by adjust the scale value.

---
 drivers/iio/adc/mcp3911.c | 135 +++++++++++++++++++++++++++++++-------
 1 file changed, 110 insertions(+), 25 deletions(-)
drivers/iio/adc/mcp3911.c +110 -25
index 084f6d1aa6d1..631015ead8e7 100644
@@ -26,6 +26,8 @@
 #define MCP3911_REG_MOD			0x06
 #define MCP3911_REG_PHASE		0x07
 #define MCP3911_REG_GAIN		0x09
+#define MCP3911_GAIN_MASK(ch)          (0x7 << 3*ch)
+#define MCP3911_GAIN_VAL(ch, val)      ((val << 3*ch) & MCP3911_GAIN_MASK(ch))
 
 #define MCP3911_REG_STATUSCOM		0x0a
 #define MCP3911_STATUSCOM_DRHIZ         BIT(12)
@@ -55,8 +57,10 @@
 #define MCP3911_REG_WRITE(reg, id)	((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
 
 #define MCP3911_NUM_CHANNELS		2
+#define MCP3911_NUM_SCALES		6
 
 static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};
+static u32 mcp3911_scale_table[MCP3911_NUM_SCALES][2];
 
 struct mcp3911 {
 	struct spi_device *spi;
@@ -65,6 +69,7 @@ struct mcp3911 {
 	struct clk *clki;
 	u32 dev_addr;
 	struct iio_trigger *trig;
+	u32 gain[MCP3911_NUM_CHANNELS];
 	struct {
 		u32 channels[2];
 		s64 ts __aligned(8);
@@ -113,7 +118,35 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,
 	return mcp3911_write(adc, reg, val, len);
 }
 
+static int mcp3911_get_gain(struct mcp3911 *adc, u8 channel, u32 *val)
+{
+	int ret = mcp3911_read(adc, MCP3911_REG_GAIN, val, 1);
+
+	if (ret)
+		return ret;
 
+	*val >>= channel * 3;
+	*val &= 0x07;
+	*val = (1 << *val);
+
+	return 0;
+}
+
+static int mcp3911_write_raw_get_fmt(struct iio_dev *indio_dev,
+					struct iio_chan_spec const *chan,
+					long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_PHASE:
+		return IIO_VAL_INT;
+	default:
+		return IIO_VAL_INT_PLUS_NANO;
+	}
+}
 static int mcp3911_read_avail(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     const int **vals, int *type, int *length,
@@ -125,6 +158,11 @@ static int mcp3911_read_avail(struct iio_dev *indio_dev,
 		*vals = mcp3911_osr_table;
 		*length = ARRAY_SIZE(mcp3911_osr_table);
 		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		*type = IIO_VAL_INT_PLUS_NANO;
+		*vals = (int *)mcp3911_scale_table;
+		*length = ARRAY_SIZE(mcp3911_scale_table) * 2;
+		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
 	}
@@ -180,29 +218,9 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,
 		break;
 
 	case IIO_CHAN_INFO_SCALE:
-		if (adc->vref) {
-			ret = regulator_get_voltage(adc->vref);
-			if (ret < 0) {
-				dev_err(indio_dev->dev.parent,
-					"failed to get vref voltage: %d\n",
-				       ret);
-				goto out;
-			}
-
-			*val = ret / 1000;
-		} else {
-			*val = MCP3911_INT_VREF_MV;
-		}
-
-		/*
-		 * For 24bit Conversion
-		 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
-		 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
-		 */
-
-		/* val2 = (2^23 * 1.5) */
-		*val2 = 12582912;
-		ret = IIO_VAL_FRACTIONAL;
+		*val = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][0];
+		*val2 = mcp3911_scale_table[ilog2(adc->gain[channel->channel])][1];
+		ret = IIO_VAL_INT_PLUS_NANO;
 		break;
 	}
 
@@ -220,6 +238,19 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 
 	mutex_lock(&adc->lock);
 	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		for (int i = 0; i < MCP3911_NUM_SCALES; i++) {
+			if (val == mcp3911_scale_table[i][0] &&
+				val2 == mcp3911_scale_table[i][1]) {
+
+				adc->gain[channel->channel] = BIT(i);
+				ret = mcp3911_update(adc, MCP3911_REG_GAIN,
+						MCP3911_GAIN_MASK(channel->channel),
+						MCP3911_GAIN_VAL(channel->channel,
+							i), 1);
+			}
+		}
+		break;
 	case IIO_CHAN_INFO_OFFSET:
 		if (val2 != 0) {
 			ret = -EINVAL;
@@ -290,6 +321,49 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int mcp3911_calc_scale_table(struct mcp3911 *adc)
+{
+	u32 ref = MCP3911_INT_VREF_MV;
+	u32 div;
+	int ret = 0;
+	int tmp0, tmp1;
+	s64 tmp2;
+
+	if (adc->vref) {
+		ret = regulator_get_voltage(adc->vref);
+		if (ret < 0) {
+			dev_err(&adc->spi->dev,
+				"failed to get vref voltage: %d\n",
+			       ret);
+			goto out;
+		}
+
+		ref = ret / 1000;
+	}
+
+	/*
+	 * For 24bit Conversion
+	 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
+	 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
+	 */
+
+	/* ref = Reference voltage
+	 * div = (2^23 * 1.5 * gain) = 12582912 * gain
+	 */
+	for (int i = 0; i < MCP3911_NUM_SCALES; i++) {
+		div = 12582912 * BIT(i);
+		tmp2 = div_s64((s64)ref * 1000000000LL, div);
+		tmp1 = div;
+		tmp0 = (int)div_s64_rem(tmp2, 1000000000, &tmp1);
+
+		mcp3911_scale_table[i][0] = 0;
+		mcp3911_scale_table[i][1] = tmp1;
+	}
+
+out:
+	return ret;
+}
+
 #define MCP3911_CHAN(idx) {					\
 		.type = IIO_VOLTAGE,				\
 		.indexed = 1,					\
@@ -302,8 +376,10 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
 			BIT(IIO_CHAN_INFO_OFFSET) |		\
 			BIT(IIO_CHAN_INFO_SCALE),		\
-		.info_mask_shared_by_type_available =		\
-			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+		.info_mask_shared_by_type_available =           \
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)	\
+		.info_mask_separate_available =			\
+			BIT(IIO_CHAN_INFO_SCALE),		\
 		.scan_type = {					\
 			.sign = 's',				\
 			.realbits = 24,				\
@@ -356,6 +432,7 @@ static const struct iio_info mcp3911_info = {
 	.read_raw = mcp3911_read_raw,
 	.write_raw = mcp3911_write_raw,
 	.read_avail = mcp3911_read_avail,
+	.write_raw_get_fmt = mcp3911_write_raw_get_fmt,
 };
 
 static irqreturn_t mcp3911_interrupt(int irq, void *dev_id)
@@ -482,6 +559,14 @@ static int mcp3911_probe(struct spi_device *spi)
 	if (ret)
 		goto clk_disable;
 
+	ret = mcp3911_calc_scale_table(adc);
+	if (ret)
+		goto clk_disable;
+
+       /* Store gain values to better calculate scale values */
+	mcp3911_get_gain(adc, 0, &adc->gain[0]);
+	mcp3911_get_gain(adc, 1, &adc->gain[1]);
+
 	dr_hiz = device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz");
 	if (dr_hiz)
 		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
Signature · 1 line
2.36.1
author maintainer
#25 lore raw

Re: [PATCH 10/10] iio: adc: mcp3911: add support to set PGA

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 380

Quoted 4 lines · Marcus Folkesson
drivers/iio/adc/mcp3911.c +2 -2
-		.info_mask_shared_by_type_available =		\
-			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
+		.info_mask_shared_by_type_available =           \
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)	\

This change was not supposed to sneak in. Removed in v2.

/Marcus

#26 lore raw

Re: [PATCH 10/10] iio: adc: mcp3911: add support to set PGA

In reply to Marcus Folkesson
Quoted 3 lines · Marcus Folkesson

Add support for setting the Programmable Gain Amplifiers by adjust the scale value.

...

drivers/iio/adc/mcp3911.c static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, · line 126

Quoted 4 lines
drivers/iio/adc/mcp3911.c +4 -0
+       int ret = mcp3911_read(adc, MCP3911_REG_GAIN, val, 1);
+
+       if (ret)
+               return ret;

Please split the assignment.

int ret;

ret = ...
if (ret)

...

drivers/iio/adc/mcp3911.c static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, · line 129

Quoted 2 lines
drivers/iio/adc/mcp3911.c +2 -0
+       *val >>= channel * 3;
+       *val &= 0x07;

GENMASK() ?

Quoted 1 line

+ *val = (1 << *val);

Unneeded parentheses, perhaps BIT()?

...

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 250

Quoted 4 lines
drivers/iio/adc/mcp3911.c +4 -0
+                               ret = mcp3911_update(adc, MCP3911_REG_GAIN,
+                                               MCP3911_GAIN_MASK(channel->channel),
+                                               MCP3911_GAIN_VAL(channel->channel,
+                                                       i), 1);

This is not good indentation, at least i), should be on the previous line.

...

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 328

Quoted 6 lines
drivers/iio/adc/mcp3911.c +5 -0
+static int mcp3911_calc_scale_table(struct mcp3911 *adc)
+{
+       u32 ref = MCP3911_INT_VREF_MV;
+       u32 div;
 
+       int ret = 0;

Useless assignment.

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 338

Quoted 11 lines
drivers/iio/adc/mcp3911.c +10 -0
+       int tmp0, tmp1;
+       s64 tmp2;
+
+       if (adc->vref) {
+               ret = regulator_get_voltage(adc->vref);
+               if (ret < 0) {
+                       dev_err(&adc->spi->dev,
+                               "failed to get vref voltage: %d\n",
+                              ret);
 
+                       goto out;

Return directly.

Quoted 15 lines
drivers/iio/adc/mcp3911.c +15 -0
+               }
+
+               ref = ret / 1000;
+       }
+
+       /*
+        * For 24bit Conversion
+        * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
+        * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
+        */
+
+       /* ref = Reference voltage
+        * div = (2^23 * 1.5 * gain) = 12582912 * gain
+        */
+       for (int i = 0; i < MCP3911_NUM_SCALES; i++) {

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 363

Quoted 10 lines
drivers/iio/adc/mcp3911.c +9 -0
+               div = 12582912 * BIT(i);
+               tmp2 = div_s64((s64)ref * 1000000000LL, div);
+               tmp1 = div;
+               tmp0 = (int)div_s64_rem(tmp2, 1000000000, &tmp1);
+
+               mcp3911_scale_table[i][0] = 0;
+               mcp3911_scale_table[i][1] = tmp1;
+       }
 
+out:

The useless label.

Quoted 1 line

+ return ret;

return 0;

Quoted 1 line

+}

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author maintainer
#27 lore raw

Re: [PATCH 10/10] iio: adc: mcp3911: add support to set PGA

Quoted 13 lines · Andy Shevchenko
Quoted 3 lines · Marcus Folkesson

Add support for setting the Programmable Gain Amplifiers by adjust the scale value.

...

drivers/iio/adc/mcp3911.c static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, · line 126

Quoted 4 lines
drivers/iio/adc/mcp3911.c +4 -0
+       int ret = mcp3911_read(adc, MCP3911_REG_GAIN, val, 1);
+
+       if (ret)
+               return ret;
Quoted 6 lines

Please split the assignment.

int ret;

ret = ...
if (ret)

OK

Quoted 8 lines

...

drivers/iio/adc/mcp3911.c static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, · line 129

Quoted 2 lines
drivers/iio/adc/mcp3911.c +2 -0
+       *val >>= channel * 3;
+       *val &= 0x07;

GENMASK() ?

I will use GENMASK

Quoted 4 lines
Quoted 2 lines

+ *val = (1 << *val);

Unneeded parentheses, perhaps BIT()?

I will switch to BIT()

Quoted 9 lines

...

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 250

Quoted 4 lines
drivers/iio/adc/mcp3911.c +4 -0
+                               ret = mcp3911_update(adc, MCP3911_REG_GAIN,
+                                               MCP3911_GAIN_MASK(channel->channel),
+                                               MCP3911_GAIN_VAL(channel->channel,
+                                                       i), 1);

This is not good indentation, at least i), should be on the previous line.

Agree

Quoted 11 lines

...

drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 328

Quoted 6 lines
drivers/iio/adc/mcp3911.c +5 -0
+static int mcp3911_calc_scale_table(struct mcp3911 *adc)
+{
+       u32 ref = MCP3911_INT_VREF_MV;
+       u32 div;
 
+       int ret = 0;

Useless assignment.

Agree

Quoted 3 lines

Return directly.

OK

Quoted 3 lines

The useless label.

Agree

Quoted 3 lines
Quoted 2 lines

+ return ret;

Thanks,
Marcus Folkesson

#28 lore raw
In reply to Marcus Folkesson
Quoted 2 lines · Marcus Folkesson

Go for the right property name that is documented in the bindings.

If the driver is already for a while in the kernel, I'm afraid we may not do this, since it's part of ABI (firmware <--> OS). You can add a new property and try it first.

Signature · 2 lines
With Best Regards,
Andy Shevchenko