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:   Sun, 28 Aug 2022 18:24:30 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Vincent Whitchurch <vincent.whitchurch@axis.com>
Cc:     <kernel@axis.com>, <lars@metafoo.de>, <axel.jonsson@axis.com>,
        <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] iio: adc: mcp320x: add triggered buffer support
Message-ID: <20220828182430.533bf8e0@jic23-huawei>
In-Reply-To: <20220824104002.2749075-3-vincent.whitchurch@axis.com>
References: <20220824104002.2749075-1-vincent.whitchurch@axis.com>
        <20220824104002.2749075-3-vincent.whitchurch@axis.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 24 Aug 2022 12:40:02 +0200
Vincent Whitchurch <vincent.whitchurch@axis.com> wrote:

> From: Axel Jonsson <axel.jonsson@axis.com>
> 
> Add support for triggered buffers.  Just read the channels in a loop to
> keep things simple.

Just curious, but what other options are there?  A quick datasheet scroll
through didn't seem to suggest you can overlap setup of next read with
reading out the previous (common on SPI ADCs).

A few minor additional comments inline.

Thanks,

Jonathan


> 
> Signed-off-by: Axel Jonsson <axel.jonsson@axis.com>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
>  drivers/iio/adc/Kconfig   |   2 +
>  drivers/iio/adc/mcp320x.c | 130 ++++++++++++++++++++++++++++----------
>  2 files changed, 99 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 48ace7412874..9f2628120885 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -696,6 +696,8 @@ config MAX9611
>  config MCP320X
>  	tristate "Microchip Technology MCP3x01/02/04/08 and MCP3550/1/3"
>  	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  Say yes here to build support for Microchip Technology's
>  	  MCP3001, MCP3002, MCP3004, MCP3008, MCP3201, MCP3202, MCP3204,
> diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c
> index 28398f34628a..9782cbd37728 100644
> --- a/drivers/iio/adc/mcp320x.c
> +++ b/drivers/iio/adc/mcp320x.c
> @@ -43,6 +43,10 @@
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  #include <linux/regulator/consumer.h>
>  
>  enum {
> @@ -231,37 +235,51 @@ static int mcp320x_read_raw(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -#define MCP320X_VOLTAGE_CHANNEL(num)				\
> -	{							\
> -		.type = IIO_VOLTAGE,				\
> -		.indexed = 1,					\
> -		.channel = (num),				\
> -		.address = (num),				\
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> -		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
> +#define MCP320X_VOLTAGE_CHANNEL(num)					\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = (num),					\
> +		.address = (num),					\
> +		.scan_index = (num),					\
> +		.scan_type = {						\
> +			.sign = 's',					\
> +			.realbits = 32,					\
> +			.storagebits = 32,				\
> +			.endianness = IIO_CPU,				\
> +		},							\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE)	\
>  	}
>  
> -#define MCP320X_VOLTAGE_CHANNEL_DIFF(chan1, chan2)		\
> -	{							\
> -		.type = IIO_VOLTAGE,				\
> -		.indexed = 1,					\
> -		.channel = (chan1),				\
> -		.channel2 = (chan2),				\
> -		.address = (chan1),				\
> -		.differential = 1,				\
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> -		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
> +#define MCP320X_VOLTAGE_CHANNEL_DIFF(chan1, chan2, diffoff)		\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = (chan1),					\
> +		.channel2 = (chan2),					\
> +		.address = (chan1),					\
> +		.scan_index = (chan1 + diffoff),			\

I wonder if it's just more readable to use scan_index as one of the parameters
of this macro. If it's the first one, then you'll
see 
	*VOLTAGE_CHANNEL(0),
	*VOLTAGE_CHANNEL(1),
	*VOLTAGE_CHANNEL_DIFF(2, 0, 1),
	*VOLTAGE_CHANNEL_DIFF(3, 1, 0),
etc

If we can avoid reformatting this in the same patch that adds the new
stuff it will make it easier to see the changes.

If you want to reformat, add another patch to do that as a no-op change.


> +		.scan_type = {						\
> +			.sign = 's',					\
> +			.realbits = 32,					\
> +			.storagebits = 32,				\
> +			.endianness = IIO_CPU,				\
> +		},							\
> +		.differential = 1,					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE)	\
>  	}
>  
>  static const struct iio_chan_spec mcp3201_channels[] = {
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1, 1),
>  };
>  
>  static const struct iio_chan_spec mcp3202_channels[] = {
>  	MCP320X_VOLTAGE_CHANNEL(0),
>  	MCP320X_VOLTAGE_CHANNEL(1),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1, 2),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0, 2),
>  };
>  
>  static const struct iio_chan_spec mcp3204_channels[] = {
> @@ -269,10 +287,10 @@ static const struct iio_chan_spec mcp3204_channels[] = {
>  	MCP320X_VOLTAGE_CHANNEL(1),
>  	MCP320X_VOLTAGE_CHANNEL(2),
>  	MCP320X_VOLTAGE_CHANNEL(3),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(2, 3),
> -	MCP320X_VOLTAGE_CHANNEL_DIFF(3, 2),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(0, 1, 4),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(1, 0, 4),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(2, 3, 4),
> +	MCP320X_VOLTAGE_CHANNEL_DIFF(3, 2, 4),
>  };