Support ROHM BD79105 ADC

The ad7476 supports some ICs with an internal reference voltage. For those ICs the reference voltage has been hard-coded as micro volts, but the value which is later used in code needs to be milli volts. This results the need to divide hard coded voltage by 1000 before using it.

Simplify code by changing the hard-coded voltage to millivolts and by dropping the division.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => :
- No changes
---

 drivers/iio/adc/ad7476.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/iio/adc/ad7476.c +6 -6

index f117aafd8fad..7b6d36999afc 100644
@@ -27,7 +27,7 @@
 struct ad7476_state;
 
 struct ad7476_chip_info {
-	unsigned int			int_vref_uv;
+	unsigned int			int_vref_mv;
 	struct iio_chan_spec		channel[2];
 	/* channels used when convst gpio is defined */
 	struct iio_chan_spec		convst_channel[2];

@@ -172,7 +172,7 @@ static const struct ad7476_chip_info ad7091r_chip_info = {
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
 	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
 	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.int_vref_uv = 2500000,
+	.int_vref_mv = 2500,
 	.has_vref = true,
 	.reset = ad7091_reset,
 };

@@ -229,7 +229,7 @@ static const struct ad7476_chip_info ad7475_chip_info = {
 static const struct ad7476_chip_info ad7495_chip_info = {
 	.channel[0] = AD7476_CHAN(12),
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.int_vref_uv = 2500000,
+	.int_vref_mv = 2500,
 	.has_vdrive = true,
 };
 

@@ -295,7 +295,7 @@ static int ad7476_probe(struct spi_device *spi)
 		return -ENODEV;
 
 	/* Use VCC for reference voltage if vref / internal vref aren't used */
-	if (!st->chip_info->int_vref_uv && !st->chip_info->has_vref) {
+	if (!st->chip_info->int_vref_mv && !st->chip_info->has_vref) {
 		ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
 		if (ret < 0)
 			return ret;

@@ -310,7 +310,7 @@ static int ad7476_probe(struct spi_device *spi)
 		ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
 		if (ret < 0) {
 			/* Vref is optional if a device has an internal reference */
-			if (!st->chip_info->int_vref_uv || ret != -ENODEV)
+			if (!st->chip_info->int_vref_mv || ret != -ENODEV)
 				return ret;
 		} else {
 			st->scale_mv = ret / 1000;

@@ -318,7 +318,7 @@ static int ad7476_probe(struct spi_device *spi)
 	}
 
 	if (!st->scale_mv)
-		st->scale_mv = st->chip_info->int_vref_uv / 1000;
+		st->scale_mv = st->chip_info->int_vref_mv;
 
 	if (st->chip_info->has_vdrive) {
 		ret = devm_regulator_get_enable(&spi->dev, "vdrive");

Signature · 1 line

2.50.1

The ad7476 supports ADCs which use separate GPIO for starting the conversion. For such devices, the driver uses different channel information if the GPIO is found. The bit information is still always used from the original (non 'convstart') channels.

This has not been causing problems because the bit information for the 'convstart' -channel and the 'normal' -channel is identical. It, however, will cause issues if an IC has different characteristics for an 'convstart' -channel and regular channel. Furthermore, this will cause problems if a device always requires the convstart GPIO and thus only defines the convstart channel.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => :
- No changes
---

 drivers/iio/adc/ad7476.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/iio/adc/ad7476.c +3 -3

index 7b6d36999afc..fc701267358e 100644
@@ -121,8 +121,8 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
 
 		if (ret < 0)
 			return ret;
-		*val = (ret >> st->chip_info->channel[0].scan_type.shift) &
-			GENMASK(st->chip_info->channel[0].scan_type.realbits - 1, 0);
+		*val = (ret >> chan->scan_type.shift) &
+			GENMASK(chan->scan_type.realbits - 1, 0);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		*val = st->scale_mv;

@@ -345,7 +345,7 @@ static int ad7476_probe(struct spi_device *spi)
 	/* Setup default message */
 
 	st->xfer.rx_buf = &st->data;
-	st->xfer.len = st->chip_info->channel[0].scan_type.storagebits / 8;
+	st->xfer.len = indio_dev->channels[0].scan_type.storagebits / 8;
 
 	spi_message_init(&st->msg);
 	spi_message_add_tail(&st->xfer, &st->msg);

Signature · 1 line

2.50.1

The chip_info structure is not required to be accessed after probe.

Remove the chip_info pointer from the driver data to reduce the scope and to make driver clearer.

Quoted 5 lines · Matti Vaittinen

The chip_info structure is not required to be accessed after probe.

Remove the chip_info pointer from the driver data to reduce the scope and to make driver clearer.

the driver

clearer or cleaner? I think you want the latter...

...

Not sure how the future of the development of this driver will look like, but it might be this patch will be reverted if one wants something else from chip_info to have a longer lifetime.

The ad7476 driver defines separate chan_spec structures for operation with and without convstart GPIO. At quick glance this may seem as if the driver did provide more than 1 data-channel to users - one for the regular data, other for the data obtained with the convstart GPIO.

The only difference between the 'convstart' and 'non convstart' -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in channel's flags.

We can drop the convstart channel spec, and related convstart macro, by allocating a mutable per driver instance channel spec an adding the flag in probe if needed. This will simplify the driver with the cost of added memory consumption.

Assuming there aren't systems with very many ADCs and very few resources, this tradeoff seems worth making.

Simplify the driver by dropping the 'convstart' channel spec and allocating the chan spec for each driver instance.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- New patch

I considered squashing this change with the one limiting the chip_info scope. Having this as a separate change should help reverting if someone complains about the increased memory consumption though.

---
 drivers/iio/adc/ad7476.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

drivers/iio/adc/ad7476.c +18 -13

index e97742912b8e..a30eb016c11c 100644
@@ -29,8 +29,6 @@ struct ad7476_state;
 struct ad7476_chip_info {
 	unsigned int			int_vref_mv;
 	struct iio_chan_spec		channel[2];
-	/* channels used when convst gpio is defined */
-	struct iio_chan_spec		convst_channel[2];
 	void (*reset)(struct ad7476_state *);
 	bool				has_vref;
 	bool				has_vdrive;

@@ -41,6 +39,7 @@ struct ad7476_state {
 	struct gpio_desc		*convst_gpio;
 	struct spi_transfer		xfer;
 	struct spi_message		msg;
+	struct iio_chan_spec		channel[2];
 	int				scale_mv;
 	/*
 	 * DMA (thus cache coherency maintenance) may require the

@@ -153,24 +152,18 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
 #define AD7940_CHAN(bits) _AD7476_CHAN((bits), 15 - (bits), \
 		BIT(IIO_CHAN_INFO_RAW))
 #define AD7091R_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), 0)
-#define AD7091R_CONVST_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), \
-		BIT(IIO_CHAN_INFO_RAW))
 #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
 		BIT(IIO_CHAN_INFO_RAW))
 
 static const struct ad7476_chip_info ad7091_chip_info = {
 	.channel[0] = AD7091R_CHAN(12),
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
 	.reset = ad7091_reset,
 };
 
 static const struct ad7476_chip_info ad7091r_chip_info = {
 	.channel[0] = AD7091R_CHAN(12),
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
 	.int_vref_mv = 2500,
 	.has_vref = true,
 	.reset = ad7091_reset,

@@ -282,7 +275,7 @@ static int ad7476_probe(struct spi_device *spi)
 	const struct ad7476_chip_info *chip_info;
 	struct ad7476_state *st;
 	struct iio_dev *indio_dev;
-	int ret;
+	int ret, i;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)

@@ -332,16 +325,28 @@ static int ad7476_probe(struct spi_device *spi)
 	if (IS_ERR(st->convst_gpio))
 		return PTR_ERR(st->convst_gpio);
 
+	/*
+	 * This will never realize. Unless someone changes the channel specs
+	 * in this driver. And if someone does, without changing the loop
+	 * below, then we'd better immediately produce a big fat error, before
+	 * the change proceeds from that developer's table.
+	 */
+	BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));
+	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
+		st->channel[i] = chip_info->channel[i];
+		if (st->convst_gpio)
+			st->channel[i].info_mask_separate |=
+				BIT(IIO_CHAN_INFO_RAW);
+	}
+
 	st->spi = spi;
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = chip_info->channel;
-	indio_dev->num_channels = 2;
+	indio_dev->channels = st->channel;
+	indio_dev->num_channels = ARRAY_SIZE(st->channel);
 	indio_dev->info = &ad7476_info;
 
-	if (st->convst_gpio)
-		indio_dev->channels = chip_info->convst_channel;
 	/* Setup default message */
 
 	st->xfer.rx_buf = &st->data;

Signature · 1 line

2.50.1

Quoted 14 lines · Matti Vaittinen

The ad7476 driver defines separate chan_spec structures for operation with and without convstart GPIO. At quick glance this may seem as if the driver did provide more than 1 data-channel to users - one for the regular data, other for the data obtained with the convstart GPIO.

The only difference between the 'convstart' and 'non convstart' -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in channel's flags.

We can drop the convstart channel spec, and related convstart macro, by allocating a mutable per driver instance channel spec an adding the flag in probe if needed. This will simplify the driver with the cost of added memory consumption.

Quoted 5 lines

Assuming there aren't systems with very many ADCs and very few resources, this tradeoff seems worth making.

Simplify the driver by dropping the 'convstart' channel spec and allocating the chan spec for each driver instance.

I do not agree with this one. Looking at the diff, code does not look simpler to me...

- Nuno Sá

Quoted 74 lines

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- New patch

I considered squashing this change with the one limiting the chip_info scope. Having this as a separate change should help reverting if someone complains about the increased memory consumption though.

---
 drivers/iio/adc/ad7476.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

drivers/iio/adc/ad7476.c +2 -9

index e97742912b8e..a30eb016c11c 100644
@@ -29,8 +29,6 @@ struct ad7476_state;
 struct ad7476_chip_info {
 	unsigned int			int_vref_mv;
 	struct iio_chan_spec		channel[2];
-	/* channels used when convst gpio is defined */
-	struct iio_chan_spec		convst_channel[2];
 	void (*reset)(struct ad7476_state *);
 	bool				has_vref;
 	bool				has_vdrive;

@@ -41,6 +39,7 @@ struct ad7476_state {
 	struct gpio_desc		*convst_gpio;
 	struct spi_transfer		xfer;
 	struct spi_message		msg;
+	struct iio_chan_spec		channel[2];
 	int				scale_mv;
 	/*
 	 * DMA (thus cache coherency maintenance) may require the

@@ -153,24 +152,18 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
 #define AD7940_CHAN(bits) _AD7476_CHAN((bits), 15 - (bits), \
 		BIT(IIO_CHAN_INFO_RAW))
 #define AD7091R_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), 0)
-#define AD7091R_CONVST_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), \
-		BIT(IIO_CHAN_INFO_RAW))
 #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
 		BIT(IIO_CHAN_INFO_RAW))
 
 static const struct ad7476_chip_info ad7091_chip_info = {
 	.channel[0] = AD7091R_CHAN(12),
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
 	.reset = ad7091_reset,
 };
 
 static const struct ad7476_chip_info ad7091r_chip_info = {
 	.channel[0] = AD7091R_CHAN(12),
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
 	.int_vref_mv = 2500,
 	.has_vref = true,
 	.reset = ad7091_reset,

@@ -282,7 +275,7 @@ static int ad7476_probe(struct spi_device *spi)
 	const struct ad7476_chip_info *chip_info;
 	struct ad7476_state *st;
 	struct iio_dev *indio_dev;
-	int ret;
+	int ret, i;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)

@@ -332,16 +325,28 @@ static int ad7476_probe(struct spi_device *spi)
 	if (IS_ERR(st->convst_gpio))
 		return PTR_ERR(st->convst_gpio);
 

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 334

Quoted 7 lines

drivers/iio/adc/ad7476.c +7 -0

+	/*
+	 * This will never realize. Unless someone changes the channel specs
+	 * in this driver. And if someone does, without changing the loop
+	 * below, then we'd better immediately produce a big fat error, before
+	 * the change proceeds from that developer's table.
+	 */
+	BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 337

Quoted 3 lines

drivers/iio/adc/ad7476.c +3 -0

+	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
+		st->channel[i] = chip_info->channel[i];
+		if (st->convst_gpio)

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 339

Quoted 2 lines

drivers/iio/adc/ad7476.c +2 -0

+			st->channel[i].info_mask_separate |=
+				BIT(IIO_CHAN_INFO_RAW);

Quoted 20 lines

drivers/iio/adc/ad7476.c +4 -4

+	}
+
 	st->spi = spi;
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = chip_info->channel;
-	indio_dev->num_channels = 2;
+	indio_dev->channels = st->channel;
+	indio_dev->num_channels = ARRAY_SIZE(st->channel);
 	indio_dev->info = &ad7476_info;
 
-	if (st->convst_gpio)
-		indio_dev->channels = chip_info->convst_channel;
 	/* Setup default message */
 
 	st->xfer.rx_buf = &st->data;

Signature · 1 line

2.50.1

Quoted 21 lines · Nuno Sá

Quoted 19 lines · Matti Vaittinen

The ad7476 driver defines separate chan_spec structures for operation with and without convstart GPIO. At quick glance this may seem as if the driver did provide more than 1 data-channel to users - one for the regular data, other for the data obtained with the convstart GPIO.

The only difference between the 'convstart' and 'non convstart' -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in channel's flags.

We can drop the convstart channel spec, and related convstart macro, by allocating a mutable per driver instance channel spec an adding the flag in probe if needed. This will simplify the driver with the cost of added memory consumption.

Assuming there aren't systems with very many ADCs and very few resources, this tradeoff seems worth making.

Simplify the driver by dropping the 'convstart' channel spec and allocating the chan spec for each driver instance.

Quoted 2 lines

I do not agree with this one. Looking at the diff, code does not look simpler to me...

Ok, on a second thought I'm ok with this. It makes adding devices easier and (IIUC) for the one you're adding later we only have "convst_channel" channels.

On comment though...

Quoted 77 lines

- Nuno Sá

Quoted 74 lines

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- New patch

I considered squashing this change with the one limiting the chip_info scope. Having this as a separate change should help reverting if someone complains about the increased memory consumption though.

---
  drivers/iio/adc/ad7476.c | 31 ++++++++++++++++++-------------
  1 file changed, 18 insertions(+), 13 deletions(-)

drivers/iio/adc/ad7476.c +2 -9

index e97742912b8e..a30eb016c11c 100644
@@ -29,8 +29,6 @@ struct ad7476_state;
  struct ad7476_chip_info {
  	unsigned int			int_vref_mv;
  	struct iio_chan_spec		channel[2];
-	/* channels used when convst gpio is defined */
-	struct iio_chan_spec		convst_channel[2];
  	void (*reset)(struct ad7476_state *);
  	bool				has_vref;
  	bool				has_vdrive;

@@ -41,6 +39,7 @@ struct ad7476_state {
  	struct gpio_desc		*convst_gpio;
  	struct spi_transfer		xfer;
  	struct spi_message		msg;
+	struct iio_chan_spec		channel[2];
  	int				scale_mv;
  	/*
  	 * DMA (thus cache coherency maintenance) may require the

@@ -153,24 +152,18 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
  #define AD7940_CHAN(bits) _AD7476_CHAN((bits), 15 - (bits), \
  		BIT(IIO_CHAN_INFO_RAW))
  #define AD7091R_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), 0)
-#define AD7091R_CONVST_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), \
-		BIT(IIO_CHAN_INFO_RAW))
  #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
  		BIT(IIO_CHAN_INFO_RAW))
 
  static const struct ad7476_chip_info ad7091_chip_info = {
  	.channel[0] = AD7091R_CHAN(12),
  	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
  	.reset = ad7091_reset,
  };
 
  static const struct ad7476_chip_info ad7091r_chip_info = {
  	.channel[0] = AD7091R_CHAN(12),
  	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
  	.int_vref_mv = 2500,
  	.has_vref = true,
  	.reset = ad7091_reset,

@@ -282,7 +275,7 @@ static int ad7476_probe(struct spi_device *spi)
  	const struct ad7476_chip_info *chip_info;
  	struct ad7476_state *st;
  	struct iio_dev *indio_dev;
-	int ret;
+	int ret, i;
 
  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
  	if (!indio_dev)

@@ -332,16 +325,28 @@ static int ad7476_probe(struct spi_device *spi)
  	if (IS_ERR(st->convst_gpio))
  		return PTR_ERR(st->convst_gpio);
 

Quoted 7 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 334

Quoted 7 lines

drivers/iio/adc/ad7476.c +7 -0

+	/*
+	 * This will never realize. Unless someone changes the channel specs
+	 * in this driver. And if someone does, without changing the loop
+	 * below, then we'd better immediately produce a big fat error, before
+	 * the change proceeds from that developer's table.
+	 */
+	BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

I guess it make sense but still looks too fancy for this :)

Quoted 4 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 337

Quoted 4 lines

drivers/iio/adc/ad7476.c +3 -0

+	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
+		st->channel[i] = chip_info->channel[i];
+		if (st->convst_gpio)

I would flip this an do:

	if (!st->convst_gpio)
		break;

Quoted 3 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 339

Quoted 3 lines

drivers/iio/adc/ad7476.c +2 -0

+			st->channel[i].info_mask_separate |=
+				BIT(IIO_CHAN_INFO_RAW);

		__set_bit()...

Quoted 22 lines · Nuno Sá

Quoted 21 lines · Nuno Sá

Quoted 19 lines · Matti Vaittinen

The ad7476 driver defines separate chan_spec structures for operation with and without convstart GPIO. At quick glance this may seem as if the driver did provide more than 1 data-channel to users - one for the regular data, other for the data obtained with the convstart GPIO.

The only difference between the 'convstart' and 'non convstart' -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in channel's flags.

We can drop the convstart channel spec, and related convstart macro, by allocating a mutable per driver instance channel spec an adding the flag in probe if needed. This will simplify the driver with the cost of added memory consumption.

Assuming there aren't systems with very many ADCs and very few resources, this tradeoff seems worth making.

Simplify the driver by dropping the 'convstart' channel spec and allocating the chan spec for each driver instance.

Quoted 6 lines

Quoted 2 lines

I do not agree with this one. Looking at the diff, code does not look simpler to me...

Ok, on a second thought I'm ok with this. It makes adding devices easier and (IIUC) for the one you're adding later we only have "convst_channel" channels.

Yes, that's right. The BD79105 requires the convstart.

Quoted 76 lines

On comment though...

Quoted 74 lines

- Nuno Sá

Quoted 71 lines

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- New patch

I considered squashing this change with the one limiting the chip_info scope. Having this as a separate change should help reverting if someone complains about the increased memory consumption though.

---
  drivers/iio/adc/ad7476.c | 31 ++++++++++++++++++-------------
  1 file changed, 18 insertions(+), 13 deletions(-)

drivers/iio/adc/ad7476.c +3 -9

index e97742912b8e..a30eb016c11c 100644
@@ -29,8 +29,6 @@ struct ad7476_state;
  struct ad7476_chip_info {
  	unsigned int			int_vref_mv;
  	struct iio_chan_spec		channel[2];
-	/* channels used when convst gpio is defined */
-	struct iio_chan_spec		convst_channel[2];
  	void (*reset)(struct ad7476_state *);
  	bool				has_vref;
  	bool				has_vdrive;

@@ -41,6 +39,7 @@ struct ad7476_state {
  	struct gpio_desc		*convst_gpio;
  	struct spi_transfer		xfer;
  	struct spi_message		msg;
+	struct iio_chan_spec		channel[2];
  	int				scale_mv;
  	/*
  	 * DMA (thus cache coherency maintenance) may require the

@@ -153,24 +152,18 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
  #define AD7940_CHAN(bits) _AD7476_CHAN((bits), 15 - (bits), \
  		BIT(IIO_CHAN_INFO_RAW))
  #define AD7091R_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), 0)
-#define AD7091R_CONVST_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), \
-		BIT(IIO_CHAN_INFO_RAW))
  #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
  		BIT(IIO_CHAN_INFO_RAW))
  static const struct ad7476_chip_info ad7091_chip_info = {
  	.channel[0] = AD7091R_CHAN(12),
  	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
  	.reset = ad7091_reset,
  };
  static const struct ad7476_chip_info ad7091r_chip_info = {
  	.channel[0] = AD7091R_CHAN(12),
  	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
  	.int_vref_mv = 2500,
  	.has_vref = true,
  	.reset = ad7091_reset,

@@ -282,7 +275,7 @@ static int ad7476_probe(struct spi_device *spi)
  	const struct ad7476_chip_info *chip_info;
  	struct ad7476_state *st;
  	struct iio_dev *indio_dev;
-	int ret;
+	int ret, i;
  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
  	if (!indio_dev)

@@ -332,16 +325,28 @@ static int ad7476_probe(struct spi_device *spi)
  	if (IS_ERR(st->convst_gpio))
  		return PTR_ERR(st->convst_gpio);
+	/*

Quoted 8 lines

Quoted 6 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 334

Quoted 6 lines

drivers/iio/adc/ad7476.c +6 -0

+	 * This will never realize. Unless someone changes the channel specs
+	 * in this driver. And if someone does, without changing the loop
+	 * below, then we'd better immediately produce a big fat error, before
+	 * the change proceeds from that developer's table.
+	 */
+	BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

I guess it make sense but still looks too fancy for this :)

Nothing else but a developer's carefulness keeps the number of channels "in sync" for these two structs. I was originally doing WARN_ON() - but then I thought that it's be even better to catch this at build time. Then I found the BUILD_BUG_ON(). I see Andy suggested static_assert() instead - I've no idea why one is preferred over other though. Let's see if I get educated by Andy :)

Quoted 8 lines

Quoted 4 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 337

Quoted 4 lines

drivers/iio/adc/ad7476.c +3 -0

+	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
+		st->channel[i] = chip_info->channel[i];
+		if (st->convst_gpio)

I would flip this an do:

	if (!st->convst_gpio)
		break;

To me this would just add an extra line of code, and more complex flow. I would definitely agree if there were more operations to be done for the 'convstart channels' - but since this is really just "if it's convstart, then set a bit" - the

if (foo)

	bar;

seems simpler than

if (!foo)

	break;

bar;

Quoted 6 lines

Quoted 4 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 339

Quoted 4 lines

drivers/iio/adc/ad7476.c +2 -0

+			st->channel[i].info_mask_separate |=
+				BIT(IIO_CHAN_INFO_RAW);

		__set_bit()...

Ok. Thanks.

Thanks for the review(s) Nuno!

Yours,
-- Matti

Quoted 138 lines · Matti Vaittinen

Quoted 22 lines · Nuno Sá

Quoted 21 lines · Nuno Sá

Quoted 19 lines · Matti Vaittinen

The ad7476 driver defines separate chan_spec structures for operation with and without convstart GPIO. At quick glance this may seem as if the driver did provide more than 1 data-channel to users - one for the regular data, other for the data obtained with the convstart GPIO.

The only difference between the 'convstart' and 'non convstart' -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in channel's flags.

We can drop the convstart channel spec, and related convstart macro, by allocating a mutable per driver instance channel spec an adding the flag in probe if needed. This will simplify the driver with the cost of added memory consumption.

Assuming there aren't systems with very many ADCs and very few resources, this tradeoff seems worth making.

Simplify the driver by dropping the 'convstart' channel spec and allocating the chan spec for each driver instance.

Quoted 6 lines

Quoted 2 lines

I do not agree with this one. Looking at the diff, code does not look simpler to me...

Ok, on a second thought I'm ok with this. It makes adding devices easier and (IIUC) for the one you're adding later we only have "convst_channel" channels.

Yes, that's right. The BD79105 requires the convstart.

Quoted 76 lines

On comment though...

Quoted 74 lines

- Nuno Sá

Quoted 71 lines

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- New patch

I considered squashing this change with the one limiting the chip_info scope. Having this as a separate change should help reverting if someone complains about the increased memory consumption though.

---
   drivers/iio/adc/ad7476.c | 31 ++++++++++++++++++-------------
   1 file changed, 18 insertions(+), 13 deletions(-)

drivers/iio/adc/ad7476.c +3 -9

index e97742912b8e..a30eb016c11c 100644
@@ -29,8 +29,6 @@ struct ad7476_state;
   struct ad7476_chip_info {
   	unsigned int			int_vref_mv;
   	struct iio_chan_spec		channel[2];
-	/* channels used when convst gpio is defined */
-	struct iio_chan_spec		convst_channel[2];
   	void (*reset)(struct ad7476_state *);
   	bool				has_vref;
   	bool				has_vdrive;

@@ -41,6 +39,7 @@ struct ad7476_state {
   	struct gpio_desc		*convst_gpio;
   	struct spi_transfer		xfer;
   	struct spi_message		msg;
+	struct iio_chan_spec		channel[2];
   	int				scale_mv;
   	/*
   	 * DMA (thus cache coherency maintenance) may require the

@@ -153,24 +152,18 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
   #define AD7940_CHAN(bits) _AD7476_CHAN((bits), 15 - (bits), \
   		BIT(IIO_CHAN_INFO_RAW))
   #define AD7091R_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), 0)
-#define AD7091R_CONVST_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), \
-		BIT(IIO_CHAN_INFO_RAW))
   #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
   		BIT(IIO_CHAN_INFO_RAW))
   static const struct ad7476_chip_info ad7091_chip_info = {
   	.channel[0] = AD7091R_CHAN(12),
   	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
   	.reset = ad7091_reset,
   };
   static const struct ad7476_chip_info ad7091r_chip_info = {
   	.channel[0] = AD7091R_CHAN(12),
   	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
   	.int_vref_mv = 2500,
   	.has_vref = true,
   	.reset = ad7091_reset,

@@ -282,7 +275,7 @@ static int ad7476_probe(struct spi_device *spi)
   	const struct ad7476_chip_info *chip_info;
   	struct ad7476_state *st;
   	struct iio_dev *indio_dev;
-	int ret;
+	int ret, i;
   	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
   	if (!indio_dev)

@@ -332,16 +325,28 @@ static int ad7476_probe(struct spi_device *spi)
   	if (IS_ERR(st->convst_gpio))
   		return PTR_ERR(st->convst_gpio);
+	/*

Quoted 8 lines

Quoted 6 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 334

Quoted 6 lines

drivers/iio/adc/ad7476.c +6 -0

+	 * This will never realize. Unless someone changes the channel specs
+	 * in this driver. And if someone does, without changing the loop
+	 * below, then we'd better immediately produce a big fat error, before
+	 * the change proceeds from that developer's table.
+	 */
+	BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

I guess it make sense but still looks too fancy for this :)

Nothing else but a developer's carefulness keeps the number of channels "in sync" for these two structs. I was originally doing WARN_ON() - but then I thought that it's be even better to catch this at build time. Then I found the BUILD_BUG_ON(). I see Andy suggested static_assert() instead - I've no idea why one is preferred over other though. Let's see if I get educated by Andy :)

Quoted 8 lines

Quoted 4 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 337

Quoted 4 lines

drivers/iio/adc/ad7476.c +3 -0

+	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
+		st->channel[i] = chip_info->channel[i];
+		if (st->convst_gpio)

I would flip this an do:

	if (!st->convst_gpio)
		break;

To me this would just add an extra line of code, and more complex flow. I would definitely agree if there were more operations to be done for the 'convstart channels' - but since this is really just "if it's convstart, then set a bit" - the

Quoted 8 lines

if (foo)

	bar;

seems simpler than

if (!foo)

	break;

bar;

Yes but in this particular case, you likely would not need to do any line break afterward because of indentation. Logically it also makes sense because st->convst_gpio is a device property (not a channel one). So it makes no sense to check it for all channels (I know we only have two channels). So if you prefer, you could even do:

if (st->convst_gpio) {
	for (...)
		__set_bit(...);
}

which also would make more sense to me.

Up to you now :)

Quoted 104 lines · Nuno Sá

Quoted 103 lines · Matti Vaittinen

Quoted 22 lines · Nuno Sá

Quoted 21 lines · Nuno Sá

Quoted 19 lines · Matti Vaittinen

The ad7476 driver defines separate chan_spec structures for operation with and without convstart GPIO. At quick glance this may seem as if the driver did provide more than 1 data-channel to users - one for the regular data, other for the data obtained with the convstart GPIO.

The only difference between the 'convstart' and 'non convstart' -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in channel's flags.

We can drop the convstart channel spec, and related convstart macro, by allocating a mutable per driver instance channel spec an adding the flag in probe if needed. This will simplify the driver with the cost of added memory consumption.

Assuming there aren't systems with very many ADCs and very few resources, this tradeoff seems worth making.

Simplify the driver by dropping the 'convstart' channel spec and allocating the chan spec for each driver instance.

Quoted 6 lines

Quoted 2 lines

I do not agree with this one. Looking at the diff, code does not look simpler to me...

Ok, on a second thought I'm ok with this. It makes adding devices easier and (IIUC) for the one you're adding later we only have "convst_channel" channels.

Yes, that's right. The BD79105 requires the convstart.

Quoted 71 lines

On comment though...

Quoted 69 lines

- Nuno Sá

Quoted 66 lines

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- New patch

I considered squashing this change with the one limiting the chip_info scope. Having this as a separate change should help reverting if someone complains about the increased memory consumption though.

---
   drivers/iio/adc/ad7476.c | 31 ++++++++++++++++++-------------
   1 file changed, 18 insertions(+), 13 deletions(-)

drivers/iio/adc/ad7476.c +2 -9

index e97742912b8e..a30eb016c11c 100644
@@ -29,8 +29,6 @@ struct ad7476_state;
   struct ad7476_chip_info {
   	unsigned int			int_vref_mv;
   	struct iio_chan_spec		channel[2];
-	/* channels used when convst gpio is defined */
-	struct iio_chan_spec		convst_channel[2];
   	void (*reset)(struct ad7476_state *);
   	bool				has_vref;
   	bool				has_vdrive;

@@ -41,6 +39,7 @@ struct ad7476_state {
   	struct gpio_desc		*convst_gpio;
   	struct spi_transfer		xfer;
   	struct spi_message		msg;
+	struct iio_chan_spec		channel[2];
   	int				scale_mv;
   	/*
   	 * DMA (thus cache coherency maintenance) may require the

@@ -153,24 +152,18 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
   #define AD7940_CHAN(bits) _AD7476_CHAN((bits), 15 - (bits), \
   		BIT(IIO_CHAN_INFO_RAW))
   #define AD7091R_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), 0)
-#define AD7091R_CONVST_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), \
-		BIT(IIO_CHAN_INFO_RAW))
   #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
   		BIT(IIO_CHAN_INFO_RAW))
   static const struct ad7476_chip_info ad7091_chip_info = {
   	.channel[0] = AD7091R_CHAN(12),
   	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
   	.reset = ad7091_reset,
   };
   static const struct ad7476_chip_info ad7091r_chip_info = {
   	.channel[0] = AD7091R_CHAN(12),
   	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
-	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
   	.int_vref_mv = 2500,
   	.has_vref = true,
   	.reset = ad7091_reset,

@@ -282,7 +275,7 @@ static int ad7476_probe(struct spi_device *spi)
   	const struct ad7476_chip_info *chip_info;
   	struct ad7476_state *st;
   	struct iio_dev *indio_dev;
-	int ret;
+	int ret, i;
   	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 330

Quoted 8 lines

drivers/iio/adc/ad7476.c +4 -0

   	if (!indio_dev)

@@ -332,16 +325,28 @@ static int ad7476_probe(struct spi_device *spi)
   	if (IS_ERR(st->convst_gpio))
   		return PTR_ERR(st->convst_gpio);
+	/*
+	 * This will never realize. Unless someone changes the channel specs
+	 * in this driver. And if someone does, without changing the loop
+	 * below, then we'd better immediately produce a big fat error, before

Quoted 46 lines

Quoted 26 lines

Quoted 4 lines

Quoted 2 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 334

Quoted 2 lines

drivers/iio/adc/ad7476.c +2 -0

+	 */
+	BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

I guess it make sense but still looks too fancy for this :)

Nothing else but a developer's carefulness keeps the number of channels "in sync" for these two structs. I was originally doing WARN_ON() - but then I thought that it's be even better to catch this at build time. Then I found the BUILD_BUG_ON(). I see Andy suggested static_assert() instead - I've no idea why one is preferred over other though. Let's see if I get educated by Andy :)

Quoted 8 lines

Quoted 4 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 337

Quoted 4 lines

drivers/iio/adc/ad7476.c +3 -0

+	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
+		st->channel[i] = chip_info->channel[i];
+		if (st->convst_gpio)

I would flip this an do:

	if (!st->convst_gpio)
		break;

To me this would just add an extra line of code, and more complex flow. I would definitely agree if there were more operations to be done for the 'convstart channels' - but since this is really just "if it's convstart, then set a bit" - the

Quoted 8 lines

if (foo)

	bar;

seems simpler than

if (!foo)

	break;

bar;

Yes but in this particular case, you likely would not need to do any line break afterward because of indentation. Logically it also makes sense because st->convst_gpio is a device property (not a channel one). So it makes no sense to check it for all channels (I know we only have two channels). So if you prefer, you could even do:

if (st->convst_gpio) {
	for (...)
		__set_bit(...);
}

Quoted 1 line

which also would make more sense to me.

I considered this option, but I need to populate all the channels in st->channel with the template data from chip_info->channel anyways. Hence I want to loop through the channels also when the st->convst_gpio is not there :)

Quoted 3 lines

Up to you now :)

Well, I already sent the v3. (Sorry, I should've waited a bit more but wanted to get it out before the weekend). I kept the same logic as in v2. You can still suggest improvements there!

Yours,
-- Matti

The ad7476 driver defines separate chan_spec structures for operation with and without convstart GPIO. At quick glance this may seem as if the driver did provide more than 1 data-channel to users - one for the regular data, other for the data obtained with the convstart GPIO.

The only difference between the 'convstart' and 'non convstart' -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in channel's flags.

We can drop the convstart channel spec, and related convstart macro, by allocating a mutable per driver instance channel spec an adding the flag

in probe if needed. This will simplify the driver with the cost of added memory consumption.

Assuming there aren't systems with very many ADCs and very few resources, this tradeoff seems worth making.

Simplify the driver by dropping the 'convstart' channel spec and allocating the chan spec for each driver instance.

Quoted 6 lines · Matti Vaittinen

The ad7476 driver defines separate chan_spec structures for operation with and without convstart GPIO. At quick glance this may seem as if the driver did provide more than 1 data-channel to users - one for the regular data, other for the data obtained with the convstart GPIO.

Quoted 6 lines

The only difference between the 'convstart' and 'non convstart' -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in channel's flags.

We can drop the convstart channel spec, and related convstart macro, by allocating a mutable per driver instance channel spec an adding the flag

and adding

Quoted 8 lines

in probe if needed. This will simplify the driver with the cost of added memory consumption.

Assuming there aren't systems with very many ADCs and very few resources, this tradeoff seems worth making.

Simplify the driver by dropping the 'convstart' channel spec and allocating the chan spec for each driver instance.

channel

(you already used 'channel spec' above, be consistent)

...

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 278

Quoted 2 lines

drivers/iio/adc/ad7476.c +1 -1

-       int ret;
+       int ret, i;

Why? Is 'i' going to be used to hold a signed value?

...

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 329

Quoted 2 lines

drivers/iio/adc/ad7476.c +2 -0

+       /*
+        * This will never realize. Unless someone changes the channel specs

realize --> happen

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 334

Quoted 5 lines

drivers/iio/adc/ad7476.c +5 -0

+        * in this driver. And if someone does, without changing the loop
+        * below, then we'd better immediately produce a big fat error, before
+        * the change proceeds from that developer's table.
+        */
+       BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

We have static_assert(). Why can't it be used?

Quoted 2 lines · Andy Shevchenko

On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
<[email protected]> wrote:

Quoted 3 lines

Quoted 1 line

+ BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

We have static_assert(). Why can't it be used?

Don't know. Can you please enlighten me why one is preferred over the other?

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Andy Shevchenko

On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
<[email protected]> wrote:

...

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

+ BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

We have static_assert(). Why can't it be used?

Quoted 1 line

Don't know. Can you please enlighten me why one is preferred over the other?

Despite already made changes, I answer to this. The static_assert() has at least two benefits over BUILD_BUG_ON():
- it can be declared in a global scope
- it produces more condensed (to the point) error message

That's why in general it's preferable over BUILD_BUG_ON().

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Andy Shevchenko

On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
<[email protected]> wrote:

Quoted 11 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

We have static_assert(). Why can't it be used?

Don't know. Can you please enlighten me why one is preferred over the other?

Despite already made changes, I answer to this. The static_assert() has at least two benefits over BUILD_BUG_ON():
- it can be declared in a global scope
- it produces more condensed (to the point) error message

Quoted 1 line

That's why in general it's preferable over BUILD_BUG_ON().

Thanks. It's always good to learn something new. One of the great things when working upstream :) (Although neither of those points seem to make a big difference here.

Oh, and AFAIR, there was a variant of BUILD_BUG_ON which allows you to add a message(?))

The ad7476 supports two IC variants which may have a 'convstart' -GPIO for starting the conversion. Currently the driver calls a function which tries to access the GPIO for all of the IC variants, whether they support 'convstart' or not. This is not an error because this function returns early if GPIO information is not populated.

We can do a tad better by calling this function only for the ICs which have the 'convstart' by providing a function pointer to the convstart function from the chip_info structure, and calling this function only for the ICs which have the function pointer set.

This does also allow to support ICs which require different convstart handling than the currently supported ICs.

Call convstart function only on the ICs which can support it and allow IC-specific convstart functions for the ICs which require different handling.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- Adapt to the change which removed the chip_info pointer from the driver's state structure.

The follow-up patch adding support for the ROHM BD79105 will bring different 'convstart' functions in use. The IC specific pointer will also prepare the way for this.

---
 drivers/iio/adc/ad7476.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/iio/adc/ad7476.c +1 -0

index a30eb016c11c..8914861802be 100644
@@ -30,6 +30,7 @@ struct ad7476_chip_info {
 	unsigned int			int_vref_mv;
 	struct iio_chan_spec		channel[2];
 	void (*reset)(struct ad7476_state *);
+	void (*conversion_pre_op)(struct ad7476_state *st);
 	bool				has_vref;
 	bool				has_vdrive;
 };

drivers/iio/adc/ad7476.c +4 -1

 	struct spi_transfer		xfer;
 	struct spi_message		msg;
 	struct iio_chan_spec		channel[2];

@@ -68,7 +70,8 @@ static irqreturn_t ad7476_trigger_handler(int irq, void  *p)
 	struct ad7476_state *st = iio_priv(indio_dev);
 	int b_sent;
 
-	ad7091_convst(st);
+	if (st->conversion_pre_op)
+		st->conversion_pre_op(st);
 
 	b_sent = spi_sync(st->spi, &st->msg);
 	if (b_sent < 0)

@@ -158,12 +161,14 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
 static const struct ad7476_chip_info ad7091_chip_info = {
 	.channel[0] = AD7091R_CHAN(12),
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	.conversion_pre_op = ad7091_convst,
 	.reset = ad7091_reset,
 };
 
 static const struct ad7476_chip_info ad7091r_chip_info = {
 	.channel[0] = AD7091R_CHAN(12),
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	.conversion_pre_op = ad7091_convst,
 	.int_vref_mv = 2500,
 	.has_vref = true,
 	.reset = ad7091_reset,

drivers/iio/adc/ad7476.c +0 -0

 	st->convst_gpio = devm_gpiod_get_optional(&spi->dev,
 						  "adi,conversion-start",
 						  GPIOD_OUT_LOW);

Signature · 1 line

2.50.1

Quoted 47 lines · Matti Vaittinen

The ad7476 supports two IC variants which may have a 'convstart' -GPIO for starting the conversion. Currently the driver calls a function which tries to access the GPIO for all of the IC variants, whether they support 'convstart' or not. This is not an error because this function returns early if GPIO information is not populated.

We can do a tad better by calling this function only for the ICs which have the 'convstart' by providing a function pointer to the convstart function from the chip_info structure, and calling this function only for the ICs which have the function pointer set.

This does also allow to support ICs which require different convstart handling than the currently supported ICs.

Call convstart function only on the ICs which can support it and allow IC-specific convstart functions for the ICs which require different handling.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- Adapt to the change which removed the chip_info pointer from the driver's state structure.

The follow-up patch adding support for the ROHM BD79105 will bring different 'convstart' functions in use. The IC specific pointer will also prepare the way for this.

---
  drivers/iio/adc/ad7476.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

drivers/iio/adc/ad7476.c +1 -0

index a30eb016c11c..8914861802be 100644
@@ -30,6 +30,7 @@ struct ad7476_chip_info {
  	unsigned int			int_vref_mv;
  	struct iio_chan_spec		channel[2];
  	void (*reset)(struct ad7476_state *);
+	void (*conversion_pre_op)(struct ad7476_state *st);
  	bool				has_vref;
  	bool				has_vdrive;
  };

@@ -37,6 +38,7 @@ struct ad7476_chip_info {
  struct ad7476_state {
  	struct spi_device		*spi;

drivers/iio/adc/ad7476.c struct ad7476_chip_info { · line 41

Quoted 2 lines

drivers/iio/adc/ad7476.c +1 -0

  	struct gpio_desc		*convst_gpio;
+	void (*conversion_pre_op)(struct ad7476_state *st);

Ok, I was going to reply to patch patch 5 saying I was not sure about the change. And now this makes it clear. My point would be that it's fairly easiy to end up needing chip info after probe. The above function pointer only has to exist because of patch 5. So I would better drop patch 5 and...

Quoted 48 lines · Nuno Sá

Quoted 47 lines · Matti Vaittinen

The ad7476 supports two IC variants which may have a 'convstart' -GPIO for starting the conversion. Currently the driver calls a function which tries to access the GPIO for all of the IC variants, whether they support 'convstart' or not. This is not an error because this function returns early if GPIO information is not populated.

We can do a tad better by calling this function only for the ICs which have the 'convstart' by providing a function pointer to the convstart function from the chip_info structure, and calling this function only for the ICs which have the function pointer set.

This does also allow to support ICs which require different convstart handling than the currently supported ICs.

Call convstart function only on the ICs which can support it and allow IC-specific convstart functions for the ICs which require different handling.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- Adapt to the change which removed the chip_info pointer from the driver's state structure.

The follow-up patch adding support for the ROHM BD79105 will bring different 'convstart' functions in use. The IC specific pointer will also prepare the way for this.

---
  drivers/iio/adc/ad7476.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

drivers/iio/adc/ad7476.c +1 -0

index a30eb016c11c..8914861802be 100644
@@ -30,6 +30,7 @@ struct ad7476_chip_info {
  	unsigned int			int_vref_mv;
  	struct iio_chan_spec		channel[2];
  	void (*reset)(struct ad7476_state *);
+	void (*conversion_pre_op)(struct ad7476_state *st);
  	bool				has_vref;
  	bool				has_vdrive;
  };

@@ -37,6 +38,7 @@ struct ad7476_chip_info {
  struct ad7476_state {
  	struct spi_device		*spi;

Quoted 8 lines

drivers/iio/adc/ad7476.c struct ad7476_chip_info { · line 41

Quoted 2 lines

drivers/iio/adc/ad7476.c +1 -0

  	struct gpio_desc		*convst_gpio;
+	void (*conversion_pre_op)(struct ad7476_state *st);

Ok, I was going to reply to patch patch 5 saying I was not sure about the change. And now this makes it clear. My point would be that it's fairly easiy to end up needing chip info after probe. The above function pointer only has to exist because of patch 5. So I would better drop patch 5 and...

Andy had the same comment. I personally like to only carry around stuff that is used after probe in the driver's private data. In my eyes it makes things clearer (and cleaner) as you know what is used. But yes, (also) here it leads to some duplication.

Well, I'll drop the patch 5.

Thanks!

Yours,
-- Matti

The ROHM BD79105 is a simple 16-bit ADC accessible via SPI*.

The BD79105 has a CONVSTART pin, which must be set high to start the ADC conversion. Unlike with the ad7091 and ad7091r which also have a CONVSTART pin, the BD79105 requires that the pin must remain high also for the duration of the SPI access.

(*) Couple of words about the SPI. The BD79105 has pins named as CONVSTART, SCLK, DIN and DOUT. For the curious reader, DIN is not SPI ISO.

DIN is a signal which can be used as a chip-select. When DIN is pulled low, the ADC will output the completed measurement via DOUT as SCLK is clocked. According to the data-sheet, the DIN can also be used for daisy-chaining multiple ADCs. Furthermore, DOUT can be used also for a 'data-ready' -IRQ. These modes aren't supported by this driver.

Support reading ADC scale and data from the BD79105 using SPI, when DIN is used as a chip-select.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- Fix the conversion delay for the BD79105
- Drop unnecessary GPIO check from the convstart disable
- Drop unintended whitespace change
- Fix spelling
---

 drivers/iio/adc/ad7476.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

drivers/iio/adc/ad7476.c +1 -0

index 8914861802be..aa8a522633eb 100644
@@ -31,6 +31,7 @@ struct ad7476_chip_info {
 	struct iio_chan_spec		channel[2];
 	void (*reset)(struct ad7476_state *);
 	void (*conversion_pre_op)(struct ad7476_state *st);
+	void (*conversion_post_op)(struct ad7476_state *st);
 	bool				has_vref;
 	bool				has_vdrive;
 };

drivers/iio/adc/ad7476.c +0 -0

 	struct spi_transfer		xfer;
 	struct spi_message		msg;
 	struct iio_chan_spec		channel[2];

@@ -63,6 +65,21 @@ static void ad7091_convst(struct ad7476_state *st)
 	udelay(1); /* Conversion time: 650 ns max */
 }

drivers/iio/adc/ad7476.c +22 -0

+	/* Worst case, 2790 nS required for conversion */
+	ndelay(2790);
+}
+
 static irqreturn_t ad7476_trigger_handler(int irq, void  *p)
 {
 	struct iio_poll_func *pf = p;

@@ -80,6 +97,8 @@ static irqreturn_t ad7476_trigger_handler(int irq, void  *p)
 	iio_push_to_buffers_with_ts(indio_dev, st->data, sizeof(st->data),
 				    iio_get_time_ns(indio_dev));
 done:
+	if (st->conversion_post_op)
+		st->conversion_post_op(st);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;

@@ -271,6 +290,20 @@ static const struct ad7476_chip_info ltc2314_14_chip_info = {
 	.has_vref = true,
 };
 
+static const struct ad7476_chip_info bd79105_chip_info = {
+	.channel[0] = AD7091R_CHAN(16),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	/*
+	 * The BD79105 starts ADC data conversion when the CONVSTART line is
+	 * set HIGH. The CONVSTART must be kept HIGH until the data has been
+	 * read from the ADC.
+	 */
+	.conversion_pre_op = bd79105_convst_enable,
+	.conversion_post_op = bd79105_convst_disable,
+	.has_vref = true,
+	.has_vdrive = true,
+};
+
 static const struct iio_info ad7476_info = {
 	.read_raw = &ad7476_read_raw,
 };

@@ -325,6 +358,7 @@ static int ad7476_probe(struct spi_device *spi)
 	}
 
 	st->conversion_pre_op = chip_info->conversion_pre_op;
+	st->conversion_post_op = chip_info->conversion_post_op;
 	st->convst_gpio = devm_gpiod_get_optional(&spi->dev,
 						  "adi,conversion-start",
 						  GPIOD_OUT_LOW);

@@ -400,6 +434,7 @@ static const struct spi_device_id ad7476_id[] = {
 	{ "ads7866", (kernel_ulong_t)&ads7866_chip_info },
 	{ "ads7867", (kernel_ulong_t)&ads7867_chip_info },
 	{ "ads7868", (kernel_ulong_t)&ads7868_chip_info },
+	{ "bd79105", (kernel_ulong_t)&bd79105_chip_info },
 	/*
 	 * The ROHM BU79100G is identical to the TI's ADS7866 from the software
 	 * point of view. The binding document mandates the ADS7866 to be

Signature · 1 line

2.50.1

Quoted 21 lines · Matti Vaittinen

The ROHM BD79105 is a simple 16-bit ADC accessible via SPI*.

The BD79105 has a CONVSTART pin, which must be set high to start the ADC conversion. Unlike with the ad7091 and ad7091r which also have a CONVSTART pin, the BD79105 requires that the pin must remain high also for the duration of the SPI access.

(*) Couple of words about the SPI. The BD79105 has pins named as CONVSTART, SCLK, DIN and DOUT. For the curious reader, DIN is not SPI ISO.

DIN is a signal which can be used as a chip-select. When DIN is pulled low, the ADC will output the completed measurement via DOUT as SCLK is clocked. According to the data-sheet, the DIN can also be used for daisy-chaining multiple ADCs. Furthermore, DOUT can be used also for a 'data-ready' -IRQ. These modes aren't supported by this driver.

Support reading ADC scale and data from the BD79105 using SPI, when DIN is used as a chip-select.

Signed-off-by Matti Vaittinen <[email protected]>

Quoted 8 lines

---
Revision history:
v1 => v2:
- Fix the conversion delay for the BD79105
- Drop unnecessary GPIO check from the convstart disable
- Drop unintended whitespace change
- Fix spelling
---

IIUC, for this chip the CONV GPIO is actually mandatory no?

If so, we
should likely fail probe in case there's no GPIO. And we could also change> the dt bindings accordingly.

Some more comments inline...

Quoted 16 lines

  drivers/iio/adc/ad7476.c | 35 +++++++++++++++++++++++++++++++++++
  1 file changed, 35 insertions(+)

drivers/iio/adc/ad7476.c +1 -0

index 8914861802be..aa8a522633eb 100644
@@ -31,6 +31,7 @@ struct ad7476_chip_info {
  	struct iio_chan_spec		channel[2];
  	void (*reset)(struct ad7476_state *);
  	void (*conversion_pre_op)(struct ad7476_state *st);
+	void (*conversion_post_op)(struct ad7476_state *st);
  	bool				has_vref;
  	bool				has_vdrive;
  };

drivers/iio/adc/ad7476.c struct ad7476_state { · line 43

Quoted 4 lines

drivers/iio/adc/ad7476.c +1 -0

@@ -39,6 +40,7 @@ struct ad7476_state {
  	struct spi_device		*spi;
  	struct gpio_desc		*convst_gpio;
  	void (*conversion_pre_op)(struct ad7476_state *st);
+	void (*conversion_post_op)(struct ad7476_state *st);

Pointer duplication again :)

Quoted 7 lines

drivers/iio/adc/ad7476.c +0 -0

  	struct spi_transfer		xfer;
  	struct spi_message		msg;
  	struct iio_chan_spec		channel[2];

@@ -63,6 +65,21 @@ static void ad7091_convst(struct ad7476_state *st)
  	udelay(1); /* Conversion time: 650 ns max */
  }
 

drivers/iio/adc/ad7476.c static void ad7091_convst(struct ad7476_state *st) · line 76

Quoted 9 lines

drivers/iio/adc/ad7476.c +9 -0

+static void bd79105_convst_disable(struct ad7476_state *st)
+{
+	gpiod_set_value(st->convst_gpio, 0);
+}
+
+static void bd79105_convst_enable(struct ad7476_state *st)
+{
+	if (!st->convst_gpio)
+		return;

I think the pattern for optional GPIOs is to just call gpiod_set_value_*() and the lib handles NULL pointers. Also the above is not coeherent with bd79105_convst_disable().

drivers/iio/adc/ad7476.c static void ad7091_convst(struct ad7476_state *st) · line 78

Quoted 3 lines

drivers/iio/adc/ad7476.c +2 -0

+
+	gpiod_set_value(st->convst_gpio, 1);

gpiod_set_value_cansleep()? I do see the driver is calling the same API in other places but I do not see a reason for it... So, precursor patch?

Quoted 31 lines · Nuno Sá

Quoted 21 lines · Matti Vaittinen

The ROHM BD79105 is a simple 16-bit ADC accessible via SPI*.

The BD79105 has a CONVSTART pin, which must be set high to start the ADC conversion. Unlike with the ad7091 and ad7091r which also have a CONVSTART pin, the BD79105 requires that the pin must remain high also for the duration of the SPI access.

(*) Couple of words about the SPI. The BD79105 has pins named as CONVSTART, SCLK, DIN and DOUT. For the curious reader, DIN is not SPI ISO.

DIN is a signal which can be used as a chip-select. When DIN is pulled low, the ADC will output the completed measurement via DOUT as SCLK is clocked. According to the data-sheet, the DIN can also be used for daisy-chaining multiple ADCs. Furthermore, DOUT can be used also for a 'data-ready' -IRQ. These modes aren't supported by this driver.

Support reading ADC scale and data from the BD79105 using SPI, when DIN is used as a chip-select.

Signed-off-by Matti Vaittinen <[email protected]>

Quoted 8 lines

---
Revision history:
v1 => v2:
- Fix the conversion delay for the BD79105
- Drop unnecessary GPIO check from the convstart disable
- Drop unintended whitespace change
- Fix spelling
---

Quoted 1 line

IIUC, for this chip the CONV GPIO is actually mandatory no?

Yes. You're right.

Quoted 2 lines

If so, we
should likely fail probe in case there's no GPIO. And we could also change> the dt bindings accordingly.

I did change the dt-binding (patch 8/10):

(quoted diff) +11 -0

+  # Devices with a convstart GPIO where it is not optional
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - rohm,bd79105
+    then:
+      required:
+        - adi,conversion-start-gpios
+

I didn't want to complicate the probe with extra checks for the GPIO based on the IC-type. But I am having second thoughts - maybe it is the right thing to do as you say :) Thanks!

Quoted 35 lines

Some more comments inline...

Quoted 16 lines

  drivers/iio/adc/ad7476.c | 35 +++++++++++++++++++++++++++++++++++
  1 file changed, 35 insertions(+)

drivers/iio/adc/ad7476.c +1 -0

index 8914861802be..aa8a522633eb 100644
@@ -31,6 +31,7 @@ struct ad7476_chip_info {
  	struct iio_chan_spec		channel[2];
  	void (*reset)(struct ad7476_state *);
  	void (*conversion_pre_op)(struct ad7476_state *st);
+	void (*conversion_post_op)(struct ad7476_state *st);
  	bool				has_vref;
  	bool				has_vdrive;
  };

drivers/iio/adc/ad7476.c struct ad7476_state { · line 43

Quoted 4 lines

drivers/iio/adc/ad7476.c +1 -0

@@ -39,6 +40,7 @@ struct ad7476_state {
  	struct spi_device		*spi;
  	struct gpio_desc		*convst_gpio;
  	void (*conversion_pre_op)(struct ad7476_state *st);
+	void (*conversion_post_op)(struct ad7476_state *st);

Pointer duplication again :)

Quoted 11 lines

drivers/iio/adc/ad7476.c +5 -0

  	struct spi_transfer		xfer;
  	struct spi_message		msg;
  	struct iio_chan_spec		channel[2];

@@ -63,6 +65,21 @@ static void ad7091_convst(struct ad7476_state *st)
  	udelay(1); /* Conversion time: 650 ns max */
  }
+static void bd79105_convst_disable(struct ad7476_state *st)
+{
+	gpiod_set_value(st->convst_gpio, 0);
+}
+

Quoted 8 lines

drivers/iio/adc/ad7476.c static void ad7091_convst(struct ad7476_state *st) · line 76

Quoted 4 lines

drivers/iio/adc/ad7476.c +4 -0

+static void bd79105_convst_enable(struct ad7476_state *st)
+{
+	if (!st->convst_gpio)
+		return;

I think the pattern for optional GPIOs is to just call gpiod_set_value_*() and the lib handles NULL pointers. Also the above is not coeherent with bd79105_convst_disable().

I definitely don't want to do *delay() if there is no reason. Haven't checked the code lately, but I suppose the ndelay() is a busy-wait, blocking _everything_ on the core it is executed.

I dropped the check from the _disable() variant since it doesn't call the delay().

But now that you (and Andy) have commented on these checks...

(even though I don't really think these checks are THAT bad. It's almost as if there were some reviewer's "unconditionally comment this"-list where NULL check for the GPIO API's was written ;) These check's are quick and very clear, and they avoid a blocking busy-wait)

...I see two other options. One is adding the check in probe as you suggest.

This check will however be substantially more complicated code than this NULL check here, because it needs to be performed only for the ICs which _require_ the convstart. Good thing is that it will error-out early and clearly, whereas current solution will just lead bogus values to be read if convstart is not correctly populated.

Other option would be making the conversion_*_op to return an error. I would still keep the NULL check but the bd79105_convst_enable() could error out. Delay would be avoided, user would get the error notification and bd79105_convst_disable() wouldn't get called.

TLDR; I will see how the "check in probe" you suggested plays out. Then I can omit these checks here :)

Quoted 6 lines

drivers/iio/adc/ad7476.c static void ad7091_convst(struct ad7476_state *st) · line 77

Quoted 3 lines

drivers/iio/adc/ad7476.c +2 -0

+
+	gpiod_set_value(st->convst_gpio, 1);

gpiod_set_value_cansleep()? I do see the driver is calling the same API in other places but I do not see a reason for it... So, precursor patch?

Ah. Great catch. *kicking himself*. I should've noticed...

Thanks!

Yours,
-- Matti

Quoted 32 lines · Matti Vaittinen

Quoted 31 lines · Nuno Sá

Quoted 29 lines · Matti Vaittinen

The ROHM BD79105 is a simple 16-bit ADC accessible via SPI*.

The BD79105 has a CONVSTART pin, which must be set high to start the ADC conversion. Unlike with the ad7091 and ad7091r which also have a CONVSTART pin, the BD79105 requires that the pin must remain high also for the duration of the SPI access.

(*) Couple of words about the SPI. The BD79105 has pins named as CONVSTART, SCLK, DIN and DOUT. For the curious reader, DIN is not SPI ISO.

DIN is a signal which can be used as a chip-select. When DIN is pulled low, the ADC will output the completed measurement via DOUT as SCLK is clocked. According to the data-sheet, the DIN can also be used for daisy-chaining multiple ADCs. Furthermore, DOUT can be used also for a 'data-ready' -IRQ. These modes aren't supported by this driver.

Support reading ADC scale and data from the BD79105 using SPI, when DIN is used as a chip-select.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v1 => v2:
- Fix the conversion delay for the BD79105
- Drop unnecessary GPIO check from the convstart disable
- Drop unintended whitespace change
- Fix spelling
---

Quoted 2 lines

...I see two other options. One is adding the check in probe as you suggest.

I do think this is the right approach. We should make sure no one tries to probe this device without any gpio because it will be pretty much useless so better to fail probe in the first place.

I'm also not sure
it's that complicated. Maybe just a chip_info flag like 'convgpio_mandatory' (likelly a bad name) and act accordingly when checking the return value.

The ROHM BD79105 is a simple 16-bit ADC accessible via SPI*.

The BD79105 has a CONVSTART pin, which must be set high to start the ADC conversion. Unlike with the ad7091 and ad7091r which also have a CONVSTART pin, the BD79105 requires that the pin must remain high also for the duration of the SPI access.

(*) Couple of words about the SPI. The BD79105 has pins named as CONVSTART, SCLK, DIN and DOUT. For the curious reader, DIN is not SPI ISO.

DIN is a signal which can be used as a chip-select. When DIN is pulled low, the ADC will output the completed measurement via DOUT as SCLK is clocked. According to the data-sheet, the DIN can also be used for daisy-chaining multiple ADCs. Furthermore, DOUT can be used also for a 'data-ready' -IRQ. These modes aren't supported by this driver.

Support reading ADC scale and data from the BD79105 using SPI, when DIN is used as a chip-select.

drivers/iio/adc/ad7476.c +2 -0

+       ndelay(2790);
+}

+ */

Quoted 20 lines · Matti Vaittinen

The ROHM BD79105 is a simple 16-bit ADC accessible via SPI*.

The BD79105 has a CONVSTART pin, which must be set high to start the ADC conversion. Unlike with the ad7091 and ad7091r which also have a CONVSTART pin, the BD79105 requires that the pin must remain high also for the duration of the SPI access.

(*) Couple of words about the SPI. The BD79105 has pins named as CONVSTART, SCLK, DIN and DOUT. For the curious reader, DIN is not SPI ISO.

DIN is a signal which can be used as a chip-select. When DIN is pulled low, the ADC will output the completed measurement via DOUT as SCLK is clocked. According to the data-sheet, the DIN can also be used for daisy-chaining multiple ADCs. Furthermore, DOUT can be used also for a 'data-ready' -IRQ. These modes aren't supported by this driver.

Support reading ADC scale and data from the BD79105 using SPI, when DIN is used as a chip-select.

...

drivers/iio/adc/ad7476.c static void ad7091_convst(struct ad7476_state *st) · line 76

Quoted 4 lines

drivers/iio/adc/ad7476.c +4 -0

+static void bd79105_convst_enable(struct ad7476_state *st)
+{
+       if (!st->convst_gpio)
+               return;

Still consider this unneeded churn. 3us delay is tolerable in almost any setup with this driver.

drivers/iio/adc/ad7476.c static void ad7091_convst(struct ad7476_state *st) · line 79

Quoted 2 lines

drivers/iio/adc/ad7476.c +2 -0

+       gpiod_set_value(st->convst_gpio, 1);
+       /* Worst case, 2790 nS required for conversion */

nS --> ns (SI unit for seconds is 's')

Quoted 2 lines

drivers/iio/adc/ad7476.c +2 -0

+       ndelay(2790);
+}

...

drivers/iio/adc/ad7476.c static const struct ad7476_chip_info ltc2314_14_chip_info = { · line 299

Quoted 4 lines

drivers/iio/adc/ad7476.c +4 -0

+       /*
+        * The BD79105 starts ADC data conversion when the CONVSTART line is
+        * set HIGH. The CONVSTART must be kept HIGH until the data has been
+        * read from the ADC.

Is this terminology in absolute levels of the pin or logical ones (that implied active-low)? If it's the latter, use active/inactive instead as the GPIO subsystem does.