linux-kernel - Re: [PATCH 2/2] iio: adc: mcp320x: add triggered buffer support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 28 Aug 2022 18:24:30 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Vincent Whitchurch <vincent.whitchurch@...s.com>
Cc:     <kernel@...s.com>, <lars@...afoo.de>, <axel.jonsson@...s.com>,
        <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] iio: adc: mcp320x: add triggered buffer support

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

> From: Axel Jonsson <axel.jonsson@...s.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@...s.com>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@...s.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),
>  };