Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 25 Jun 2022 12:45:37 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Marcus Folkesson <marcus.folkesson@gmail.com>,
        Rob Herring <robh+dt@kernel.org>
Cc:     Kent Gustavsson <kent@minoris.se>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 03/10] iio: adc: mcp3911: add support for buffers
Message-ID: <20220625124537.163bf426@jic23-huawei>
In-Reply-To: <20220625103853.2470346-3-marcus.folkesson@gmail.com>
References: <20220625103853.2470346-1-marcus.folkesson@gmail.com>
        <20220625103853.2470346-3-marcus.folkesson@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;