[RFC PATCH 0/2] iio: adc: ad7476: Simplifications

[RFC PATCH 0/2] iio: adc: ad7476: Simplifications

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

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(-)

-- 
2.50.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 10941 bytes --]

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 <mazziesaccount@gmail.com>

---
100% Untested.
No functional changes intended
---
 drivers/iio/adc/ad7476.c | 292 +++++++++++++++++++--------------------
 1 file changed, 143 insertions(+), 149 deletions(-)

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index aea734aa06bd..bdfffc4f5724 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -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);
-- 
2.50.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 5355 bytes --]

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 <mazziesaccount@gmail.com>

---
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(-)

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index bdfffc4f5724..7b1d6a0fd941 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -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)
-- 
2.50.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jonathan Cameron]

On Fri, 1 Aug 2025 13:07:13 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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 <mazziesaccount@gmail.com>
> 
> ---
> 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.

>  
>  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;

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


>  
>  	reg = devm_regulator_get(&spi->dev, "vcc");
>  	if (IS_ERR(reg))
> @@ -408,41 +402,41 @@ static int ad7476_probe(struct spi_device *spi)
>  }

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

[Jonathan Cameron]

On Fri, 1 Aug 2025 13:07:39 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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 <mazziesaccount@gmail.com>

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

Re: [RFC PATCH 0/2] iio: adc: ad7476: Simplifications

[Nuno Sá]

On Fri, Aug 01, 2025 at 01:06:46PM +0300, Matti Vaittinen wrote:
> 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(-)
> 
> -- 
> 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á <nuno.sa@analog.com>

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

[Andy Shevchenko]

On Fri, Aug 1, 2025 at 12:07 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
>
> 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.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/2] iio: adc: ad7476: Simplifications

[Jonathan Cameron]

On Fri, 1 Aug 2025 13:06:46 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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.

> 
> 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.
> 
> 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


> 
> 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(-)
> 

Re: [RFC PATCH 0/2] iio: adc: ad7476: Simplifications

[Matti Vaittinen]

On 02/08/2025 13:59, Jonathan Cameron wrote:
> On Fri, 1 Aug 2025 13:06:46 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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.
> 
>>
>> 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.
>>
>> 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).

> 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?

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

[Matti Vaittinen]

On 02/08/2025 01:01, Andy Shevchenko wrote:
> On Fri, Aug 1, 2025 at 12:07 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>>
>> 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.

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

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

[Matti Vaittinen]

On 01/08/2025 14:09, Jonathan Cameron wrote:
> On Fri, 1 Aug 2025 13:07:13 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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 <mazziesaccount@gmail.com>
>>
>> ---
>> 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.
> 
>>   
>>   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;
> 
> 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!

> 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

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

[Andy Shevchenko]

On Mon, Aug 4, 2025 at 7:56 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> On 02/08/2025 01:01, Andy Shevchenko wrote:
> > On Fri, Aug 1, 2025 at 12:07 PM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:
> >>
> >> 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.
>
> 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.

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.

> 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.

Final argument makes sense to me.

> Anyways, Thanks for the feedback!

You're always welcome.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Aug 4, 2025 at 7:57 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> On 01/08/2025 14:09, Jonathan Cameron wrote:
> > On Fri, 1 Aug 2025 13:07:13 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:

...

> >>      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!

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

> > 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.

-- 
With Best Regards,
Andy Shevchenko

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

[David Lechner]

On 8/1/25 5:07 AM, Matti Vaittinen wrote:
> 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.
> 

...

> +	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.

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

[Matti Vaittinen]

Hi David,

Thanks for taking a look at this :)

On 05/08/2025 19:09, David Lechner wrote:
> On 8/1/25 5:07 AM, Matti Vaittinen wrote:
>> 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.
>>
> 
> ...
> 
>> +	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!

> 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