[PATCH v2 00/10] Support ROHM BD79105 ADC

[PATCH v2 00/10] Support ROHM BD79105 ADC

[Matti Vaittinen]

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

Add support for the ROHM BD79105 ADC
(and do some minor simplifications to the ad7476 driver while at it).

The first 2 patches were originally sent as an RFC:
https://lore.kernel.org/all/cover.1754041258.git.mazziesaccount@gmail.com/

Revision history:
  v1 => v2:
   - Two new patches:
     5/10 "Limit the scope of the chip_info" and
     6/10 "Drop convstart chan_spec"
     Please, let me know if you think some of the changes should be
     squashed.
   - Multiple fixes as suggested during v1 review. More accurate
     changelog included in individual patches
  Simplification RFC => ROHM BD79105 support series v1:
   - Use spi_get_device_match_data()
   - Fix uV to mV conversion
   - Rewording of commit message
   - Added patches 3 to 8.

Matti Vaittinen (10):
  iio: adc: ad7476: Simplify chip type detection
  iio: adc: ad7476: Simplify scale handling
  iio: adc: ad7476: Use mV for internal reference
  iio: adc: ad7476: Use correct channel for bit info
  iio: adc: ad7476: Limit the scope of the chip_info
  iio: adc: ad7476: Drop convstart chan_spec
  iio: adc: ad7476: Conditionally call convstart
  dt-bindings: iio: adc: ad7476: Add ROHM bd79105
  iio: adc: ad7476: Support ROHM BD79105
  MAINTAINERS: A driver for simple 1-channel SPI ADCs

 .../bindings/iio/adc/adi,ad7476.yaml          |  54 ++
 MAINTAINERS                                   |   5 +
 drivers/iio/adc/ad7476.c                      | 462 +++++++++---------
 3 files changed, 290 insertions(+), 231 deletions(-)


base-commit: 93ef68672bb353838cdf8314be8765c05768916b
-- 
2.50.1


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

[PATCH v2 01/10] iio: adc: ad7476: Simplify chip type detection

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 11392 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 and may encourage
adding IC specific quircks in the code, instead of centralizing the IC
differences in one place, the chip-info.

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. Finally, switch to the
spi_get_device_match_data() and add a check for the return value.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: Nuno Sá <nuno.sa@analog.com>

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

 This patch was originally sent as an RFC in 2 patch series:
 https://lore.kernel.org/all/cover.1754041258.git.mazziesaccount@gmail.com/
 Changes from the RFC => v1:
 - Use spi_get_device_match_data()
 - Slightly reword commit message
---
 drivers/iio/adc/ad7476.c | 296 +++++++++++++++++++--------------------
 1 file changed, 146 insertions(+), 150 deletions(-)

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index aea734aa06bd..9869526c1524 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 = {
@@ -311,8 +305,10 @@ static int ad7476_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
-	st->chip_info =
-		&ad7476_chip_info_tbl[spi_get_device_id(spi)->driver_data];
+
+	st->chip_info = spi_get_device_match_data(spi);
+	if (!st->chip_info)
+		return -ENODEV;
 
 	reg = devm_regulator_get(&spi->dev, "vcc");
 	if (IS_ERR(reg))
@@ -408,41 +404,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 --]

[PATCH v2 02/10] iio: adc: ad7476: Simplify scale handling

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 5446 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 [1] 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.

Link: https://lore.kernel.org/linux-iio/20250331122247.05c6b09d@jic23-huawei/ #1
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: Nuno Sá <nuno.sa@analog.com>

---
Revision history:
 v1 => v2:
 - use link-tag

 This patch was originally sent as an RFC in 2 patch series:
 https://lore.kernel.org/all/cover.1754041258.git.mazziesaccount@gmail.com/
 Changes from the RFC:
 - Fix the faulty conversion from  uV to mV as pointed out by David.
---
 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 9869526c1524..f117aafd8fad 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));
@@ -310,58 +294,32 @@ static int ad7476_probe(struct spi_device *spi)
 	if (!st->chip_info)
 		return -ENODEV;
 
-	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 / 1000;
+
 	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 --]

[PATCH v2 03/10] iio: adc: ad7476: Use mV for internal reference

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>
---
Revision history:
 v1 => :
 - No changes
---
 drivers/iio/adc/ad7476.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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


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

Re: [PATCH v2 03/10] iio: adc: ad7476: Use mV for internal reference

[Nuno Sá]

On Thu, Aug 07, 2025 at 12:34:18PM +0300, Matti Vaittinen wrote:
> 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 <mazziesaccount@gmail.com>
> ---
> Revision history:
>  v1 => :
>  - No changes
> ---

Makes sense

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

>  drivers/iio/adc/ad7476.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> index f117aafd8fad..7b6d36999afc 100644
> --- a/drivers/iio/adc/ad7476.c
> +++ b/drivers/iio/adc/ad7476.c
> @@ -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");
> -- 
> 2.50.1
> 

[PATCH v2 04/10] iio: adc: ad7476: Use correct channel for bit info

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>
---
Revision history:
 v1 => :
 - No changes
---
 drivers/iio/adc/ad7476.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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


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

Re: [PATCH v2 04/10] iio: adc: ad7476: Use correct channel for bit info

[Nuno Sá]

On Thu, Aug 07, 2025 at 12:34:30PM +0300, Matti Vaittinen wrote:
> 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 <mazziesaccount@gmail.com>
> ---
> Revision history:
>  v1 => :
>  - No changes
> ---

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

>  drivers/iio/adc/ad7476.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> index 7b6d36999afc..fc701267358e 100644
> --- a/drivers/iio/adc/ad7476.c
> +++ b/drivers/iio/adc/ad7476.c
> @@ -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);
> -- 
> 2.50.1
> 

[PATCH v2 05/10] iio: adc: ad7476: Limit the scope of the chip_info

[Matti Vaittinen]

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

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.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
 v1 => v2:
 - New patch
---
 drivers/iio/adc/ad7476.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index fc701267358e..e97742912b8e 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -38,7 +38,6 @@ struct ad7476_chip_info {
 
 struct ad7476_state {
 	struct spi_device		*spi;
-	const struct ad7476_chip_info	*chip_info;
 	struct gpio_desc		*convst_gpio;
 	struct spi_transfer		xfer;
 	struct spi_message		msg;
@@ -280,6 +279,7 @@ static const struct iio_info ad7476_info = {
 
 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;
@@ -290,12 +290,12 @@ static int ad7476_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 
-	st->chip_info = spi_get_device_match_data(spi);
-	if (!st->chip_info)
+	chip_info = spi_get_device_match_data(spi);
+	if (!chip_info)
 		return -ENODEV;
 
 	/* Use VCC for reference voltage if vref / internal vref aren't used */
-	if (!st->chip_info->int_vref_mv && !st->chip_info->has_vref) {
+	if (!chip_info->int_vref_mv && !chip_info->has_vref) {
 		ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
 		if (ret < 0)
 			return ret;
@@ -306,11 +306,11 @@ static int ad7476_probe(struct spi_device *spi)
 			return ret;
 	}
 
-	if (st->chip_info->has_vref) {
+	if (chip_info->has_vref) {
 		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_mv || ret != -ENODEV)
+			if (!chip_info->int_vref_mv || ret != -ENODEV)
 				return ret;
 		} else {
 			st->scale_mv = ret / 1000;
@@ -318,9 +318,9 @@ static int ad7476_probe(struct spi_device *spi)
 	}
 
 	if (!st->scale_mv)
-		st->scale_mv = st->chip_info->int_vref_mv;
+		st->scale_mv = chip_info->int_vref_mv;
 
-	if (st->chip_info->has_vdrive) {
+	if (chip_info->has_vdrive) {
 		ret = devm_regulator_get_enable(&spi->dev, "vdrive");
 		if (ret)
 			return ret;
@@ -336,12 +336,12 @@ static int ad7476_probe(struct spi_device *spi)
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = st->chip_info->channel;
+	indio_dev->channels = chip_info->channel;
 	indio_dev->num_channels = 2;
 	indio_dev->info = &ad7476_info;
 
 	if (st->convst_gpio)
-		indio_dev->channels = st->chip_info->convst_channel;
+		indio_dev->channels = chip_info->convst_channel;
 	/* Setup default message */
 
 	st->xfer.rx_buf = &st->data;
@@ -355,8 +355,8 @@ static int ad7476_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	if (st->chip_info->reset)
-		st->chip_info->reset(st);
+	if (chip_info->reset)
+		chip_info->reset(st);
 
 	return devm_iio_device_register(&spi->dev, indio_dev);
 }
-- 
2.50.1


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

Re: [PATCH v2 05/10] iio: adc: ad7476: Limit the scope of the chip_info

[Andy Shevchenko]

On Thu, Aug 7, 2025 at 11:34 AM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
>
> 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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 05/10] iio: adc: ad7476: Limit the scope of the chip_info

[Matti Vaittinen]

On 08/08/2025 00:12, Andy Shevchenko wrote:
> On Thu, Aug 7, 2025 at 11:34 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>>
>> 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...
> 

I actually think both :)

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

Nuno had the same comment. I kind of like the idea of only having those 
bits of chip_info that are used after probe, stored in the "state 
struct". Or, to reverse this, I don't like having the unused (after the 
probe) data stored in the state struct. For me it is both clearer, and 
cleaner.

But yes, as You and Nuno pointed out, this leads to some data 
duplication. If the opinions were "1 against 1", I would try discussing 
this - but meh, I'll drop this as you both suggested.

Thanks for the review Nuno & Andy.

Yours,
	-- Matti

[PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Matti Vaittinen]

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

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

---
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));
+	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;
-- 
2.50.1


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

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Nuno Sá]

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

- Nuno Sá

> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> 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));
> +	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;
> -- 
> 2.50.1
> 

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Nuno Sá]

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.

On comment though...

> 
> - Nuno Sá
> 
> > 
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > 
> > ---
> > 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 :)

> > +	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;
		
> > +			st->channel[i].info_mask_separate |=
> > +				BIT(IIO_CHAN_INFO_RAW);
		
		__set_bit()...

- Nuno Sá

> > +	}
> > +
> >  	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;
> > -- 
> > 2.50.1
> > 
> 
> 

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Matti Vaittinen]

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

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

Ok. Thanks.


Thanks for the review(s) Nuno!

Yours,
	-- Matti

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Nuno Sá]

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

Up to you now :)

- Nuno Sá

> 
> > 		
> > > > +			st->channel[i].info_mask_separate |=
> > > > +				BIT(IIO_CHAN_INFO_RAW);
> > 		
> > 		__set_bit()...
> 
> Ok. Thanks.
> 
> 
> Thanks for the review(s) Nuno!
> 
> Yours,
> 	-- Matti

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Matti Vaittinen]

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

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Nuno Sá]

On Fri, Aug 08, 2025 at 12:09:21PM +0300, Matti Vaittinen wrote:
> 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@gmail.com>
> > > > > > 
> > > > > > ---
> > > > > > 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
5eefac72163e88cc6697bec77c54e4393788e1bf> > > > > > +	 * 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
> :)

Ahh right! I completely missed that line:

st->channel[i] = chip_info->channel[i];

- Nuno Sá

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

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Andy Shevchenko]

On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
<mazziesaccount@gmail.com> 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

and adding

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

...

> -       int ret;
> +       int ret, i;

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

...

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

realize --> happen

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Matti Vaittinen]

On 08/08/2025 00:16, Andy Shevchenko wrote:
> On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
> <mazziesaccount@gmail.com> 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
> 
> and adding
> 
>> 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)
> 
> ...
> 
>> -       int ret;
>> +       int ret, i;
> 
> Why? Is 'i' going to be used to hold a signed value?
> 
> ...
> 
>> +       /*
>> +        * This will never realize. Unless someone changes the channel specs
> 
> realize --> happen
> 
>> +        * 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?

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

Thanks for the review Andy!

Yours,
	-- Matti

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Andy Shevchenko]

On Fri, Aug 8, 2025 at 7:38 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> On 08/08/2025 00:16, Andy Shevchenko wrote:
> > On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:

...

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

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Matti Vaittinen]

On 08/08/2025 15:52, Andy Shevchenko wrote:
> On Fri, Aug 8, 2025 at 7:38 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>> On 08/08/2025 00:16, Andy Shevchenko wrote:
>>> On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
> 
> ...
> 
>>>> +       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
> 
> 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(?))

Yours,
	-- Matti

Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Andy Shevchenko]

On Fri, Aug 8, 2025 at 3:30 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> On 08/08/2025 15:52, Andy Shevchenko wrote:
> > On Fri, Aug 8, 2025 at 7:38 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >> On 08/08/2025 00:16, Andy Shevchenko wrote:
> >>> On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
> >>> <mazziesaccount@gmail.com> wrote:

...

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

I think the second one is the main point in my comment. Yes, the first
one doesn't matter.

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

static_assert() can be one or two args, the second variant is with the
custom message added.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 07/10] iio: adc: ad7476: Conditionally call convstart

[Matti Vaittinen]

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

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

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index a30eb016c11c..8914861802be 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -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;
 	struct gpio_desc		*convst_gpio;
+	void (*conversion_pre_op)(struct ad7476_state *st);
 	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,
@@ -319,6 +324,7 @@ static int ad7476_probe(struct spi_device *spi)
 			return ret;
 	}
 
+	st->conversion_pre_op = chip_info->conversion_pre_op;
 	st->convst_gpio = devm_gpiod_get_optional(&spi->dev,
 						  "adi,conversion-start",
 						  GPIOD_OUT_LOW);
-- 
2.50.1


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

Re: [PATCH v2 07/10] iio: adc: ad7476: Conditionally call convstart

[Nuno Sá]

On Thu, Aug 07, 2025 at 12:35:03PM +0300, Matti Vaittinen wrote:
> 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 <mazziesaccount@gmail.com>
> ---
> 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(-)
> 
> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> index a30eb016c11c..8914861802be 100644
> --- a/drivers/iio/adc/ad7476.c
> +++ b/drivers/iio/adc/ad7476.c
> @@ -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;
>  	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...

>  	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,
> @@ -319,6 +324,7 @@ static int ad7476_probe(struct spi_device *spi)
>  			return ret;
>  	}
>  
> +	st->conversion_pre_op = chip_info->conversion_pre_op;

... no need for the above. There's no real reason for the above to be
done at runtime.

- Nuno Sá

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

Re: [PATCH v2 07/10] iio: adc: ad7476: Conditionally call convstart

[Matti Vaittinen]

On 07/08/2025 15:47, Nuno Sá wrote:
> On Thu, Aug 07, 2025 at 12:35:03PM +0300, Matti Vaittinen wrote:
>> 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 <mazziesaccount@gmail.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
>> index a30eb016c11c..8914861802be 100644
>> --- a/drivers/iio/adc/ad7476.c
>> +++ b/drivers/iio/adc/ad7476.c
>> @@ -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;
>>   	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

Re: [PATCH v2 07/10] iio: adc: ad7476: Conditionally call convstart

[Nuno Sá]

On Fri, Aug 08, 2025 at 08:43:18AM +0300, Matti Vaittinen wrote:
> On 07/08/2025 15:47, Nuno Sá wrote:
> > On Thu, Aug 07, 2025 at 12:35:03PM +0300, Matti Vaittinen wrote:
> > > 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 <mazziesaccount@gmail.com>
> > > ---
> > > 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(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> > > index a30eb016c11c..8914861802be 100644
> > > --- a/drivers/iio/adc/ad7476.c
> > > +++ b/drivers/iio/adc/ad7476.c
> > > @@ -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;
> > >   	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.

And also remember that like this you're pretending that const stuff needs to
be set at runtime which is really not the case.

- Nuno Sá

> 
> Well, I'll drop the patch 5.
> 
> Thanks!
> 
> Yours,
> 	-- Matti

[PATCH v2 08/10] dt-bindings: iio: adc: ad7476: Add ROHM bd79105

[Matti Vaittinen]

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

The ROHM BD79105 is a simple, 16-bit, 1-channel ADC with a 'CONVSTART'
pin used to start the ADC conversion. Other than the 'CONVSTART', there
are 3 supply pins (one used as a reference), analog inputs, ground and
communication pins. It's worth noting that the pin somewhat confusingly
labeled as 'DIN', is a pin which should be used as a chip-select. The IC
does not have any writable registers.

The device is designed so that the output pin can, in addition to
outputting the data, be used as a 'data-ready'-IRQ. This, however, would
require the IRQ to be masked from host side for the duration of the data
reads - and it wouldn't also work when the SPI is shared. (As access to
the other SPI devices would cause data line changes to be detected as
IRQs - and the BD79105 provides no means to detect if it has generated
an IRQ).

Hence the device-tree does not contain any IRQ properties.

Add a compatible for the bd79105.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Revision history:
 v1 => v2:
 - BD79105 can provide data-ready IRQ (or GPIO) via DOUT-pin.
---
 .../bindings/iio/adc/adi,ad7476.yaml          | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7476.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7476.yaml
index d0cb32f136e5..c411a7467651 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7476.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7476.yaml
@@ -41,6 +41,7 @@ properties:
               - adi,ad7910
               - adi,ad7920
               - adi,ad7940
+              - rohm,bd79105
               - ti,adc081s
               - ti,adc101s
               - ti,adc121s
@@ -55,6 +56,11 @@ properties:
   reg:
     maxItems: 1
 
+  interrupts:
+    description:
+      The data-ready interrupt. Provided via DOUT pin.
+    maxItems: 1
+
   vcc-supply:
     description:
       Main powersupply voltage for the chips, sometimes referred to as VDD on
@@ -75,6 +81,10 @@ properties:
     description: A GPIO used to trigger the start of a conversion
     maxItems: 1
 
+  rdy-gpios:
+    description: A GPIO for detecting the data-ready.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -82,6 +92,20 @@ required:
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
 
+# Devices with an IRQ
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - rohm,bd79105
+    then:
+      properties:
+        interrupts: true
+    else:
+      properties:
+        interrupts: false
+
   # Devices where reference is vcc
   - if:
       properties:
@@ -115,6 +139,7 @@ allOf:
               - adi,ad7274
               - adi,ad7475
               - lltc,ltc2314-14
+              - rohm,bd79105
     then:
       properties:
         vref-supply: true
@@ -131,6 +156,7 @@ allOf:
               - adi,ad7274
               - adi,ad7475
               - lltc,ltc2314-14
+              - rohm,bd79105
     then:
       required:
         - vref-supply
@@ -141,12 +167,28 @@ allOf:
             enum:
               - adi,ad7475
               - adi,ad7495
+              - rohm,bd79105
     then:
       properties:
         vdrive-supply: true
     else:
       properties:
         vdrive-supply: false
+
+  # Devices which support polling the data-ready via GPIO
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - rohm,bd79105
+    then:
+      properties:
+        rdy-gpios: true
+    else:
+      properties:
+        rdy-gpios: false
+
   - if:
       properties:
         compatible:
@@ -154,6 +196,7 @@ allOf:
             enum:
               - adi,ad7091
               - adi,ad7091r
+              - rohm,bd79105
     then:
       properties:
         adi,conversion-start-gpios: true
@@ -161,6 +204,17 @@ allOf:
       properties:
         adi,conversion-start-gpios: false
 
+  # Devices with a convstart GPIO where it is not optional
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - rohm,bd79105
+    then:
+      required:
+        - adi,conversion-start-gpios
+
 unevaluatedProperties: false
 
 examples:
-- 
2.50.1


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

[PATCH v2 09/10] iio: adc: ad7476: Support ROHM BD79105

[Matti Vaittinen]

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

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

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index 8914861802be..aa8a522633eb 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -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;
 };
@@ -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);
 	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);
+}
+
+static void bd79105_convst_enable(struct ad7476_state *st)
+{
+	if (!st->convst_gpio)
+		return;
+
+	gpiod_set_value(st->convst_gpio, 1);
+	/* 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
-- 
2.50.1


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

Re: [PATCH v2 09/10] iio: adc: ad7476: Support ROHM BD79105

[Nuno Sá]

On Thu, Aug 07, 2025 at 12:35:25PM +0300, Matti Vaittinen wrote:
> 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 <mazziesaccount@gmail.com>
> ---
> 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... 
>  drivers/iio/adc/ad7476.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> index 8914861802be..aa8a522633eb 100644
> --- a/drivers/iio/adc/ad7476.c
> +++ b/drivers/iio/adc/ad7476.c
> @@ -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;
>  };
> @@ -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 :)

>  	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);
> +}
> +
> +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().

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

- Nuno Sá

> +	/* 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
> -- 
> 2.50.1
> 

Re: [PATCH v2 09/10] iio: adc: ad7476: Support ROHM BD79105

[Matti Vaittinen]

On 07/08/2025 16:01, Nuno Sá wrote:
> On Thu, Aug 07, 2025 at 12:35:25PM +0300, Matti Vaittinen wrote:
>> 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 <mazziesaccount@gmail.com>
>> ---
>> 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?

Yes. You're right.

> 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):
+  # 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!

> Some more comments inline...
>>   drivers/iio/adc/ad7476.c | 35 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>
>> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
>> index 8914861802be..aa8a522633eb 100644
>> --- a/drivers/iio/adc/ad7476.c
>> +++ b/drivers/iio/adc/ad7476.c
>> @@ -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;
>>   };
>> @@ -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 :)
> 
>>   	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);
>> +}
>> +
>> +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 :)

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

Re: [PATCH v2 09/10] iio: adc: ad7476: Support ROHM BD79105

[Nuno Sá]

On Fri, Aug 08, 2025 at 09:11:03AM +0300, Matti Vaittinen wrote:
> On 07/08/2025 16:01, Nuno Sá wrote:
> > On Thu, Aug 07, 2025 at 12:35:25PM +0300, Matti Vaittinen wrote:
> > > 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 <mazziesaccount@gmail.com>
> > > ---
> > > 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?
> 
> Yes. You're right.
> 
> > 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):
> +  # 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!
> 
> > Some more comments inline...
> > >   drivers/iio/adc/ad7476.c | 35 +++++++++++++++++++++++++++++++++++
> > >   1 file changed, 35 insertions(+)
> > > 
> > > diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> > > index 8914861802be..aa8a522633eb 100644
> > > --- a/drivers/iio/adc/ad7476.c
> > > +++ b/drivers/iio/adc/ad7476.c
> > > @@ -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;
> > >   };
> > > @@ -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 :)
> > 
> > >   	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);
> > > +}
> > > +
> > > +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().

It's a valid point but...

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

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.

- Nuno Sá

> 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 :)
> 
> > 
> > > +
> > > +	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

Re: [PATCH v2 09/10] iio: adc: ad7476: Support ROHM BD79105

[Matti Vaittinen]

On 08/08/2025 11:54, Nuno Sá wrote:
> On Fri, Aug 08, 2025 at 09:11:03AM +0300, Matti Vaittinen wrote:
>> On 07/08/2025 16:01, Nuno Sá wrote:
>>> On Thu, Aug 07, 2025 at 12:35:25PM +0300, Matti Vaittinen wrote:
>>>> 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 <mazziesaccount@gmail.com>
>>>> ---
>>>> 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
>>>> ---
>>>

...

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

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

Just sent v3 couple of minutes ago, and this was exactly what I did. 
(although with another name for the flag).

Thanks! :)

Yours,
	-- Matti

Re: [PATCH v2 09/10] iio: adc: ad7476: Support ROHM BD79105

[Andy Shevchenko]

On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
>
> 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.

...

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

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

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

> +       ndelay(2790);
> +}

...

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

> +        */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/10] iio: adc: ad7476: Support ROHM BD79105

[Matti Vaittinen]

On 08/08/2025 00:28, Andy Shevchenko wrote:
> On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>>
>> 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.
> 
> ...
> 
>> +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.

Very simple and 100% clear if (), which avoids uninterruptible 
busy-wait. I _really_ 100% disagree with you.

Luckily Nuno suggested a solution which I think will mitigate the need 
to fight over this. :)


>> +       gpiod_set_value(st->convst_gpio, 1);
>> +       /* Worst case, 2790 nS required for conversion */
> 
> nS --> ns (SI unit for seconds is 's')
> 
>> +       ndelay(2790);
>> +}
> 
> ...
> 
>> +       /*
>> +        * 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.

Ah. This is electrical high. I have:
adi,conversion-start-gpios = <&gpio2 2 GPIO_ACTIVE_HIGH>;
in the DT. Open to suggestions how to make this more obvious. Thanks!

Yours,
	-- Matti

[PATCH v2 10/10] MAINTAINERS: A driver for simple 1-channel SPI ADCs

[Matti Vaittinen]

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

Add undersigned as a maintainer for the ad7476.c which supports a few
simple 1-channel ADC connected to SPI.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

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

I'll try to keep this on eye.

I only have access to the ROHM BD79105 and BU79100g. I would welcome
anyone with access to other supported ADCs (and time, energy and the
knowledge) to join me. :)
---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f8c8f682edf6..36fa6333f7b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -455,6 +455,11 @@ F:	Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
 F:	Documentation/iio/ad7380.rst
 F:	drivers/iio/adc/ad7380.c
 
+AD7476 ADC DRIVER FOR VARIOUS SIMPLE 1-CHANNEL SPI ADCs
+M:	Matti Vaittinen <mazziesaccount@gmail.com>
+S:	Maintained
+F:	drivers/iio/adc/ad7476.c
+
 AD7877 TOUCHSCREEN DRIVER
 M:	Michael Hennerich <michael.hennerich@analog.com>
 S:	Supported
-- 
2.50.1


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