diff mbox series

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

Message ID 20220625103853.2470346-3-marcus.folkesson@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series [v2,01/10] iio: adc: mcp3911: correct "microchip,device-addr" property | expand

Commit Message

Marcus Folkesson June 25, 2022, 10:38 a.m. UTC
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(-)

Comments

Jonathan Cameron June 25, 2022, 11:45 a.m. UTC | #1
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;
Marcus Folkesson June 30, 2022, 8:32 a.m. UTC | #2
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
Jonathan Cameron July 7, 2022, 5:57 p.m. UTC | #3
> > >  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!
diff mbox series

Patch

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;