Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec
From: Matti Vaittinen
Date: Fri Aug 08 2025 - 05:09:37 EST
On 08/08/2025 12:00, Nuno Sá wrote:
On Fri, Aug 08, 2025 at 08:37:07AM +0300, Matti Vaittinen wrote:
On 07/08/2025 16:10, Nuno Sá wrote:
On Thu, Aug 07, 2025 at 01:41:31PM +0100, Nuno Sá wrote:
On Thu, Aug 07, 2025 at 12:34:52PM +0300, Matti Vaittinen wrote:
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.
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.
On comment though...
- Nuno Sá
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
---
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(-)
diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index e97742912b8e..a30eb016c11c 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -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));
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 :)
+ 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;
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.
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 :)
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