[PATCH v2 01/10] iio: adc: mcp3911: correct "microchip,device-addr" property

[PATCH v2 01/10] iio: adc: mcp3911: correct "microchip,device-addr" property

[Marcus Folkesson]

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

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - Fallback to "device-addr" due to compatibility (Andy Shevchenko)

 drivers/iio/adc/mcp3911.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 1cb4590fe412..c5a0f19d7834 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -208,7 +208,13 @@ static int mcp3911_config(struct mcp3911 *adc)
 	u32 configreg;
 	int ret;
 
-	device_property_read_u32(dev, "device-addr", &adc->dev_addr);
+	ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);
+
+	/* Fallback to "device-addr" due to historical mismatch between
+	 * dt-bindings and implementation
+	 */
+	if (ret)
+		device_property_read_u32(dev, "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",
-- 
2.36.1

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

[Marcus Folkesson]

Keep using managed resources as much as possible.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - No changes

 drivers/iio/adc/mcp3911.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index c5a0f19d7834..25a235cce56c 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -312,7 +312,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;
 
@@ -332,8 +332,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);
-- 
2.36.1

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

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:45 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Keep using managed resources as much as possible.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
See explanation on why this is wrong sent to the v1 version of this patch.

It's a fiddly and not always well understood area (here be dragons!).

Jonathan

> ---
> 
> Notes:
>     v2:
>         - No changes
> 
>  drivers/iio/adc/mcp3911.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index c5a0f19d7834..25a235cce56c 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -312,7 +312,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;
>  
> @@ -332,8 +332,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);

[PATCH v2 03/10] iio: adc: mcp3911: add support for buffers

[Marcus Folkesson]

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

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - No changes

 drivers/iio/adc/mcp3911.c | 58 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 25a235cce56c..2a4bf374f140 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -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,
@@ -303,7 +353,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);
 
@@ -312,6 +362,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;
-- 
2.36.1

Re: [PATCH v2 03/10] iio: adc: mcp3911: add support for buffers

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:46 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Add support for buffers to make the driver fit for more usecases.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Hi Marcus,

Good to see this feature.  A few comments inline, mostly around
optimising the data flow / device accesses.

Thanks,

Jonathan

> ---
> 
> Notes:
>     v2:
>         - No changes
> 
>  drivers/iio/adc/mcp3911.c | 58 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 25a235cce56c..2a4bf374f140 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -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);

I was a bit surprised not to see some overlap of address setting and
read out here if both channels are enabled, so opened up the data sheet.

Can we take advantage of "Continuous communication looping on address set"
(6.7 on the datasheet).  Also for buffered capture we'd normally make
things like shifting and endian conversion a userspace problem.  Can we
not do that here for some reason?  You'd need to take care to ensure
any buffers that might be used directly for DMA obey DMA safety rules
(currently avoided by using spi_write_then_read), but it would be
nice to do less data manipulation int his path if we can.



> +
> +		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,
> @@ -303,7 +353,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;

The core sets INDIO_BUFFER_TRIGGERED as part of devm_iio_triggered_buffer_setup()
so you need to set DIRECT_MODE here (that one isn't visible to the core)

>  	indio_dev->info = &mcp3911_info;
>  	spi_set_drvdata(spi, indio_dev);
>  
> @@ -312,6 +362,12 @@ static int mcp3911_probe(struct spi_device *spi)
>  
>  	mutex_init(&adc->lock);
>  
> +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
Can't use devm here for the same reason it was inappropriate in patch 2.

I'd suggest a precursor patch(es) to move the driver fully over to
devm_ managed such that you don't need a remove function and the ordering
is maintained.
> +			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;

Re: [PATCH v2 03/10] iio: adc: mcp3911: add support for buffers

[Marcus Folkesson]

[-- Attachment #1: Type: text/plain, Size: 5727 bytes --]

Hi Jonathan,

On Sat, Jun 25, 2022 at 12:45:37PM +0100, Jonathan Cameron wrote:
> On Sat, 25 Jun 2022 12:38:46 +0200
> Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
> 
> > Add support for buffers to make the driver fit for more usecases.
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Hi Marcus,
> 
> Good to see this feature.  A few comments inline, mostly around
> optimising the data flow / device accesses.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > 
> > Notes:
> >     v2:
> >         - No changes
> > 
> >  drivers/iio/adc/mcp3911.c | 58 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 57 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > index 25a235cce56c..2a4bf374f140 100644
> > --- a/drivers/iio/adc/mcp3911.c
> > +++ b/drivers/iio/adc/mcp3911.c
> > @@ -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);
> 
> I was a bit surprised not to see some overlap of address setting and
> read out here if both channels are enabled, so opened up the data sheet.
> 
> Can we take advantage of "Continuous communication looping on address set"

Sure, I will make it use continuous reads when both channels are enabled,
thanks.

> (6.7 on the datasheet).  Also for buffered capture we'd normally make
> things like shifting and endian conversion a userspace problem.  Can we
> not do that here for some reason?  You'd need to take care to ensure

The endian conversion&shifting was actually the reason for why I did not
make use of continoues reads from the beginning.

> any buffers that might be used directly for DMA obey DMA safety rules
> (currently avoided by using spi_write_then_read), but it would be
> nice to do less data manipulation int his path if we can.

I will change to spi_sync() and skip the data manipulation.

> 
> 
> 
> > +
> > +		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,
> > @@ -303,7 +353,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;
> 
> The core sets INDIO_BUFFER_TRIGGERED as part of devm_iio_triggered_buffer_setup()
> so you need to set DIRECT_MODE here (that one isn't visible to the core)

Ok, thank you. I sent patches that fixes this in two other ADC-drivers
as well to avoid more people following the same thing.

> 
> >  	indio_dev->info = &mcp3911_info;
> >  	spi_set_drvdata(spi, indio_dev);
> >  
> > @@ -312,6 +362,12 @@ static int mcp3911_probe(struct spi_device *spi)
> >  
> >  	mutex_init(&adc->lock);
> >  
> > +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> Can't use devm here for the same reason it was inappropriate in patch 2.
> 
> I'd suggest a precursor patch(es) to move the driver fully over to
> devm_ managed such that you don't need a remove function and the ordering
> is maintained.

Yep, I will keep this and fix patch 2 instead.

> > +			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;
> 

/Marcus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 03/10] iio: adc: mcp3911: add support for buffers

[Jonathan Cameron]

> > >  static const struct iio_info mcp3911_info = {
> > >  	.read_raw = mcp3911_read_raw,
> > >  	.write_raw = mcp3911_write_raw,
> > > @@ -303,7 +353,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;  
> > 
> > The core sets INDIO_BUFFER_TRIGGERED as part of devm_iio_triggered_buffer_setup()
> > so you need to set DIRECT_MODE here (that one isn't visible to the core)  
> 
> Ok, thank you. I sent patches that fixes this in two other ADC-drivers
> as well to avoid more people following the same thing.

Thanks. Much appreciated!

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

[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.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - Removed blank lines (Andy Shevchenko)
        - Removed dr_hiz variable (Andy Shevchenko)

 drivers/iio/adc/mcp3911.c | 65 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 2a4bf374f140..f4ee0c27c2ab 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -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;
@@ -298,6 +311,23 @@ 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;
@@ -352,6 +382,15 @@ static int mcp3911_probe(struct spi_device *spi)
 	if (ret)
 		goto clk_disable;
 
+	if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-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;
@@ -362,6 +401,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);
-- 
2.36.1

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

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:47 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> 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.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Hi Marcus,

A few minor things inline.

Jonathan

> ---
> 
> Notes:
>     v2:
>         - Removed blank lines (Andy Shevchenko)
>         - Removed dr_hiz variable (Andy Shevchenko)
> 
>  drivers/iio/adc/mcp3911.c | 65 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 2a4bf374f140..f4ee0c27c2ab 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -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))

Hmm. So I think this protection is only relevant for races around
disabling of the trigger.  Those shouldn't matter as just look
like the actual write to disable the trigger was a bit later relative
to the asynchronous capture of data.  So I think you can drop it.
If it is here for some other reason please ad a comment.

With that dropped you can use 
iio_trigger_generic_data_rdy_poll() for your interrupt handler
(as it's identical to the rest of this code).

> +		iio_trigger_poll(adc->trig);
> +
> +	return IRQ_HANDLED;
> +};
> +
>  static int mcp3911_config(struct mcp3911 *adc)
>  {
>  	struct device *dev = &adc->spi->dev;
> @@ -298,6 +311,23 @@ 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;
> @@ -352,6 +382,15 @@ static int mcp3911_probe(struct spi_device *spi)
>  	if (ret)
>  		goto clk_disable;
>  
> +	if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-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;
> @@ -362,6 +401,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;
You definitely want to use devm managed cleanup for these.

There is a patch set that adds these as standard functions, but I haven't
yet seen it being picked up for this cycle (reviews have been slow coming).

https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/

In meantime role your own with devm_add_action_or_reset()

> +
> +		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
> +		 */
Gah. Always annoying when devices don't support masking. Your handling is indeed
the best we can do.
> +		ret = devm_request_irq(&spi->dev, spi->irq,
> +				&mcp3911_interrupt,
> +				IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN | IRQF_ONESHOT,
Don't set trigger_falling in the driver, rely on the firmware bindings to do that
for you as there may well be inverters in the path on some boards that aren't
visible to Linux.   We used to always do it in the driver and unfortunately are stuck
with it where already present to avoid breaking boards that previously worked.

We don't want to introduce more cases of this pattern though!

Jonathan

> +				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);

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

[Jonathan Cameron]

...

> >  static int mcp3911_probe(struct spi_device *spi)
> >  {
> >  	struct iio_dev *indio_dev;
> > @@ -352,6 +382,15 @@ static int mcp3911_probe(struct spi_device *spi)
> >  	if (ret)
> >  		goto clk_disable;
> >  
> > +	if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-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;
> > @@ -362,6 +401,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;  
> You definitely want to use devm managed cleanup for these.
> 
> There is a patch set that adds these as standard functions, but I haven't
> yet seen it being picked up for this cycle (reviews have been slow coming).
> 
> https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/

Ah, elsewhere in my unread email was a thread that says it's in clk-next so
will be in the next merge window.  I haven't confirmed, but looks like Stephen
put up an immutable branch so I could pull it into the IIO togreg branch if you
want to transition directly to that new code. @Stephen, would you be fine
with me pulling your clk-devm-enable branch into IIO (with the fix which
got posted earlier in the week presumably also going on that branch when
you push out?)

Thanks,

Jonathan



> 
> In meantime role your own with devm_add_action_or_reset()

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

[Stephen Boyd]

Quoting Jonathan Cameron (2022-06-25 05:06:37)
> > > @@ -362,6 +401,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;  
> > You definitely want to use devm managed cleanup for these.
> > 
> > There is a patch set that adds these as standard functions, but I haven't
> > yet seen it being picked up for this cycle (reviews have been slow coming).
> > 
> > https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/
> 
> Ah, elsewhere in my unread email was a thread that says it's in clk-next so
> will be in the next merge window.  I haven't confirmed, but looks like Stephen
> put up an immutable branch so I could pull it into the IIO togreg branch if you
> want to transition directly to that new code. @Stephen, would you be fine
> with me pulling your clk-devm-enable branch into IIO (with the fix which
> got posted earlier in the week presumably also going on that branch when
> you push out?)

Yes that's fine. Thanks for the heads up.

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

[Marcus Folkesson]

[-- Attachment #1: Type: text/plain, Size: 2213 bytes --]

On Sat, Jun 25, 2022 at 01:06:37PM +0100, Jonathan Cameron wrote:
> 
> ...
> 
> > >  static int mcp3911_probe(struct spi_device *spi)
> > >  {
> > >  	struct iio_dev *indio_dev;
> > > @@ -352,6 +382,15 @@ static int mcp3911_probe(struct spi_device *spi)
> > >  	if (ret)
> > >  		goto clk_disable;
> > >  
> > > +	if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-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;
> > > @@ -362,6 +401,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;  
> > You definitely want to use devm managed cleanup for these.
> > 
> > There is a patch set that adds these as standard functions, but I haven't
> > yet seen it being picked up for this cycle (reviews have been slow coming).
> > 
> > https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/
> 
> Ah, elsewhere in my unread email was a thread that says it's in clk-next so
> will be in the next merge window.  I haven't confirmed, but looks like Stephen
> put up an immutable branch so I could pull it into the IIO togreg branch if you
> want to transition directly to that new code. @Stephen, would you be fine
> with me pulling your clk-devm-enable branch into IIO (with the fix which
> got posted earlier in the week presumably also going on that branch when
> you push out?)

Please do, thanks

> 
> Thanks,
> 
> Jonathan
> 
> 
> 
> > 
> > In meantime role your own with devm_add_action_or_reset()

Best regards,
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Marcus Folkesson]

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

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - Removed default value and changed description (Krzysztof Kozlo)

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

diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
index 95ab285f4eba..57a16380f864 100644
--- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
@@ -36,6 +36,13 @@ properties:
     description: IRQ line of the ADC
     maxItems: 1
 
+  microchip,data-ready-hiz:
+    description:
+      Data Ready Pin Inactive State Control
+      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
+
   microchip,device-addr:
     description: Device address when multiple MCP3911 chips are present on the same SPI bus.
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.36.1

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

[Krzysztof Kozlowski]

On 25/06/2022 12:38, Marcus Folkesson wrote:
> The Data Ready Output Pin is either hard wired to work as high
> impedance or push-pull. Make it configurable.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

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

[Marcus Folkesson]

The chip supports oversampling ratio, so expose it to userspace.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - Make use of osr table
        - Change formatting and typos

 drivers/iio/adc/mcp3911.c | 47 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index f4ee0c27c2ab..1469c12ebbb2 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -5,6 +5,8 @@
  * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
  * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
  */
+#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,17 @@ 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:
+		for (int i = 0; i < sizeof(mcp3911_osr_table); i++) {
+			if (val == mcp3911_osr_table[i]) {
+				val = FIELD_PREP(MCP3911_CONFIG_OSR, i);
+				ret = mcp3911_update(adc, MCP3911_REG_CONFIG, MCP3911_CONFIG_OSR,
+						val, 2);
+				break;
+			}
+		}
+		break;
 	}
 
 out:
@@ -198,9 +240,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 +298,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)
-- 
2.36.1

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

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:49 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> The chip supports oversampling ratio, so expose it to userspace.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Hi Marcus,

A few minor comments inline.

Thanks,

Jonathan


> ---
> 
> Notes:
>     v2:
>         - Make use of osr table
>         - Change formatting and typos
> 
>  drivers/iio/adc/mcp3911.c | 47 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index f4ee0c27c2ab..1469c12ebbb2 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -5,6 +5,8 @@
>   * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
>   * Copyright (C) 2018 Kent Gustavsson <kent@minoris.se>
>   */
> +#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,17 @@ 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:
> +		for (int i = 0; i < sizeof(mcp3911_osr_table); i++) {
> +			if (val == mcp3911_osr_table[i]) {

Default type of IIO writes is IIO_VAL_INT_PLUS_MICRO.  You can either provide
write_raw_get_fmt() or be lazy and check val2 == 0 here.
 
> +				val = FIELD_PREP(MCP3911_CONFIG_OSR, i);
> +				ret = mcp3911_update(adc, MCP3911_REG_CONFIG, MCP3911_CONFIG_OSR,
> +						val, 2);
> +				break;
> +			}
> +		}
> +		break;
>  	}
>  
>  out:
> @@ -198,9 +240,13 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,
>  		.indexed = 1,					\
>  		.channel = idx,					\
>  		.scan_index = idx,				\
> +		.scan_index = idx,				\

repeated... I guess a merge conflict resolution issue.

> +		.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 +298,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)

[PATCH v2 07/10] iio: adc: mcp3911: use correct formula for AD conversion

[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.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - No changes

 drivers/iio/adc/mcp3911.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 1469c12ebbb2..ede1ad97ed4d 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -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;
 	}
 
-- 
2.36.1

Re: [PATCH v2 07/10] iio: adc: mcp3911: use correct formula for AD conversion

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:50 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> The ADC conversion is actually not rail-to-rail but with a factor 1.5.
> Make use of this factor when calculating actual voltage.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Fixes tag?  Also, fixes should be at the beginning of the patch set
to make it easier to backport them to stable kernels etc.

Otherwise looks good to me.

Thanks,

Jonathan


> ---
> 
> Notes:
>     v2:
>         - No changes
> 
>  drivers/iio/adc/mcp3911.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 1469c12ebbb2..ede1ad97ed4d 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -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;
>  	}
>  

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

[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.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - Fix formatting (Andy Schevchenko)

 drivers/iio/adc/mcp3911.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index ede1ad97ed4d..a0609d7663e1 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -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,15 @@ 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:
 		for (int i = 0; i < sizeof(mcp3911_osr_table); i++) {
 			if (val == mcp3911_osr_table[i]) {
@@ -248,7 +268,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),		\
-- 
2.36.1

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

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:51 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> 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.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Until now I think we've only had the phase modifier for output channels.
So at minimum need to add documentation for it in
Documentation/ABI/testing/sysfs-bus-iio

However, the snag is that it's defined in terms of radians.
The usecase here assumes that the sensor is measuring some sort of
wave form, but unfortunately we don't know what that is - hence
the setting is in terms of clock delay.

As such, though the datasheet calls if phase, I think that is
stretching the meaning too far in the IIO ABI. We probably need
something new.  

Years ago, for devices that are actually a single ADC and a MUX
where we pretend in IIO that the channels are sampled synchronously
we talked about provided the timing delay information to userspace.
Nothing ever came of it, but that is effectively the same concept
as you have here.

So, it's a time measurement so units will need to be seconds -
userspace has no idea of the clk speed of a device. For two channels
the relationship is straight forward, but I wonder for 3 channel devices
how we would handle it.  The two different sources of this delay might
lead to different controls being optimal.

Naming wise, perhaps samplingdelay?

If you have actual ADCs that operate independently then relationship to
a base reference point will be independent. 
So for a 3 channel device you'd have

in_voltage0_samplingdelay  0
in_voltage1_samplingdelay  Phase register 1 code / DMCLK
in_voltage2_samplingdelay  Phase register 2 code / DMCLK

But for a device that is a mux in front of one actual ADC
then the timing is likely to be relative to previous channel
Hence if all turned on...

in_voltage0_samplingdelay  0
in_voltage1_samplingdelay  Phase register 1 code / DMCLK
in_voltage2_samplingdelay  Phase register 2 code + Phase register 1 code / DMCLK

If only 0 and 2 enabled.

in_voltage0_samplingdelay  0
in_voltage2_samplingdelay  Phase register X code

However we can probably just make that problem for the driver. Sometimes
we'll have to reject or approximate particular combinations of enabled channels
and requested delays. 
One corner case that is nasty will be if there is just one controllable delay.
In that case it would seem natural to have just one attribute, but the delay
would be cumulative across multiple enabled channels.  For that I think
we'd just need different ABI.

in_voltage_intersampledelay  maybe?  With two channels the various options
would all work but we should think ahead...

There is another complexity. These values apply to the buffered data, not
otherwise. Moving them into bufferX/ would nicely associate them with the
enabled channels and make it more obvious that there is a coupling there

However, it is more complex to add attributes to the buffers..
If we think that is the right way to go for ABI it wouldn't be too hard to
add to the core - but will need a new callback.

So my gut feeling is that this should be

bufferX/in_voltage0_samplingdelay 0
bufferX/in_voltage1_samplingdelay Phase register 1 code / DMCLK seconds
but it is a rather nasty layering violation.

That will require us adding a new callback read_scan_el_raw() and appropriate
enum etc.

Things will get more complex for 3 channel deviceson multibuffer devices or when there are in
kernel consumers (as those may effect the enabled channels but aren't visible in
bufferX).  However, I don't see it being that likely we'll get that combination
of features any time soon (famous last words!)

Gut feeling is that adding this feature (and discussion of ABI) will
take a while, but it shouldn't block picking up the rest of the series
in the meantime.

Jonathan


> ---
> 
> Notes:
>     v2:
>         - Fix formatting (Andy Schevchenko)
> 
>  drivers/iio/adc/mcp3911.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index ede1ad97ed4d..a0609d7663e1 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -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,15 @@ 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:
>  		for (int i = 0; i < sizeof(mcp3911_osr_table); i++) {
>  			if (val == mcp3911_osr_table[i]) {
> @@ -248,7 +268,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),		\

[PATCH v2 09/10] iio: adc: mcp3911: make use of the sign bit

[Marcus Folkesson]

The device supports negative values as well.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - No changes

 drivers/iio/adc/mcp3911.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index a0609d7663e1..a019264e73e3 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -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;
 
-- 
2.36.1

Re: [PATCH v2 09/10] iio: adc: mcp3911: make use of the sign bit

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:52 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> The device supports negative values as well.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
This looks like a fix to me.  So fixes tag and move it to the start
of the series.

Jonathan

> ---
> 
> Notes:
>     v2:
>         - No changes
> 
>  drivers/iio/adc/mcp3911.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index a0609d7663e1..a019264e73e3 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -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;
>  

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

[Marcus Folkesson]

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

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---

Notes:
    v2:
        - add missing comma
        - Address comments from Andy Shevchenko

 drivers/iio/adc/mcp3911.c | 134 +++++++++++++++++++++++++++++++-------
 1 file changed, 110 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index a019264e73e3..f0179385485f 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -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,6 +118,37 @@ 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;
+
+	ret = mcp3911_read(adc, MCP3911_REG_GAIN, val, 1);
+	if (ret)
+		return ret;
+
+	*val >>= channel * 3;
+	*val &= GENMASK(2, 0);
+	*val = BIT(*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,
@@ -124,6 +160,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 +221,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 +241,18 @@ 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;
@@ -264,6 +297,48 @@ 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;
+	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);
+			return ret;
+		}
+
+		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;
+	}
+
+	return 0;
+}
+
 #define MCP3911_CHAN(idx) {					\
 		.type = IIO_VOLTAGE,				\
 		.indexed = 1,					\
@@ -276,8 +351,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 =		\
+		.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,				\
@@ -330,6 +407,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)
@@ -460,6 +538,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]);
+
 	if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
 		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
 				0, 2);
-- 
2.36.1

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

[kernel test robot]

Hi Marcus,

I love your patch! Perhaps something to improve:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v5.19-rc3 next-20220624]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Marcus-Folkesson/iio-adc-mcp3911-correct-microchip-device-addr-property/20220625-184118
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-randconfig-a001
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/def017eb4efc80ab515495d6eb7d59d142c0276d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Marcus-Folkesson/iio-adc-mcp3911-correct-microchip-device-addr-property/20220625-184118
        git checkout def017eb4efc80ab515495d6eb7d59d142c0276d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/iio/adc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/iio/adc/mcp3911.c: In function 'mcp3911_calc_scale_table':
>> drivers/iio/adc/mcp3911.c:305:13: warning: variable 'tmp0' set but not used [-Wunused-but-set-variable]
     305 |         int tmp0, tmp1;
         |             ^~~~
   drivers/iio/adc/mcp3911.c: At top level:
   drivers/iio/adc/mcp3911.c:366:22: warning: initialized field overwritten [-Woverride-init]
     366 |         MCP3911_CHAN(0),
         |                      ^
   drivers/iio/adc/mcp3911.c:347:31: note: in definition of macro 'MCP3911_CHAN'
     347 |                 .scan_index = idx,                              \
         |                               ^~~
   drivers/iio/adc/mcp3911.c:366:22: note: (near initialization for 'mcp3911_channels[0].scan_index')
     366 |         MCP3911_CHAN(0),
         |                      ^
   drivers/iio/adc/mcp3911.c:347:31: note: in definition of macro 'MCP3911_CHAN'
     347 |                 .scan_index = idx,                              \
         |                               ^~~
   drivers/iio/adc/mcp3911.c:367:22: warning: initialized field overwritten [-Woverride-init]
     367 |         MCP3911_CHAN(1),
         |                      ^
   drivers/iio/adc/mcp3911.c:347:31: note: in definition of macro 'MCP3911_CHAN'
     347 |                 .scan_index = idx,                              \
         |                               ^~~
   drivers/iio/adc/mcp3911.c:367:22: note: (near initialization for 'mcp3911_channels[1].scan_index')
     367 |         MCP3911_CHAN(1),
         |                      ^
   drivers/iio/adc/mcp3911.c:347:31: note: in definition of macro 'MCP3911_CHAN'
     347 |                 .scan_index = idx,                              \
         |                               ^~~


vim +/tmp0 +305 drivers/iio/adc/mcp3911.c

   299	
   300	static int mcp3911_calc_scale_table(struct mcp3911 *adc)
   301	{
   302		u32 ref = MCP3911_INT_VREF_MV;
   303		u32 div;
   304		int ret;
 > 305		int tmp0, tmp1;
   306		s64 tmp2;
   307	
   308		if (adc->vref) {
   309			ret = regulator_get_voltage(adc->vref);
   310			if (ret < 0) {
   311				dev_err(&adc->spi->dev,
   312					"failed to get vref voltage: %d\n",
   313				       ret);
   314				return ret;
   315			}
   316	
   317			ref = ret / 1000;
   318		}
   319	
   320		/*
   321		 * For 24bit Conversion
   322		 * Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5
   323		 * Voltage = Raw * (Vref)/(2^23 * Gain * 1.5)
   324		 */
   325	
   326		/* ref = Reference voltage
   327		 * div = (2^23 * 1.5 * gain) = 12582912 * gain
   328		 */
   329		for (int i = 0; i < MCP3911_NUM_SCALES; i++) {
   330			div = 12582912 * BIT(i);
   331			tmp2 = div_s64((s64)ref * 1000000000LL, div);
   332			tmp1 = div;
   333			tmp0 = (int)div_s64_rem(tmp2, 1000000000, &tmp1);
   334	
   335			mcp3911_scale_table[i][0] = 0;
   336			mcp3911_scale_table[i][1] = tmp1;
   337		}
   338	
   339		return 0;
   340	}
   341	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:53 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Add support for setting the Programmable Gain Amplifiers by adjust the
> scale value.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
A few minor things inline.

Thanks,

Jonathan

> ---
> 
> Notes:
>     v2:
>         - add missing comma
>         - Address comments from Andy Shevchenko
> 
>  drivers/iio/adc/mcp3911.c | 134 +++++++++++++++++++++++++++++++-------
>  1 file changed, 110 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index a019264e73e3..f0179385485f 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -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,6 +118,37 @@ 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;
> +
> +	ret = mcp3911_read(adc, MCP3911_REG_GAIN, val, 1);
> +	if (ret)
> +		return ret;
> +
> +	*val >>= channel * 3;
> +	*val &= GENMASK(2, 0);
> +	*val = BIT(*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;

Ah. here is what I asked for earlier. Please move it back to the
patches that introduced the other features.

> +	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,
> @@ -124,6 +160,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 +221,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 +241,18 @@ 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;
> @@ -264,6 +297,48 @@ 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;
> +	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);
> +			return ret;
> +		}
> +
> +		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

comment syntax is wrong.  Also a bit odd to have two comment
blocks next to each other. Please combine them.

> +	 * 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;
> +	}
> +
> +	return 0;
> +}
> +
>  #define MCP3911_CHAN(idx) {					\
>  		.type = IIO_VOLTAGE,				\
>  		.indexed = 1,					\
> @@ -276,8 +351,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 =		\
> +		.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,				\
> @@ -330,6 +407,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)
> @@ -460,6 +538,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 */

Run checkpatch.pl You have spaces here which should be a tab.

> +	mcp3911_get_gain(adc, 0, &adc->gain[0]);
> +	mcp3911_get_gain(adc, 1, &adc->gain[1]);

We'd normally set something like this to default value rather than
rely on sane state from a previous startup.  Either that or rely on reset
(which device has, but I don't think driver supports yet).



> +
>  	if (device_property_read_bool(&adc->spi->dev, "microchip,data-ready-hiz"))
>  		ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
>  				0, 2);

Re: [PATCH v2 01/10] iio: adc: mcp3911: correct "microchip,device-addr" property

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:44 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Go for the right property name that is documented in the bindings.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Need a fixes tag so we know how far to back port this.

> ---
> 
> Notes:
>     v2:
>         - Fallback to "device-addr" due to compatibility (Andy Shevchenko)
> 
>  drivers/iio/adc/mcp3911.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 1cb4590fe412..c5a0f19d7834 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -208,7 +208,13 @@ static int mcp3911_config(struct mcp3911 *adc)
>  	u32 configreg;
>  	int ret;
>  
> -	device_property_read_u32(dev, "device-addr", &adc->dev_addr);
> +	ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);
> +
> +	/* Fallback to "device-addr" due to historical mismatch between
Multiline comment syntax should be

	/*
	 * Fallabck to "...
	 * dt-...
`	 */

> +	 * dt-bindings and implementation
> +	 */
> +	if (ret)
> +		device_property_read_u32(dev, "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",

Re: [PATCH v2 01/10] iio: adc: mcp3911: correct "microchip,device-addr" property

[Jonathan Cameron]

On Sat, 25 Jun 2022 12:38:44 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Go for the right property name that is documented in the bindings.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Hi Marcus,

Series with more than 1 or 2 patches should always have a cover letter
(git format-patch --cover-letter) to provide some overview information
and allow for general comments on the series.

Also, whilst I know you are keen to move forwards quickly it is usually
a good idea to give more than 2 days for all reviews to come in on a series
and discussion of any questions to finish.

For instance, I just replied to your question to Andy on patch 2 and
that basically says your patch 2 in v2 is taking the wrong approach.
If you'd waited a few more days you'd have save on the noise by resolving
that before sending more patches.

Thanks,

Jonathan


> ---
> 
> Notes:
>     v2:
>         - Fallback to "device-addr" due to compatibility (Andy Shevchenko)
> 
>  drivers/iio/adc/mcp3911.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 1cb4590fe412..c5a0f19d7834 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -208,7 +208,13 @@ static int mcp3911_config(struct mcp3911 *adc)
>  	u32 configreg;
>  	int ret;
>  
> -	device_property_read_u32(dev, "device-addr", &adc->dev_addr);
> +	ret = device_property_read_u32(dev, "microchip,device-addr", &adc->dev_addr);
> +
> +	/* Fallback to "device-addr" due to historical mismatch between
> +	 * dt-bindings and implementation
> +	 */
> +	if (ret)
> +		device_property_read_u32(dev, "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",