iio: adc: ad7476: Simplifications

RFC PATCH In review Matti Vaittinen 6 participants 15 messages Aug 1, 2025 – Aug 6, 2025

Subsystems (3)

IIO Analog Devices IIO drivers AD7476 ADC driver for various simple 1-channel SPI ADCs

lore

Share & tools

lore

Matti Vaittinen

author maintainer

2025-08-01 10:06 UTC #0 lore raw

cover iio: adc: ad7476: Simplifications

This series suggests some simplifications to the ad7476 ADC. It is currently 100% untested, and shouldn't be merged as is. I'd like to hear opinions on these changes before adding support to the ROHM BD79105 ADC.

Intention of the patch 1 is pretty trivial. I'd just like to hear if people think the enum + ID table approach is preferred over direct pointers to IC specific structs in SPI device's driver_data.

Real reason for the RFC version is the patch 2. It aims to clear the supply handling logic. I did also an alternate version which requires the names of the regulators to be provided in the chip_data:
https://github.com/M-Vaittinen/linux/commit/cf5b3078feb17f9a0069b2c7c86f6d980e879356

I believe the version in the link --^
is clearer, but it can potentially help people to add issues with supply enable ordering.

I can't still say if the patch 2 contained in this series is better, or if the one behind the link is better way to go. So, RFC it is :)

Matti Vaittinen (2):
iio: adc: ad7476: Simplify chip type detection
iio: adc: ad7476: Simplify scale handling

 drivers/iio/adc/ad7476.c | 376 +++++++++++++++++----------------------
 1 file changed, 164 insertions(+), 212 deletions(-)

Signature · 1 line

2.50.1

Matti Vaittinen

author maintainer

2025-08-01 10:07 UTC #1 lore raw

RFC PATCH 1/2 iio: adc: ad7476: Simplify chip type detection

The ad7476 driver uses a table of structures for defining the IC variant specific data. Table is indexed using enum values, which are picked by SPI ID.

Having the table and an enum adds extra complexity. It is potentially unsafe if someone alters the enumeration values, or size of the IC data table.

Simplify this by dropping the table and using individual structures for the IC specific data, and storing the IC specific structure's address directly in the SPI ID data.

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

---
100% Untested.
No functional changes intended

---
 drivers/iio/adc/ad7476.c | 292 +++++++++++++++++++--------------------
 1 file changed, 143 insertions(+), 149 deletions(-)

drivers/iio/adc/ad7476.c +143 ~~-149~~

index aea734aa06bd..bdfffc4f5724 100644
@@ -52,29 +52,6 @@ struct ad7476_state {
 	unsigned char data[ALIGN(2, sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
 };
 
-enum ad7476_supported_device_ids {
-	ID_AD7091,
-	ID_AD7091R,
-	ID_AD7273,
-	ID_AD7274,
-	ID_AD7276,
-	ID_AD7277,
-	ID_AD7278,
-	ID_AD7466,
-	ID_AD7467,
-	ID_AD7468,
-	ID_AD7475,
-	ID_AD7495,
-	ID_AD7940,
-	ID_ADC081S,
-	ID_ADC101S,
-	ID_ADC121S,
-	ID_ADS7866,
-	ID_ADS7867,
-	ID_ADS7868,
-	ID_LTC2314_14,
-};
-
 static void ad7091_convst(struct ad7476_state *st)
 {
 	if (!st->convst_gpio)

@@ -190,102 +167,119 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
 #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
 		BIT(IIO_CHAN_INFO_RAW))
 
-static const struct ad7476_chip_info ad7476_chip_info_tbl[] = {
-	[ID_AD7091] = {
-		.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,
-	},
-	[ID_AD7091R] = {
-		.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_uv = 2500000,
-		.has_vref = true,
-		.reset = ad7091_reset,
-	},
-	[ID_AD7273] = {
-		.channel[0] = AD7940_CHAN(10),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-		.has_vref = true,
-	},
-	[ID_AD7274] = {
-		.channel[0] = AD7940_CHAN(12),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-		.has_vref = true,
-	},
-	[ID_AD7276] = {
-		.channel[0] = AD7940_CHAN(12),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_AD7277] = {
-		.channel[0] = AD7940_CHAN(10),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_AD7278] = {
-		.channel[0] = AD7940_CHAN(8),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_AD7466] = {
-		.channel[0] = AD7476_CHAN(12),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_AD7467] = {
-		.channel[0] = AD7476_CHAN(10),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_AD7468] = {
-		.channel[0] = AD7476_CHAN(8),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_AD7475] = {
-		.channel[0] = AD7476_CHAN(12),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-		.has_vref = true,
-		.has_vdrive = true,
-	},
-	[ID_AD7495] = {
-		.channel[0] = AD7476_CHAN(12),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-		.int_vref_uv = 2500000,
-		.has_vdrive = true,
-	},
-	[ID_AD7940] = {
-		.channel[0] = AD7940_CHAN(14),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_ADC081S] = {
-		.channel[0] = ADC081S_CHAN(8),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_ADC101S] = {
-		.channel[0] = ADC081S_CHAN(10),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_ADC121S] = {
-		.channel[0] = ADC081S_CHAN(12),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_ADS7866] = {
-		.channel[0] = ADS786X_CHAN(12),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_ADS7867] = {
-		.channel[0] = ADS786X_CHAN(10),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_ADS7868] = {
-		.channel[0] = ADS786X_CHAN(8),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-	},
-	[ID_LTC2314_14] = {
-		.channel[0] = AD7940_CHAN(14),
-		.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
-		.has_vref = true,
-	},
+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_uv = 2500000,
+	.has_vref = true,
+	.reset = ad7091_reset,
+};
+
+static const struct ad7476_chip_info ad7273_chip_info = {
+	.channel[0] = AD7940_CHAN(10),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	.has_vref = true,
+};
+
+static const struct ad7476_chip_info ad7274_chip_info = {
+	.channel[0] = AD7940_CHAN(12),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	.has_vref = true,
+};
+
+static const struct ad7476_chip_info ad7276_chip_info = {
+	.channel[0] = AD7940_CHAN(12),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7277_chip_info = {
+	.channel[0] = AD7940_CHAN(10),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7278_chip_info = {
+	.channel[0] = AD7940_CHAN(8),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7466_chip_info = {
+	.channel[0] = AD7476_CHAN(12),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7467_chip_info = {
+	.channel[0] = AD7476_CHAN(10),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7468_chip_info = {
+	.channel[0] = AD7476_CHAN(8),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ad7475_chip_info = {
+	.channel[0] = AD7476_CHAN(12),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	.has_vref = true,
+	.has_vdrive = true,
+};
+
+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,
+	.has_vdrive = true,
+};
+
+static const struct ad7476_chip_info ad7940_chip_info = {
+	.channel[0] = AD7940_CHAN(14),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info adc081s_chip_info = {
+	.channel[0] = ADC081S_CHAN(8),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info adc101s_chip_info = {
+	.channel[0] = ADC081S_CHAN(10),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info adc121s_chip_info = {
+	.channel[0] = ADC081S_CHAN(12),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ads7866_chip_info = {
+	.channel[0] = ADS786X_CHAN(12),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ads7867_chip_info = {
+	.channel[0] = ADS786X_CHAN(10),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ads7868_chip_info = {
+	.channel[0] = ADS786X_CHAN(8),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct ad7476_chip_info ltc2314_14_chip_info = {
+	.channel[0] = AD7940_CHAN(14),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	.has_vref = true,
 };
 
 static const struct iio_info ad7476_info = {

@@ -312,7 +306,7 @@ static int ad7476_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 	st->chip_info =
-		&ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
+		(struct ad7476_chip_info *)spi_get_device_id(spi)->driver_data;
 
 	reg = devm_regulator_get(&spi->dev, "vcc");
 	if (IS_ERR(reg))

@@ -408,41 +402,41 @@ static int ad7476_probe(struct spi_device *spi)
 }
 
 static const struct spi_device_id ad7476_id[] = {
-	{ "ad7091", ID_AD7091 },
-	{ "ad7091r", ID_AD7091R },
-	{ "ad7273", ID_AD7273 },
-	{ "ad7274", ID_AD7274 },
-	{ "ad7276", ID_AD7276},
-	{ "ad7277", ID_AD7277 },
-	{ "ad7278", ID_AD7278 },
-	{ "ad7466", ID_AD7466 },
-	{ "ad7467", ID_AD7467 },
-	{ "ad7468", ID_AD7468 },
-	{ "ad7475", ID_AD7475 },
-	{ "ad7476", ID_AD7466 },
-	{ "ad7476a", ID_AD7466 },
-	{ "ad7477", ID_AD7467 },
-	{ "ad7477a", ID_AD7467 },
-	{ "ad7478", ID_AD7468 },
-	{ "ad7478a", ID_AD7468 },
-	{ "ad7495", ID_AD7495 },
-	{ "ad7910", ID_AD7467 },
-	{ "ad7920", ID_AD7466 },
-	{ "ad7940", ID_AD7940 },
-	{ "adc081s", ID_ADC081S },
-	{ "adc101s", ID_ADC101S },
-	{ "adc121s", ID_ADC121S },
-	{ "ads7866", ID_ADS7866 },
-	{ "ads7867", ID_ADS7867 },
-	{ "ads7868", ID_ADS7868 },
+	{ "ad7091", (kernel_ulong_t)&ad7091_chip_info },
+	{ "ad7091r", (kernel_ulong_t)&ad7091r_chip_info },
+	{ "ad7273", (kernel_ulong_t)&ad7273_chip_info },
+	{ "ad7274", (kernel_ulong_t)&ad7274_chip_info },
+	{ "ad7276", (kernel_ulong_t)&ad7276_chip_info },
+	{ "ad7277", (kernel_ulong_t)&ad7277_chip_info },
+	{ "ad7278", (kernel_ulong_t)&ad7278_chip_info },
+	{ "ad7466", (kernel_ulong_t)&ad7466_chip_info },
+	{ "ad7467", (kernel_ulong_t)&ad7467_chip_info },
+	{ "ad7468", (kernel_ulong_t)&ad7468_chip_info },
+	{ "ad7475", (kernel_ulong_t)&ad7475_chip_info },
+	{ "ad7476", (kernel_ulong_t)&ad7466_chip_info },
+	{ "ad7476a", (kernel_ulong_t)&ad7466_chip_info },
+	{ "ad7477", (kernel_ulong_t)&ad7467_chip_info },
+	{ "ad7477a", (kernel_ulong_t)&ad7467_chip_info },
+	{ "ad7478", (kernel_ulong_t)&ad7468_chip_info },
+	{ "ad7478a", (kernel_ulong_t)&ad7468_chip_info },
+	{ "ad7495", (kernel_ulong_t)&ad7495_chip_info },
+	{ "ad7910", (kernel_ulong_t)&ad7467_chip_info },
+	{ "ad7920", (kernel_ulong_t)&ad7466_chip_info },
+	{ "ad7940", (kernel_ulong_t)&ad7940_chip_info },
+	{ "adc081s", (kernel_ulong_t)&adc081s_chip_info },
+	{ "adc101s", (kernel_ulong_t)&adc101s_chip_info },
+	{ "adc121s", (kernel_ulong_t)&adc121s_chip_info },
+	{ "ads7866", (kernel_ulong_t)&ads7866_chip_info },
+	{ "ads7867", (kernel_ulong_t)&ads7867_chip_info },
+	{ "ads7868", (kernel_ulong_t)&ads7868_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
 	 * marked as a fallback for the BU79100G, but we still need the SPI ID
 	 * here to make the module loading work.
 	 */
-	{ "bu79100g", ID_ADS7866 },
-	{ "ltc2314-14", ID_LTC2314_14 },
+	{ "bu79100g", (kernel_ulong_t)&ads7866_chip_info },
+	{ "ltc2314-14", (kernel_ulong_t)&ltc2314_14_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ad7476_id);

Signature · 1 line

2.50.1

Jonathan Cameron

2025-08-01 11:09 UTC #2 lore raw

Re: [RFC PATCH 1/2] iio: adc: ad7476: Simplify chip type detection

Quoted 12 lines · On Fri, 1 Aug 2025 13:07:13 +0300 Matti Vaittinen <[email protected]>…

The ad7476 driver uses a table of structures for defining the IC variant specific data. Table is indexed using enum values, which are picked by SPI ID.
Having the table and an enum adds extra complexity. It is potentially unsafe if someone alters the enumeration values, or size of the IC data table.
Simplify this by dropping the table and using individual structures for the IC specific data, and storing the IC specific structure's address directly in the SPI ID data.

Quoted 5 lines

Signed-off-by Matti Vaittinen <[email protected]>
---
100% Untested.
No functional changes intended

One tiny thing inline, otherwise looks good to me. This aligns with how we prefer to do things these days. Tends to end up easier to read than the enum array thing and best of all removes any temptation to use the enum for anything else.

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

Quoted 8 lines

static const struct iio_info ad7476_info = {

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

@@ -312,7 +306,7 @@ static int ad7476_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 	st->chip_info =
-		&ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
+		(struct ad7476_chip_info *)spi_get_device_id(spi)->driver_data;

Switch to spi_get_device_match_data()
which checks via generic firmware paths first (so DT here) and then the old school tables. Also returns a void * so gets rid of need to cast.

Only works with all pointers (or a lot of care) because a value 0 is a fail to match. So kind of enabled by your patch.

Jonathan

Quoted 5 lines

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

 	reg = devm_regulator_get(&spi->dev, "vcc");
 	if (IS_ERR(reg))

@@ -408,41 +402,41 @@ static int ad7476_probe(struct spi_device *spi)
 }

Matti Vaittinen

author maintainer

2025-08-04 05:57 UTC #3 lore raw

Re: [RFC PATCH 1/2] iio: adc: ad7476: Simplify chip type detection

Quoted 30 lines · Jonathan Cameron

Quoted 12 lines · On Fri, 1 Aug 2025 13:07:13 +0300 Matti Vaittinen <[email protected]>…
The ad7476 driver uses a table of structures for defining the IC variant specific data. Table is indexed using enum values, which are picked by SPI ID.
Having the table and an enum adds extra complexity. It is potentially unsafe if someone alters the enumeration values, or size of the IC data table.
Simplify this by dropping the table and using individual structures for the IC specific data, and storing the IC specific structure's address directly in the SPI ID data.
Quoted 5 lines
Signed-off-by Matti Vaittinen <[email protected]>
---
100% Untested.
No functional changes intended
One tiny thing inline, otherwise looks good to me. This aligns with how we prefer to do things these days. Tends to end up easier to read than the enum array thing and best of all removes any temptation to use the enum for anything else.
Quoted 4 lines
  
  static const struct iio_info ad7476_info = {
(quoted diff) +0 -0
@@ -312,7 +306,7 @@ static int ad7476_probe(struct spi_device *spi)
 

Quoted 8 lines

drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 309
Quoted 4 lines
drivers/iio/adc/ad7476.c +1 -1
  	st = iio_priv(indio_dev);
  	st->chip_info =
-		&ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
+		(struct ad7476_chip_info *)spi_get_device_id(spi)->driver_data;
Switch to spi_get_device_match_data()
which checks via generic firmware paths first (so DT here) and then the old school tables. Also returns a void * so gets rid of need to cast.

Ah. Right! Thanks!

Quoted 2 lines

Only works with all pointers (or a lot of care) because a value 0 is a fail to match. So kind of enabled by your patch.

Yours,
-- Matti

Andy Shevchenko

2025-08-04 08:34 UTC #4 lore raw

Re: [RFC PATCH 1/2] iio: adc: ad7476: Simplify chip type detection

Quoted 3 lines · Matti Vaittinen

Quoted 2 lines · Jonathan Cameron
On Fri, 1 Aug 2025 13:07:13 +0300
Matti Vaittinen <[email protected]> wrote:

...

Quoted 10 lines

Quoted 8 lines
drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 309
Quoted 4 lines
     st = iio_priv(indio_dev);
     st->chip_info =
drivers/iio/adc/ad7476.c +1 -1
-            &ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
+            (struct ad7476_chip_info *)spi_get_device_id(spi)->driver_data;
Switch to spi_get_device_match_data()
which checks via generic firmware paths first (so DT here) and then the old school tables. Also returns a void * so gets rid of need to cast.
Ah. Right! Thanks!

More importantly it returns _const_ void *. And qualifier makes a lot of sense here.

Quoted 2 lines

Quoted 2 lines
Only works with all pointers (or a lot of care) because a value 0 is a fail to match. So kind of enabled by your patch.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2025-08-01 22:02 UTC #5 lore raw

Re: [RFC PATCH 1/2] iio: adc: ad7476: Simplify chip type detection

In reply to Matti Vaittinen

Quoted 8 lines · Matti Vaittinen

The ad7476 driver uses a table of structures for defining the IC variant specific data. Table is indexed using enum values, which are picked by SPI ID.
Having the table and an enum adds extra complexity. It is potentially unsafe if someone alters the enumeration values, or size of the IC data table.

I don't see the problem here. I like the part about converting ID tables to use chip_info instead of plain integers, but other than that I do not see how enum is worse than the split version.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Matti Vaittinen

author maintainer

2025-08-04 05:56 UTC #6 lore raw

Re: [RFC PATCH 1/2] iio: adc: ad7476: Simplify chip type detection

Quoted 7 lines · Andy Shevchenko

Quoted 5 lines · Matti Vaittinen
The ad7476 driver uses a table of structures for defining the IC variant specific data. Table is indexed using enum values, which are picked by SPI ID.

Quoted 7 lines

Quoted 3 lines
Having the table and an enum adds extra complexity. It is potentially unsafe if someone alters the enumeration values, or size of the IC data table.
I don't see the problem here. I like the part about converting ID tables to use chip_info instead of plain integers, but other than that I do not see how enum is worse than the split version.

The potential culprit with using the enum for array indexing is, that it requires the array size and enum values (used for indexing) to stay in sync. Eg, used enum values must be smaller than the size of the array. Also, the chip-info items in the array must be kept in locations which match the enum values.

Yes, we have ways to do this, often using the last enum value as the size of the array, and/or using designated array initializers. It still requires programmer to do this correctly. Changing enum at the top of the file may break the array indexing (in seemingly unrelated place, at the bottom of the file). I agree this is pretty trivial issue, but it's still a thing to keep in mind.

Splitting the chip-info in own structs and using direct pointer to the struct makes it harder to get it wrong.

Finally, dropping the enum makes adding code which does decisions based on the chip-ID less appealing. It hopefully encourages adding _all_ IC specific quirks in the chip-info instead, which will keep the code path (IMHO) cleaner when all chip-specifics are in the chip-info.

Anyways, Thanks for the feedback!

Yours,
-- Matti

Andy Shevchenko

2025-08-04 08:32 UTC #7 lore raw

Re: [RFC PATCH 1/2] iio: adc: ad7476: Simplify chip type detection

Quoted 22 lines · Matti Vaittinen

Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · Matti Vaittinen
The ad7476 driver uses a table of structures for defining the IC variant specific data. Table is indexed using enum values, which are picked by SPI ID.
Quoted 7 lines
Quoted 3 lines
Having the table and an enum adds extra complexity. It is potentially unsafe if someone alters the enumeration values, or size of the IC data table.
I don't see the problem here. I like the part about converting ID tables to use chip_info instead of plain integers, but other than that I do not see how enum is worse than the split version.
The potential culprit with using the enum for array indexing is, that it requires the array size and enum values (used for indexing) to stay in sync. Eg, used enum values must be smaller than the size of the array. Also, the chip-info items in the array must be kept in locations which match the enum values.

Quoted 4 lines

Yes, we have ways to do this, often using the last enum value as the size of the array,
and/or using designated array initializers.

That's what I kept in mind and seems already the case in this driver. That's why I doubt the brave statement in the commit message.

Quoted 6 lines

It still
requires programmer to do this correctly. Changing enum at the top of the file may break the array indexing (in seemingly unrelated place, at the bottom of the file). I agree this is pretty trivial issue, but it's still a thing to keep in mind.

Quoted 7 lines

Splitting the chip-info in own structs and using direct pointer to the struct makes it harder to get it wrong.
Finally, dropping the enum makes adding code which does decisions based on the chip-ID less appealing. It hopefully encourages adding _all_ IC specific quirks in the chip-info instead, which will keep the code path (IMHO) cleaner when all chip-specifics are in the chip-info.

Final argument makes sense to me.

Quoted 1 line

Anyways, Thanks for the feedback!

You're always welcome.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Matti Vaittinen

author maintainer

2025-08-01 10:07 UTC #8 lore raw

RFC PATCH 2/2 iio: adc: ad7476: Simplify scale handling

In reply to Matti Vaittinen

The ad7476 driver supports variants with different amount of supply regulators. On some variants there is only VCC, which is used as a reference voltage. Others have separate VREF regulator, and some rely on internal VREF. Some have both internal VREF and option to connect external one.

The ad7476 driver reads the regulator voltage only when the user asks to get the scale. This means the driver needs to do some dancing while picking the correct reference regulator (or internal reference), and store it for the later use.

According to the discussion:
https://lore.kernel.org/linux-iio/20250331122247.05c6b09d@jic23-huawei/
variable voltage references are rare, making it hard to justify the added complexity for supporting those.

Drop the support for the variable voltage references and simplify things by using the managed regulator get and enable interfaces.

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

---
NOTE: This is 100% untested as I lack of the hardware. I have tried to maintain the existing logic, but there is always some room for an error. All testing and logic reviewing is greatly appreciated.

---
 drivers/iio/adc/ad7476.c | 84 ++++++++++------------------------------
 1 file changed, 21 insertions(+), 63 deletions(-)

drivers/iio/adc/ad7476.c +21 ~~-63~~

index bdfffc4f5724..7b1d6a0fd941 100644
@@ -39,10 +39,10 @@ struct ad7476_chip_info {
 struct ad7476_state {
 	struct spi_device		*spi;
 	const struct ad7476_chip_info	*chip_info;
-	struct regulator		*ref_reg;
 	struct gpio_desc		*convst_gpio;
 	struct spi_transfer		xfer;
 	struct spi_message		msg;
+	int				scale_mv;
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
 	 * transfer buffers to live in their own cache lines.

@@ -111,7 +111,6 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
 {
 	int ret;
 	struct ad7476_state *st = iio_priv(indio_dev);
-	int scale_uv;
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:

@@ -126,14 +125,7 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
 			GENMASK(st->chip_info->channel[0].scan_type.realbits - 1, 0);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		if (st->ref_reg) {
-			scale_uv = regulator_get_voltage(st->ref_reg);
-			if (scale_uv < 0)
-				return scale_uv;
-		} else {
-			scale_uv = st->chip_info->int_vref_uv;
-		}
-		*val = scale_uv / 1000;
+		*val = st->scale_mv;
 		*val2 = chan->scan_type.realbits;
 		return IIO_VAL_FRACTIONAL_LOG2;
 	}

@@ -286,18 +278,10 @@ static const struct iio_info ad7476_info = {
 	.read_raw = &ad7476_read_raw,
 };
 
-static void ad7476_reg_disable(void *data)
-{
-	struct regulator *reg = data;
-
-	regulator_disable(reg);
-}
-
 static int ad7476_probe(struct spi_device *spi)
 {
 	struct ad7476_state *st;
 	struct iio_dev *indio_dev;
-	struct regulator *reg;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));

@@ -308,58 +292,32 @@ static int ad7476_probe(struct spi_device *spi)
 	st->chip_info =
 		(struct ad7476_chip_info *)spi_get_device_id(spi)->driver_data;
 
-	reg = devm_regulator_get(&spi->dev, "vcc");
-	if (IS_ERR(reg))
-		return PTR_ERR(reg);
-
-	ret = regulator_enable(reg);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(&spi->dev, ad7476_reg_disable, reg);
-	if (ret)
-		return ret;
-
-	/* Either vcc or vref (below) as appropriate */
-	if (!st->chip_info->int_vref_uv)
-		st->ref_reg = reg;
+	/* Use VCC for reference voltage if vref / internal vref aren't used */
+	if (!st->chip_info->int_vref_uv && !st->chip_info->has_vref) {
+		ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
+		if (ret < 0)
+			return ret;
+		st->scale_mv = ret / 1000;
+	} else {
+		ret = devm_regulator_get_enable(&spi->dev, "vcc");
+		if (ret < 0)
+			return ret;
+	}
 
 	if (st->chip_info->has_vref) {
-
-		/* If a device has an internal reference vref is optional */
-		if (st->chip_info->int_vref_uv) {
-			reg = devm_regulator_get_optional(&spi->dev, "vref");
-			if (IS_ERR(reg) && (PTR_ERR(reg) != -ENODEV))
-				return PTR_ERR(reg);
-		} else {
-			reg = devm_regulator_get(&spi->dev, "vref");
-			if (IS_ERR(reg))
-				return PTR_ERR(reg);
-		}
-
-		if (!IS_ERR(reg)) {
-			ret = regulator_enable(reg);
-			if (ret)
+		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)
 				return ret;
-
-			ret = devm_add_action_or_reset(&spi->dev,
-						       ad7476_reg_disable,
-						       reg);
-			if (ret)
-				return ret;
-			st->ref_reg = reg;
 		} else {
-			/*
-			 * Can only get here if device supports both internal
-			 * and external reference, but the regulator connected
-			 * to the external reference is not connected.
-			 * Set the reference regulator pointer to NULL to
-			 * indicate this.
-			 */
-			st->ref_reg = NULL;
+			st->scale_mv = ret / 1000;
 		}
 	}
 
+	if (!st->scale_mv)
+		st->scale_mv = st->chip_info->int_vref_uv / 100;
+
 	if (st->chip_info->has_vdrive) {
 		ret = devm_regulator_get_enable(&spi->dev, "vdrive");
 		if (ret)

Signature · 1 line

2.50.1

Jonathan Cameron

2025-08-01 11:12 UTC #9 lore raw

Re: [RFC PATCH 2/2] iio: adc: ad7476: Simplify scale handling

Quoted 16 lines · On Fri, 1 Aug 2025 13:07:39 +0300 Matti Vaittinen <[email protected]>…

The ad7476 driver supports variants with different amount of supply regulators. On some variants there is only VCC, which is used as a reference voltage. Others have separate VREF regulator, and some rely on internal VREF. Some have both internal VREF and option to connect external one.
The ad7476 driver reads the regulator voltage only when the user asks to get the scale. This means the driver needs to do some dancing while picking the correct reference regulator (or internal reference), and store it for the later use.
According to the discussion:
https://lore.kernel.org/linux-iio/20250331122247.05c6b09d@jic23-huawei/
variable voltage references are rare, making it hard to justify the added complexity for supporting those.

Quoted 4 lines

Drop the support for the variable voltage references and simplify things by using the managed regulator get and enable interfaces.
Signed-off-by Matti Vaittinen <[email protected]>

So there is risk of regression in doing this to an existing driver. I'm not that worried about it because as you note, we almost never see variable reference voltages. So this is the whole, if no one notices it's not a regression exception to Linus' rules on regressions.

Looks good to me.

Jonathan

David Lechner

reviewer

2025-08-05 16:09 UTC #10 lore raw

Re: [RFC PATCH 2/2] iio: adc: ad7476: Simplify scale handling

In reply to Matti Vaittinen

Quoted 6 lines · Matti Vaittinen

The ad7476 driver supports variants with different amount of supply regulators. On some variants there is only VCC, which is used as a reference voltage. Others have separate VREF regulator, and some rely on internal VREF. Some have both internal VREF and option to connect external one.

...

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

Quoted 3 lines

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

+	if (!st->scale_mv)
+		st->scale_mv = st->chip_info->int_vref_uv / 100;
+

Shouldn't this be 1000 rather than 100 to go from microvolts to millivolts?

Also, I would just change the chip info to int_vref_mv to avoid needing to do the division at all.

Matti Vaittinen

author maintainer

2025-08-06 05:08 UTC #11 lore raw

Re: [RFC PATCH 2/2] iio: adc: ad7476: Simplify scale handling

Hi David,

Thanks for taking a look at this :)

Quoted 8 lines · David Lechner

Quoted 6 lines · Matti Vaittinen
The ad7476 driver supports variants with different amount of supply regulators. On some variants there is only VCC, which is used as a reference voltage. Others have separate VREF regulator, and some rely on internal VREF. Some have both internal VREF and option to connect external one.

Quoted 7 lines

...
drivers/iio/adc/ad7476.c static int ad7476_probe(struct spi_device *spi) · line 320
Quoted 3 lines
drivers/iio/adc/ad7476.c +3 -0
+	if (!st->scale_mv)
+		st->scale_mv = st->chip_info->int_vref_uv / 100;
+
Shouldn't this be 1000 rather than 100 to go from microvolts to millivolts?

Yes, Thanks!

Quoted 2 lines

Also, I would just change the chip info to int_vref_mv to avoid needing to do the division at all.

Yep. I'll do that.

Yours,
-- Matti

Nuno Sá

Reviewed-by on the v1 series

2025-08-01 12:22 UTC #12 lore raw

In reply to Matti Vaittinen

Quoted 20 lines · Matti Vaittinen

This series suggests some simplifications to the ad7476 ADC. It is currently 100% untested, and shouldn't be merged as is. I'd like to hear opinions on these changes before adding support to the ROHM BD79105 ADC.
Intention of the patch 1 is pretty trivial. I'd just like to hear if people think the enum + ID table approach is preferred over direct pointers to IC specific structs in SPI device's driver_data.
Real reason for the RFC version is the patch 2. It aims to clear the supply handling logic. I did also an alternate version which requires the names of the regulators to be provided in the chip_data:
https://github.com/M-Vaittinen/linux/commit/cf5b3078feb17f9a0069b2c7c86f6d980e879356
I believe the version in the link --^
is clearer, but it can potentially help people to add issues with supply enable ordering.
I can't still say if the patch 2 contained in this series is better, or if the one behind the link is better way to go. So, RFC it is :)

Quoted 10 lines

Matti Vaittinen (2):
iio: adc: ad7476: Simplify chip type detection
iio: adc: ad7476: Simplify scale handling
 drivers/iio/adc/ad7476.c | 376 +++++++++++++++++----------------------
 1 file changed, 164 insertions(+), 212 deletions(-)
Signature · 1 line
2.50.1

With the suggestion given by Jonathan on the first patch:
(I also dunno there's someone with variable voltages...)

Reviewed-by Nuno Sá <[email protected]>

Jonathan Cameron

maintainer

2025-08-02 10:59 UTC #13 lore raw

In reply to Matti Vaittinen

Quoted 7 lines · On Fri, 1 Aug 2025 13:06:46 +0300 Matti Vaittinen <[email protected]>…

This series suggests some simplifications to the ad7476 ADC. It is currently 100% untested, and shouldn't be merged as is. I'd like to hear opinions on these changes before adding support to the ROHM BD79105 ADC.
Intention of the patch 1 is pretty trivial. I'd just like to hear if people think the enum + ID table approach is preferred over direct pointers to IC specific structs in SPI device's driver_data.

Definitely prefer direct pointers as you have here.

Quoted 6 lines

Real reason for the RFC version is the patch 2. It aims to clear the supply handling logic. I did also an alternate version which requires the names of the regulators to be provided in the chip_data:
https://github.com/M-Vaittinen/linux/commit/cf5b3078

Quoted 6 lines

I believe the version in the link --^
is clearer, but it can potentially help people to add issues with supply enable ordering.
I can't still say if the patch 2 contained in this series is better, or if the one behind the link is better way to go. So, RFC it is :)

I missed this (who reads cover letters?) in first look. Anyhow, having taken a quick look at that alternative I slightly prefer the one you have here.

Even if we have supply ordering issues, it seems like they are unlikely to vary randomly across supported parts so should be easy to incorporate those rules with the approach here if needed.

Jonathan

Quoted 8 lines

Matti Vaittinen (2):
iio: adc: ad7476: Simplify chip type detection
iio: adc: ad7476: Simplify scale handling
 drivers/iio/adc/ad7476.c | 376 +++++++++++++++++----------------------
 1 file changed, 164 insertions(+), 212 deletions(-)

Matti Vaittinen

author maintainer

2025-08-04 05:29 UTC #14 lore raw

Quoted 23 lines · Jonathan Cameron

Quoted 7 lines · On Fri, 1 Aug 2025 13:06:46 +0300 Matti Vaittinen <[email protected]>…
This series suggests some simplifications to the ad7476 ADC. It is currently 100% untested, and shouldn't be merged as is. I'd like to hear opinions on these changes before adding support to the ROHM BD79105 ADC.
Intention of the patch 1 is pretty trivial. I'd just like to hear if people think the enum + ID table approach is preferred over direct pointers to IC specific structs in SPI device's driver_data.
Definitely prefer direct pointers as you have here.
Quoted 10 lines
Real reason for the RFC version is the patch 2. It aims to clear the supply handling logic. I did also an alternate version which requires the names of the regulators to be provided in the chip_data:
https://github.com/M-Vaittinen/linux/commit/cf5b3078
I believe the version in the link --^
is clearer, but it can potentially help people to add issues with supply enable ordering.

Quoted 5 lines

Quoted 2 lines
I can't still say if the patch 2 contained in this series is better, or if the one behind the link is better way to go. So, RFC it is :)
I missed this (who reads cover letters?) in first look. Anyhow, having taken a quick look at that alternative I slightly prefer the one you have here.

I need to ask if the "here" refers to the patch contained in this series (let's say it's version 1)

or the
https://github.com/M-Vaittinen/linux/commit/cf5b3078
(which I shall call version 2 from now on).

Quoted 3 lines

Even if we have supply ordering issues, it seems like they are unlikely to vary randomly across supported parts so should be easy to incorporate those rules with the approach here if needed.

Reason why I mentioned the supply ordering is, that (AFAIK) enabling the supplies in wrong order may silently damage IC's in the long run. Nasty problems which may randomly manifest themselves only after a longer period of time - breaking the hardware with seemingly no reason.

As far as I know, the usual case is that the VCC (or VDD or whatchamacallit) should be enabled prior V_IO (Vdrive or whatchamacallit) or Vref. The version 1 (as well as the currently merged driver) do always enable VCC first. The version 2 does first bulk-enable to non VREF supplies and only then enables the VREF. Some ICs use VCC as VREF, which might result the VCC being enabled only after other supplies, but I didn't notice any in-tree supported ICs having both the VCC as VREF and separate Vdrive. Also, I have no proper information regarding _how_ unsafe it is to do enabling at wrong order. I suppose different ICs can be more or less tolerant towards this.

Hence, I assume this is rather safe. Problem being "assume" and "rather" - which is why I wanted to have another opinion as well :)

Thanks for the feedback all!

Yours,
-- Matti

PS. Anyone planning to attend the ELCE at Amsterdam this autumn?