diff mbox series

[v5,5/7] iio: adc: ad7380: prepare for parts with more channels

Message ID 20240319-adding-new-ad738x-driver-v5-5-ce7df004ceb3@baylibre.com (mailing list archive)
State Changes Requested
Headers show
Series iio: adc: add new ad7380 driver | expand

Commit Message

Julien Stephan March 19, 2024, 10:11 a.m. UTC
The current driver supports only parts with 2 channels.
In order to prepare the support of new compatible ADCs with more
channels, this commit:
  - defines MAX_NUM_CHANNEL to specify the maximum number of
    channels currently supported by the driver
  - adds available_scan_mask member in ad7380_chip_info structure
  - fixes spi xfer struct len depending on number of channels
  - fixes scan_data.raw buffer size to handle more channels
  - adds a timing specifications structure in ad7380_chip_info structure

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

Comments

Jonathan Cameron March 24, 2024, 1:07 p.m. UTC | #1
On Tue, 19 Mar 2024 11:11:26 +0100
Julien Stephan <jstephan@baylibre.com> wrote:

> The current driver supports only parts with 2 channels.
> In order to prepare the support of new compatible ADCs with more
> channels, this commit:
>   - defines MAX_NUM_CHANNEL to specify the maximum number of
>     channels currently supported by the driver
>   - adds available_scan_mask member in ad7380_chip_info structure
>   - fixes spi xfer struct len depending on number of channels
>   - fixes scan_data.raw buffer size to handle more channels
>   - adds a timing specifications structure in ad7380_chip_info structure
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>

>  struct ad7380_state {
> @@ -148,15 +168,15 @@ struct ad7380_state {
>  	struct spi_device *spi;
>  	struct regmap *regmap;
>  	unsigned int vref_mv;
> -	unsigned int vcm_mv[2];
> +	unsigned int vcm_mv[MAX_NUM_CHANNELS];
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> -	 * Make the buffer large enough for 2 16-bit samples and one 64-bit
> +	 * Make the buffer large enough for MAX_NUM_CHANNELS 16-bit samples and one 64-bit
>  	 * aligned 64 bit timestamp.
>  	 */
>  	struct {
> -		u16 raw[2];
> +		u16 raw[MAX_NUM_CHANNELS];

This can get problematic for drivers supporting devices with varying numbers of channels.
You are fine up to 4 channels as the structure layout isn't changing.  The reason is that
it implies the timestamp location, which if you were for instance to support 8 channels
would be incorrect if only 4 of them were enabled.
Now the timestamp location is only used implicitly in the driver (as the IIO core
handles the actual insertion of the data) so it's not a bug as such to do this, but it
can give people the wrong impression during testing etc.

As such, I'd like to see the comment mention that "as MAX_NUM_CHANNELS is 4
the layout of the structure is the same for all parts".

Note I've reviewed one driver today that did have different layouts depending on
which device was in use and ended up falsely indicating the timestamp was later
than it actually was in the buffer.  Hence my request for a defensive comment!

>  
>  		s64 ts __aligned(8);
>  	} scan_data __aligned(IIO_DMA_MINALIGN);
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 996ca83feaed..3aca41ce9a14 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -29,6 +29,7 @@ 
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
+#define MAX_NUM_CHANNELS		2
 /* 2.5V internal reference voltage */
 #define AD7380_INTERNAL_REF_MV		2500
 
@@ -65,12 +66,19 @@ 
 #define AD7380_ALERT_LOW_TH		GENMASK(11, 0)
 #define AD7380_ALERT_HIGH_TH		GENMASK(11, 0)
 
+#define T_CONVERT_NS 190		/* conversion time */
+struct ad7380_timing_specs {
+	const unsigned int t_csh_ns;	/* CS minimum high time */
+};
+
 struct ad7380_chip_info {
 	const char *name;
 	const struct iio_chan_spec *channels;
 	unsigned int num_channels;
 	const char * const *vcm_supplies;
 	unsigned int num_vcm_supplies;
+	const unsigned long *available_scan_masks;
+	const struct ad7380_timing_specs *timing_specs;
 };
 
 #define AD7380_CHANNEL(index, bits, diff) {			\
@@ -115,16 +123,24 @@  static const unsigned long ad7380_2_channel_scan_masks[] = {
 	0
 };
 
+static const struct ad7380_timing_specs ad7380_timing = {
+	.t_csh_ns = 10,
+};
+
 static const struct ad7380_chip_info ad7380_chip_info = {
 	.name = "ad7380",
 	.channels = ad7380_channels,
 	.num_channels = ARRAY_SIZE(ad7380_channels),
+	.available_scan_masks = ad7380_2_channel_scan_masks,
+	.timing_specs = &ad7380_timing,
 };
 
 static const struct ad7380_chip_info ad7381_chip_info = {
 	.name = "ad7381",
 	.channels = ad7381_channels,
 	.num_channels = ARRAY_SIZE(ad7381_channels),
+	.available_scan_masks = ad7380_2_channel_scan_masks,
+	.timing_specs = &ad7380_timing,
 };
 
 static const struct ad7380_chip_info ad7383_chip_info = {
@@ -133,6 +149,8 @@  static const struct ad7380_chip_info ad7383_chip_info = {
 	.num_channels = ARRAY_SIZE(ad7383_channels),
 	.vcm_supplies = ad7380_2_channel_vcm_supplies,
 	.num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+	.available_scan_masks = ad7380_2_channel_scan_masks,
+	.timing_specs = &ad7380_timing,
 };
 
 static const struct ad7380_chip_info ad7384_chip_info = {
@@ -141,6 +159,8 @@  static const struct ad7380_chip_info ad7384_chip_info = {
 	.num_channels = ARRAY_SIZE(ad7384_channels),
 	.vcm_supplies = ad7380_2_channel_vcm_supplies,
 	.num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+	.available_scan_masks = ad7380_2_channel_scan_masks,
+	.timing_specs = &ad7380_timing,
 };
 
 struct ad7380_state {
@@ -148,15 +168,15 @@  struct ad7380_state {
 	struct spi_device *spi;
 	struct regmap *regmap;
 	unsigned int vref_mv;
-	unsigned int vcm_mv[2];
+	unsigned int vcm_mv[MAX_NUM_CHANNELS];
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
-	 * Make the buffer large enough for 2 16-bit samples and one 64-bit
+	 * Make the buffer large enough for MAX_NUM_CHANNELS 16-bit samples and one 64-bit
 	 * aligned 64 bit timestamp.
 	 */
 	struct {
-		u16 raw[2];
+		u16 raw[MAX_NUM_CHANNELS];
 
 		s64 ts __aligned(8);
 	} scan_data __aligned(IIO_DMA_MINALIGN);
@@ -194,7 +214,7 @@  static int ad7380_regmap_reg_read(void *context, unsigned int reg,
 			.tx_buf = &st->tx,
 			.cs_change = 1,
 			.cs_change_delay = {
-				.value = 10, /* t[CSH] */
+				.value = st->chip_info->timing_specs->t_csh_ns,
 				.unit = SPI_DELAY_UNIT_NSECS,
 			},
 		}, {
@@ -255,7 +275,8 @@  static irqreturn_t ad7380_trigger_handler(int irq, void *p)
 	struct ad7380_state *st = iio_priv(indio_dev);
 	struct spi_transfer xfer = {
 		.bits_per_word = st->chip_info->channels[0].scan_type.realbits,
-		.len = 4,
+		.len = (st->chip_info->num_channels - 1) *
+		       ((st->chip_info->channels->scan_type.storagebits > 16) ? 4 : 2),
 		.rx_buf = st->scan_data.raw,
 	};
 	int ret;
@@ -282,21 +303,22 @@  static int ad7380_read_direct(struct ad7380_state *st,
 			.speed_hz = AD7380_REG_WR_SPEED_HZ,
 			.bits_per_word = chan->scan_type.realbits,
 			.delay = {
-				.value = 190, /* t[CONVERT] */
+				.value = T_CONVERT_NS,
 				.unit = SPI_DELAY_UNIT_NSECS,
 			},
 			.cs_change = 1,
 			.cs_change_delay = {
-				.value = 10, /* t[CSH] */
+				.value = st->chip_info->timing_specs->t_csh_ns,
 				.unit = SPI_DELAY_UNIT_NSECS,
 			},
 		},
-		/* then read both channels */
+		/* then read all channels */
 		{
 			.speed_hz = AD7380_REG_WR_SPEED_HZ,
 			.bits_per_word = chan->scan_type.realbits,
 			.rx_buf = st->scan_data.raw,
-			.len = 4,
+			.len = (st->chip_info->num_channels - 1) *
+			       ((chan->scan_type.storagebits > 16) ? 4 : 2),
 		},
 	};
 	int ret;
@@ -472,7 +494,7 @@  static int ad7380_probe(struct spi_device *spi)
 	indio_dev->name = st->chip_info->name;
 	indio_dev->info = &ad7380_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->available_scan_masks = ad7380_2_channel_scan_masks;
+	indio_dev->available_scan_masks = st->chip_info->available_scan_masks;
 
 	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
 					      iio_pollfunc_store_time,