[PATCH v4 0/5] iio: adc: ti-adc128s052: Add support for adc102s051

[PATCH v4 0/5] iio: adc: ti-adc128s052: Add support for adc102s051

[Sukrut Bellary]

The patch series adds the support for adc102s051 and family.

The family of devices are easier to support since they all
(no matter the resolution) seem to respond in 12-bits with the LSBs set to
0 for the reduced resolution devices.

Changes in v4:
	Patch 1:
	- No changes in dt-bindings.

	- Rebase on v6.16-rc1.
	- split changes in multiple patches.
	- Use shift and realbits.
	- Use separate structure for each device type.
	- cleanup - fix the order.
	- Add lower resolution devices support.
	- Add MAINTAINERS entry.

- Link to v3:
	https://lore.kernel.org/lkml/20250408132120.836461-1-sbellary@baylibre.com/

Changes in v3:
	Patch 1:
	- No changes in dt-bindings

	Patch 2:
	- used be16_to_cpu() for the endian conversion.
	- used config index enum while setting up the adc128_config[]

- Link to v2:
	https://lore.kernel.org/lkml/20231022031203.632153-1-sukrut.bellary@linux.com/

Changes in v2:
	Patch 1:
	- No changes in dt-bindings

	Patch 2:
	- Arranged of_device_id and spi_device_id in numeric order.
	- Used enum to index into adc128_config.
	- Reorder adc128_config in alphabetical.
	- Include channel resolution information.
	- Shift is calculated per resolution and used in scaling and
	raw data read.

- Link to v1:
	https://lore.kernel.org/all/20220701042919.18180-1-nm@ti.com/

Sukrut Bellary (5):
  dt-bindings: iio: adc: ti,adc128s052: Add adc08c and adc10c family
  iio: adc: ti-adc128s052: Use shift and realbits
  iio: adc: ti-adc128s052: cleanup changes
  iio: adc: ti-adc128s052: Add lower resolution devices support
  MAINTAINERS: maintainer for TI's ADCs' driver ti-adc128s052

 .../bindings/iio/adc/ti,adc128s052.yaml       |   6 +
 MAINTAINERS                                   |   1 +
 drivers/iio/adc/ti-adc128s052.c               | 184 ++++++++++++------
 3 files changed, 133 insertions(+), 58 deletions(-)

-- 
2.34.1

[PATCH v4 1/5] dt-bindings: iio: adc: ti,adc128s052: Add adc08c and adc10c family

[Sukrut Bellary]

The adcxx4s communicates with a host processor via an SPI/Microwire Bus
interface. The device family responds with 12-bit data, of which the LSB bits
are 0 for the lower resolution devices. The unavailable bits are 0 in LSB.
Shift is calculated per resolution and used in scaling and raw data read.

I have been able to test adc102s051,
hence adding just the missing ones in that family.

Lets reuse the binding to support the family of devices with name
ADC<bb><c>S<sss>, where
* bb is the resolution in number of bits (8, 10, 12)
* c is the number of channels (1, 2, 4, 8)
* sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
and 101 for 1 MSPS)

Complete datasheets are available at TI's website here:
https://www.ti.com/lit/ds/symlink/adc<bb><c>s<sss>.pdf

Co-developed-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Sukrut Bellary <sbellary@baylibre.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/iio/adc/ti,adc128s052.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,adc128s052.yaml b/Documentation/devicetree/bindings/iio/adc/ti,adc128s052.yaml
index 775eee972b12..392b4a3e867c 100644
--- a/Documentation/devicetree/bindings/iio/adc/ti,adc128s052.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/ti,adc128s052.yaml
@@ -16,6 +16,12 @@ description: |
 properties:
   compatible:
     enum:
+      - ti,adc082s021
+      - ti,adc082s051
+      - ti,adc082s101
+      - ti,adc102s021
+      - ti,adc102s051
+      - ti,adc102s101
       - ti,adc122s021
       - ti,adc122s051
       - ti,adc122s101
-- 
2.34.1

[PATCH v4 2/5] iio: adc: ti-adc128s052: Use shift and realbits

[Sukrut Bellary]

This adcxx communicates with a host processor via an SPI/Microwire Bus
interface. The device family responds with 12-bit data, of which the LSB bits
are transmitted by the lower resolution devices as 0. The unavailable bits are
0 in LSB. Shift is calculated per resolution and used in scaling and raw data
read.

Create a separate structure for each device type instead of an array.
These changes help to reuse the driver to support the family of devices with
name ADC<bb><c>S<sss>, where
* bb is the resolution in number of bits (8, 10, 12)
* c is the number of channels (1, 2, 4, 8)
* sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
and 101 for 1 MSPS)

Complete datasheets are available at TI's website here:
https://www.ti.com/lit/ds/symlink/adc<bb><c>s<sss>.pdf

Co-developed-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Sukrut Bellary <sbellary@baylibre.com>
---
 drivers/iio/adc/ti-adc128s052.c | 115 ++++++++++++++++++--------------
 1 file changed, 66 insertions(+), 49 deletions(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index 1b46a8155803..2b206745e53d 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -41,13 +41,14 @@ struct adc128 {
 	} __aligned(IIO_DMA_MINALIGN);
 };
 
-static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
+static int adc128_adc_conversion(struct adc128 *adc,
+				 struct iio_chan_spec const *channel)
 {
 	int ret;
 
 	guard(mutex)(&adc->lock);
 
-	adc->buffer[0] = channel << 3;
+	adc->buffer[0] = channel->channel << 3;
 	adc->buffer[1] = 0;
 
 	ret = spi_write(adc->spi, &adc->buffer, sizeof(adc->buffer));
@@ -58,7 +59,10 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
 	if (ret < 0)
 		return ret;
 
-	return be16_to_cpu(adc->buffer16) & 0xFFF;
+	ret = (be16_to_cpu(adc->buffer16) >> channel->scan_type.shift) &
+	       GENMASK(channel->scan_type.realbits - 1, 0);
+
+	return ret;
 }
 
 static int adc128_read_raw(struct iio_dev *indio_dev,
@@ -71,7 +75,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 
-		ret = adc128_adc_conversion(adc, channel->channel);
+		ret = adc128_adc_conversion(adc, channel);
 		if (ret < 0)
 			return ret;
 
@@ -81,7 +85,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SCALE:
 
 		*val = adc->vref_mv;
-		*val2 = 12;
+		*val2 = channel->scan_type.realbits;
 		return IIO_VAL_FRACTIONAL_LOG2;
 
 	default:
@@ -90,15 +94,24 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
 
 }
 
-#define ADC128_VOLTAGE_CHANNEL(num)	\
-	{ \
-		.type = IIO_VOLTAGE, \
-		.indexed = 1, \
-		.channel = (num), \
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
-		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
+#define _ADC128_VOLTAGE_CHANNEL(num, real_bits)				\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.indexed = 1,						\
+		.channel = (num),					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+		.scan_index = (num),					\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = (real_bits),			\
+			.storagebits = 16,				\
+			.shift = (12 - real_bits),			\
+		},							\
 	}
 
+#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12)
+
 static const struct iio_chan_spec adc128s052_channels[] = {
 	ADC128_VOLTAGE_CHANNEL(0),
 	ADC128_VOLTAGE_CHANNEL(1),
@@ -124,26 +137,30 @@ static const struct iio_chan_spec adc124s021_channels[] = {
 
 static const char * const bd79104_regulators[] = { "iovdd" };
 
-static const struct adc128_configuration adc128_config[] = {
-	{
-		.channels = adc128s052_channels,
-		.num_channels = ARRAY_SIZE(adc128s052_channels),
-		.refname = "vref",
-	}, {
-		.channels = adc122s021_channels,
-		.num_channels = ARRAY_SIZE(adc122s021_channels),
-		.refname = "vref",
-	}, {
-		.channels = adc124s021_channels,
-		.num_channels = ARRAY_SIZE(adc124s021_channels),
-		.refname = "vref",
-	}, {
-		.channels = adc128s052_channels,
-		.num_channels = ARRAY_SIZE(adc128s052_channels),
-		.refname = "vdd",
-		.other_regulators = &bd79104_regulators,
-		.num_other_regulators = 1,
-	},
+static const struct adc128_configuration adc122s021_config = {
+	.channels = adc122s021_channels,
+	.num_channels = ARRAY_SIZE(adc122s021_channels),
+	.refname = "vref",
+};
+
+static const struct adc128_configuration adc124s021_config = {
+	.channels = adc124s021_channels,
+	.num_channels = ARRAY_SIZE(adc124s021_channels),
+	.refname = "vref",
+};
+
+static const struct adc128_configuration adc128s052_config = {
+	.channels = adc128s052_channels,
+	.num_channels = ARRAY_SIZE(adc128s052_channels),
+	.refname = "vref",
+};
+
+static const struct adc128_configuration bd79104_config = {
+	.channels = adc128s052_channels,
+	.num_channels = ARRAY_SIZE(adc128s052_channels),
+	.refname = "vdd",
+	.other_regulators = &bd79104_regulators,
+	.num_other_regulators = 1,
 };
 
 static const struct iio_info adc128_info = {
@@ -199,33 +216,33 @@ static int adc128_probe(struct spi_device *spi)
 }
 
 static const struct of_device_id adc128_of_match[] = {
-	{ .compatible = "ti,adc128s052", .data = &adc128_config[0] },
-	{ .compatible = "ti,adc122s021", .data = &adc128_config[1] },
-	{ .compatible = "ti,adc122s051", .data = &adc128_config[1] },
-	{ .compatible = "ti,adc122s101", .data = &adc128_config[1] },
-	{ .compatible = "ti,adc124s021", .data = &adc128_config[2] },
-	{ .compatible = "ti,adc124s051", .data = &adc128_config[2] },
-	{ .compatible = "ti,adc124s101", .data = &adc128_config[2] },
-	{ .compatible = "rohm,bd79104", .data = &adc128_config[3] },
+	{ .compatible = "ti,adc128s052", .data = &adc128s052_config },
+	{ .compatible = "ti,adc122s021", .data = &adc122s021_config },
+	{ .compatible = "ti,adc122s051", .data = &adc122s021_config },
+	{ .compatible = "ti,adc122s101", .data = &adc122s021_config },
+	{ .compatible = "ti,adc124s021", .data = &adc124s021_config },
+	{ .compatible = "ti,adc124s051", .data = &adc124s021_config },
+	{ .compatible = "ti,adc124s101", .data = &adc124s021_config },
+	{ .compatible = "rohm,bd79104",  .data = &bd79104_config },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adc128_of_match);
 
 static const struct spi_device_id adc128_id[] = {
-	{ "adc128s052", (kernel_ulong_t)&adc128_config[0] },
-	{ "adc122s021",	(kernel_ulong_t)&adc128_config[1] },
-	{ "adc122s051",	(kernel_ulong_t)&adc128_config[1] },
-	{ "adc122s101",	(kernel_ulong_t)&adc128_config[1] },
-	{ "adc124s021", (kernel_ulong_t)&adc128_config[2] },
-	{ "adc124s051", (kernel_ulong_t)&adc128_config[2] },
-	{ "adc124s101", (kernel_ulong_t)&adc128_config[2] },
-	{ "bd79104", (kernel_ulong_t)&adc128_config[3] },
+	{ "adc128s052", (kernel_ulong_t)&adc128s052_config },
+	{ "adc122s021",	(kernel_ulong_t)&adc122s021_config },
+	{ "adc122s051",	(kernel_ulong_t)&adc122s021_config },
+	{ "adc122s101",	(kernel_ulong_t)&adc122s021_config },
+	{ "adc124s021", (kernel_ulong_t)&adc124s021_config },
+	{ "adc124s051", (kernel_ulong_t)&adc124s021_config },
+	{ "adc124s101", (kernel_ulong_t)&adc124s021_config },
+	{ "bd79104",	(kernel_ulong_t)&bd79104_config },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, adc128_id);
 
 static const struct acpi_device_id adc128_acpi_match[] = {
-	{ "AANT1280", (kernel_ulong_t)&adc128_config[2] },
+	{ "AANT1280", (kernel_ulong_t)&adc124s021_config },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);
-- 
2.34.1

[PATCH v4 3/5] iio: adc: ti-adc128s052: cleanup changes

[Sukrut Bellary]

Arrange device IDs in alphabetical and numerical order. new device ID addition
can follow the same convention. Also, arrange the structures in order.
This is a cosmetic change only, and the functionality remains unchanged.

Co-developed-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Sukrut Bellary <sbellary@baylibre.com>
---
 drivers/iio/adc/ti-adc128s052.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index 2b206745e53d..fbf15c83c127 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -112,27 +112,27 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
 
 #define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12)
 
-static const struct iio_chan_spec adc128s052_channels[] = {
+static const struct iio_chan_spec adc122s021_channels[] = {
 	ADC128_VOLTAGE_CHANNEL(0),
 	ADC128_VOLTAGE_CHANNEL(1),
-	ADC128_VOLTAGE_CHANNEL(2),
-	ADC128_VOLTAGE_CHANNEL(3),
-	ADC128_VOLTAGE_CHANNEL(4),
-	ADC128_VOLTAGE_CHANNEL(5),
-	ADC128_VOLTAGE_CHANNEL(6),
-	ADC128_VOLTAGE_CHANNEL(7),
 };
 
-static const struct iio_chan_spec adc122s021_channels[] = {
+static const struct iio_chan_spec adc124s021_channels[] = {
 	ADC128_VOLTAGE_CHANNEL(0),
 	ADC128_VOLTAGE_CHANNEL(1),
+	ADC128_VOLTAGE_CHANNEL(2),
+	ADC128_VOLTAGE_CHANNEL(3),
 };
 
-static const struct iio_chan_spec adc124s021_channels[] = {
+static const struct iio_chan_spec adc128s052_channels[] = {
 	ADC128_VOLTAGE_CHANNEL(0),
 	ADC128_VOLTAGE_CHANNEL(1),
 	ADC128_VOLTAGE_CHANNEL(2),
 	ADC128_VOLTAGE_CHANNEL(3),
+	ADC128_VOLTAGE_CHANNEL(4),
+	ADC128_VOLTAGE_CHANNEL(5),
+	ADC128_VOLTAGE_CHANNEL(6),
+	ADC128_VOLTAGE_CHANNEL(7),
 };
 
 static const char * const bd79104_regulators[] = { "iovdd" };
@@ -216,27 +216,27 @@ static int adc128_probe(struct spi_device *spi)
 }
 
 static const struct of_device_id adc128_of_match[] = {
-	{ .compatible = "ti,adc128s052", .data = &adc128s052_config },
+	{ .compatible = "rohm,bd79104",  .data = &bd79104_config    },
 	{ .compatible = "ti,adc122s021", .data = &adc122s021_config },
 	{ .compatible = "ti,adc122s051", .data = &adc122s021_config },
 	{ .compatible = "ti,adc122s101", .data = &adc122s021_config },
 	{ .compatible = "ti,adc124s021", .data = &adc124s021_config },
 	{ .compatible = "ti,adc124s051", .data = &adc124s021_config },
 	{ .compatible = "ti,adc124s101", .data = &adc124s021_config },
-	{ .compatible = "rohm,bd79104",  .data = &bd79104_config },
+	{ .compatible = "ti,adc128s052", .data = &adc128s052_config },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adc128_of_match);
 
 static const struct spi_device_id adc128_id[] = {
-	{ "adc128s052", (kernel_ulong_t)&adc128s052_config },
 	{ "adc122s021",	(kernel_ulong_t)&adc122s021_config },
 	{ "adc122s051",	(kernel_ulong_t)&adc122s021_config },
 	{ "adc122s101",	(kernel_ulong_t)&adc122s021_config },
 	{ "adc124s021", (kernel_ulong_t)&adc124s021_config },
 	{ "adc124s051", (kernel_ulong_t)&adc124s021_config },
 	{ "adc124s101", (kernel_ulong_t)&adc124s021_config },
-	{ "bd79104",	(kernel_ulong_t)&bd79104_config },
+	{ "adc128s052", (kernel_ulong_t)&adc128s052_config },
+	{ "bd79104",	(kernel_ulong_t)&bd79104_config	   },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, adc128_id);
-- 
2.34.1

[PATCH v4 4/5] iio: adc: ti-adc128s052: Add lower resolution devices support

[Sukrut Bellary]

The adcxx communicates with a host processor via an SPI/Microwire Bus
interface. The device family responds with 12-bit data, of which the LSB bits
are transmitted by the lower resolution devices as 0.
The unavailable bits are 0 in LSB.
Shift is calculated per resolution and used in scaling and raw data read.

Lets reuse the driver to support the family of devices with name
ADC<bb><c>S<sss>, where
* bb is the resolution in number of bits (8, 10, 12)
* c is the number of channels (1, 2, 4, 8)
* sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
and 101 for 1 MSPS)

Complete datasheets are available at TI's website here:
https://www.ti.com/lit/ds/symlink/adc<bb><c>s<sss>.pdf

Tested only with ti-adc102s051 on BegalePlay SBC.
https://www.beagleboard.org/boards/beagleplay

Co-developed-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Sukrut Bellary <sbellary@baylibre.com>
---
 drivers/iio/adc/ti-adc128s052.c | 51 +++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index fbf15c83c127..b515ed0bb1d5 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -7,6 +7,21 @@
  * https://www.ti.com/lit/ds/symlink/adc128s052.pdf
  * https://www.ti.com/lit/ds/symlink/adc122s021.pdf
  * https://www.ti.com/lit/ds/symlink/adc124s021.pdf
+ *
+ * The adcxx4s communicates with a host processor via an SPI/Microwire Bus
+ * interface. This driver supports the whole family of devices with a name
+ * ADC<bb><c>S<sss>, where
+ * bb is the resolution in number of bits (8, 10, 12)
+ * c is the number of channels (1, 2, 4, 8)
+ * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS and
+ * 101 for 1 MSPS)
+ *
+ * Complete datasheets are available at TI's website here:
+ * https://www.ti.com/lit/ds/symlink/adc<bb><c>s<sss>.pdf
+ *
+ * 8, 10, and 12 bits converters send 12-bit data with unavailable bits set to
+ * 0 in LSB.
+ * Shift is calculated per resolution and used in scaling and raw data read.
  */
 
 #include <linux/cleanup.h>
@@ -110,8 +125,20 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
 		},							\
 	}
 
+#define ADC082_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 8)
+#define ADC102_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 10)
 #define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12)
 
+static const struct iio_chan_spec adc082s021_channels[] = {
+	ADC082_VOLTAGE_CHANNEL(0),
+	ADC082_VOLTAGE_CHANNEL(1),
+};
+
+static const struct iio_chan_spec adc102s021_channels[] = {
+	ADC102_VOLTAGE_CHANNEL(0),
+	ADC102_VOLTAGE_CHANNEL(1),
+};
+
 static const struct iio_chan_spec adc122s021_channels[] = {
 	ADC128_VOLTAGE_CHANNEL(0),
 	ADC128_VOLTAGE_CHANNEL(1),
@@ -137,6 +164,18 @@ static const struct iio_chan_spec adc128s052_channels[] = {
 
 static const char * const bd79104_regulators[] = { "iovdd" };
 
+static const struct adc128_configuration adc082s021_config = {
+	.channels = adc082s021_channels,
+	.num_channels = ARRAY_SIZE(adc082s021_channels),
+	.refname = "vref",
+};
+
+static const struct adc128_configuration adc102s021_config = {
+	.channels = adc102s021_channels,
+	.num_channels = ARRAY_SIZE(adc102s021_channels),
+	.refname = "vref",
+};
+
 static const struct adc128_configuration adc122s021_config = {
 	.channels = adc122s021_channels,
 	.num_channels = ARRAY_SIZE(adc122s021_channels),
@@ -217,6 +256,12 @@ static int adc128_probe(struct spi_device *spi)
 
 static const struct of_device_id adc128_of_match[] = {
 	{ .compatible = "rohm,bd79104",  .data = &bd79104_config    },
+	{ .compatible = "ti,adc082s021", .data = &adc082s021_config },
+	{ .compatible = "ti,adc082s051", .data = &adc082s021_config },
+	{ .compatible = "ti,adc082s101", .data = &adc082s021_config },
+	{ .compatible = "ti,adc102s021", .data = &adc102s021_config },
+	{ .compatible = "ti,adc102s051", .data = &adc102s021_config },
+	{ .compatible = "ti,adc102s101", .data = &adc102s021_config },
 	{ .compatible = "ti,adc122s021", .data = &adc122s021_config },
 	{ .compatible = "ti,adc122s051", .data = &adc122s021_config },
 	{ .compatible = "ti,adc122s101", .data = &adc122s021_config },
@@ -229,6 +274,12 @@ static const struct of_device_id adc128_of_match[] = {
 MODULE_DEVICE_TABLE(of, adc128_of_match);
 
 static const struct spi_device_id adc128_id[] = {
+	{ "adc082s021", (kernel_ulong_t)&adc082s021_config },
+	{ "adc082s051", (kernel_ulong_t)&adc082s021_config },
+	{ "adc082s101", (kernel_ulong_t)&adc082s021_config },
+	{ "adc102s021", (kernel_ulong_t)&adc102s021_config },
+	{ "adc102s051", (kernel_ulong_t)&adc102s021_config },
+	{ "adc102s101", (kernel_ulong_t)&adc102s021_config },
 	{ "adc122s021",	(kernel_ulong_t)&adc122s021_config },
 	{ "adc122s051",	(kernel_ulong_t)&adc122s021_config },
 	{ "adc122s101",	(kernel_ulong_t)&adc122s021_config },
-- 
2.34.1

[PATCH v4 5/5] MAINTAINERS: maintainer for TI's ADCs' driver ti-adc128s052

[Sukrut Bellary]

Add undersigned as a maintainer for the ti-adc128s052.c which supports a
few TI's ADCs.

Signed-off-by: Sukrut Bellary <sbellary@baylibre.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a92290fffa16..bf1c7fdcd2f1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24719,6 +24719,7 @@ F:	drivers/gpio/gpio-thunderx.c
 
 TI ADC12xs and ROHM BD79104 ADC driver
 M:	Matti Vaittinen <mazziesaccount@gmail.com>
+M:	Sukrut Bellary <sbellary@baylibre.com>
 S:	Maintained
 F:	drivers/iio/adc/ti-adc128s052.c
 L:	linux-iio@vger.kernel.org
-- 
2.34.1

Re: [PATCH v4 2/5] iio: adc: ti-adc128s052: Use shift and realbits

[Jonathan Cameron]

On Sat, 14 Jun 2025 02:15:01 -0700
Sukrut Bellary <sbellary@baylibre.com> wrote:

> This adcxx communicates with a host processor via an SPI/Microwire Bus
> interface. The device family responds with 12-bit data, of which the LSB bits
> are transmitted by the lower resolution devices as 0. The unavailable bits are
> 0 in LSB. Shift is calculated per resolution and used in scaling and raw data
> read.
> 
> Create a separate structure for each device type instead of an array.
> These changes help to reuse the driver to support the family of devices with
> name ADC<bb><c>S<sss>, where
> * bb is the resolution in number of bits (8, 10, 12)
> * c is the number of channels (1, 2, 4, 8)
> * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
> and 101 for 1 MSPS)
> 
> Complete datasheets are available at TI's website here:
> https://www.ti.com/lit/ds/symlink/adc<bb><c>s<sss>.pdf
> 
> Co-developed-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Sukrut Bellary <sbellary@baylibre.com>
> ---
>  drivers/iio/adc/ti-adc128s052.c | 115 ++++++++++++++++++--------------
>  1 file changed, 66 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index 1b46a8155803..2b206745e53d 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -41,13 +41,14 @@ struct adc128 {
>  	} __aligned(IIO_DMA_MINALIGN);
>  };
>  
> -static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
> +static int adc128_adc_conversion(struct adc128 *adc,
> +				 struct iio_chan_spec const *channel)
>  {
>  	int ret;
>  
>  	guard(mutex)(&adc->lock);
>  
> -	adc->buffer[0] = channel << 3;
> +	adc->buffer[0] = channel->channel << 3;
>  	adc->buffer[1] = 0;
>  
>  	ret = spi_write(adc->spi, &adc->buffer, sizeof(adc->buffer));
> @@ -58,7 +59,10 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
>  	if (ret < 0)
>  		return ret;
>  
> -	return be16_to_cpu(adc->buffer16) & 0xFFF;
> +	ret = (be16_to_cpu(adc->buffer16) >> channel->scan_type.shift) &
> +	       GENMASK(channel->scan_type.realbits - 1, 0);
> +
Even though it is a bit long I'd go with

	return (be16_to_cpu(adc->buffer16) >> channel->scan_type.shift) &
		GENMASK();

> +	return ret;
>  }
>  
>  static int adc128_read_raw(struct iio_dev *indio_dev,
> @@ -71,7 +75,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  
> -		ret = adc128_adc_conversion(adc, channel->channel);
> +		ret = adc128_adc_conversion(adc, channel);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -81,7 +85,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SCALE:
>  
>  		*val = adc->vref_mv;
> -		*val2 = 12;
> +		*val2 = channel->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  
>  	default:
> @@ -90,15 +94,24 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
>  
>  }
>  
> -#define ADC128_VOLTAGE_CHANNEL(num)	\
> -	{ \
> -		.type = IIO_VOLTAGE, \
> -		.indexed = 1, \
> -		.channel = (num), \
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> -		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
> +#define _ADC128_VOLTAGE_CHANNEL(num, real_bits)				\
> +	{								\

I would minimise the churn and stick to existing style of one space then \
I don't think we have any specific style guidance around this.

> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = (num),					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +		.scan_index = (num),					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = (real_bits),			\
> +			.storagebits = 16,				\
> +			.shift = (12 - real_bits),			\
> +		},							\
>  	}
>  
> +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12)

I wonder if it would be clearer to just have the 12 explicit in each entry
and skip this two levels of macro thing?

> +
>  static const struct iio_chan_spec adc128s052_channels[] = {
>  	ADC128_VOLTAGE_CHANNEL(0),
>  	ADC128_VOLTAGE_CHANNEL(1),
> @@ -124,26 +137,30 @@ static const struct iio_chan_spec adc124s021_channels[] = {
>  
>  static const char * const bd79104_regulators[] = { "iovdd" };
>  
> -static const struct adc128_configuration adc128_config[] = {
> -	{
> -		.channels = adc128s052_channels,
> -		.num_channels = ARRAY_SIZE(adc128s052_channels),
> -		.refname = "vref",
> -	}, {
> -		.channels = adc122s021_channels,
> -		.num_channels = ARRAY_SIZE(adc122s021_channels),
> -		.refname = "vref",
> -	}, {
> -		.channels = adc124s021_channels,
> -		.num_channels = ARRAY_SIZE(adc124s021_channels),
> -		.refname = "vref",
> -	}, {
> -		.channels = adc128s052_channels,
> -		.num_channels = ARRAY_SIZE(adc128s052_channels),
> -		.refname = "vdd",
> -		.other_regulators = &bd79104_regulators,
> -		.num_other_regulators = 1,
> -	},
> +static const struct adc128_configuration adc122s021_config = {
> +	.channels = adc122s021_channels,
> +	.num_channels = ARRAY_SIZE(adc122s021_channels),
> +	.refname = "vref",
> +};

Ideal would be to have this as a precursor patch rather than adding complexity
to this one which is focused on the bits related stuff.

It's a good change to have but does make it harder to spot the main
content in here.


> +
> +static const struct adc128_configuration adc124s021_config = {
> +	.channels = adc124s021_channels,
> +	.num_channels = ARRAY_SIZE(adc124s021_channels),
> +	.refname = "vref",
> +};

Re: [PATCH v4 4/5] iio: adc: ti-adc128s052: Add lower resolution devices support

[Andy Shevchenko]

On Sat, Jun 14, 2025 at 12:15 PM Sukrut Bellary <sbellary@baylibre.com> wrote:
>
> The adcxx communicates with a host processor via an SPI/Microwire Bus
> interface. The device family responds with 12-bit data, of which the LSB bits
> are transmitted by the lower resolution devices as 0.
> The unavailable bits are 0 in LSB.
> Shift is calculated per resolution and used in scaling and raw data read.
>
> Lets reuse the driver to support the family of devices with name
> ADC<bb><c>S<sss>, where

I believe it's incorrect, i.e. it's something like ...S<ss><?>, where
<?> is something you need to clarify, and <ss> is definitely a speed
in kSPS.

> * bb is the resolution in number of bits (8, 10, 12)
> * c is the number of channels (1, 2, 4, 8)
> * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
> and 101 for 1 MSPS)
>
> Complete datasheets are available at TI's website here:
> https://www.ti.com/lit/ds/symlink/adc<bb><c>s<sss>.pdf
>
> Tested only with ti-adc102s051 on BegalePlay SBC.
> https://www.beagleboard.org/boards/beagleplay

...

>   * https://www.ti.com/lit/ds/symlink/adc128s052.pdf
>   * https://www.ti.com/lit/ds/symlink/adc122s021.pdf
>   * https://www.ti.com/lit/ds/symlink/adc124s021.pdf

Forgot to sort out in the previous patch?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/5] iio: adc: ti-adc128s052: cleanup changes

[Matti Vaittinen]

On 14/06/2025 12:15, Sukrut Bellary wrote:
> Arrange device IDs in alphabetical and numerical order. new device ID addition
> can follow the same convention. Also, arrange the structures in order.
> This is a cosmetic change only, and the functionality remains unchanged.
> 
> Co-developed-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Sukrut Bellary <sbellary@baylibre.com>

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

> ---
>   drivers/iio/adc/ti-adc128s052.c | 26 +++++++++++++-------------
>   1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index 2b206745e53d..fbf15c83c127 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -112,27 +112,27 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
>   
>   #define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12)
>   
> -static const struct iio_chan_spec adc128s052_channels[] = {
> +static const struct iio_chan_spec adc122s021_channels[] = {
>   	ADC128_VOLTAGE_CHANNEL(0),
>   	ADC128_VOLTAGE_CHANNEL(1),
> -	ADC128_VOLTAGE_CHANNEL(2),
> -	ADC128_VOLTAGE_CHANNEL(3),
> -	ADC128_VOLTAGE_CHANNEL(4),
> -	ADC128_VOLTAGE_CHANNEL(5),
> -	ADC128_VOLTAGE_CHANNEL(6),
> -	ADC128_VOLTAGE_CHANNEL(7),
>   };
>   
> -static const struct iio_chan_spec adc122s021_channels[] = {
> +static const struct iio_chan_spec adc124s021_channels[] = {
>   	ADC128_VOLTAGE_CHANNEL(0),
>   	ADC128_VOLTAGE_CHANNEL(1),
> +	ADC128_VOLTAGE_CHANNEL(2),
> +	ADC128_VOLTAGE_CHANNEL(3),
>   };
>   
> -static const struct iio_chan_spec adc124s021_channels[] = {
> +static const struct iio_chan_spec adc128s052_channels[] = {
>   	ADC128_VOLTAGE_CHANNEL(0),
>   	ADC128_VOLTAGE_CHANNEL(1),
>   	ADC128_VOLTAGE_CHANNEL(2),
>   	ADC128_VOLTAGE_CHANNEL(3),
> +	ADC128_VOLTAGE_CHANNEL(4),
> +	ADC128_VOLTAGE_CHANNEL(5),
> +	ADC128_VOLTAGE_CHANNEL(6),
> +	ADC128_VOLTAGE_CHANNEL(7),
>   };
>   
>   static const char * const bd79104_regulators[] = { "iovdd" };
> @@ -216,27 +216,27 @@ static int adc128_probe(struct spi_device *spi)
>   }
>   
>   static const struct of_device_id adc128_of_match[] = {
> -	{ .compatible = "ti,adc128s052", .data = &adc128s052_config },
> +	{ .compatible = "rohm,bd79104",  .data = &bd79104_config    },
>   	{ .compatible = "ti,adc122s021", .data = &adc122s021_config },
>   	{ .compatible = "ti,adc122s051", .data = &adc122s021_config },
>   	{ .compatible = "ti,adc122s101", .data = &adc122s021_config },
>   	{ .compatible = "ti,adc124s021", .data = &adc124s021_config },
>   	{ .compatible = "ti,adc124s051", .data = &adc124s021_config },
>   	{ .compatible = "ti,adc124s101", .data = &adc124s021_config },
> -	{ .compatible = "rohm,bd79104",  .data = &bd79104_config },
> +	{ .compatible = "ti,adc128s052", .data = &adc128s052_config },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, adc128_of_match);
>   
>   static const struct spi_device_id adc128_id[] = {
> -	{ "adc128s052", (kernel_ulong_t)&adc128s052_config },
>   	{ "adc122s021",	(kernel_ulong_t)&adc122s021_config },
>   	{ "adc122s051",	(kernel_ulong_t)&adc122s021_config },
>   	{ "adc122s101",	(kernel_ulong_t)&adc122s021_config },
>   	{ "adc124s021", (kernel_ulong_t)&adc124s021_config },
>   	{ "adc124s051", (kernel_ulong_t)&adc124s021_config },
>   	{ "adc124s101", (kernel_ulong_t)&adc124s021_config },
> -	{ "bd79104",	(kernel_ulong_t)&bd79104_config },
> +	{ "adc128s052", (kernel_ulong_t)&adc128s052_config },
> +	{ "bd79104",	(kernel_ulong_t)&bd79104_config	   },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(spi, adc128_id);

Re: [PATCH v4 1/5] dt-bindings: iio: adc: ti,adc128s052: Add adc08c and adc10c family

[Krzysztof Kozlowski]

On Sat, Jun 14, 2025 at 02:15:00AM GMT, Sukrut Bellary wrote:
> The adcxx4s communicates with a host processor via an SPI/Microwire Bus
> interface. The device family responds with 12-bit data, of which the LSB bits

You have checkpatch warnings.

My ACK was given with conditional - PASSING checkpatch.

Does it pass? No. Don't add tags if you do not fulfill the criteria.

Drop the tag.

Best regards,
Krzysztof

Re: [PATCH v4 1/5] dt-bindings: iio: adc: ti,adc128s052: Add adc08c and adc10c family

[Sukrut Bellary]

On Mon, Jun 16, 2025 at 09:19:07AM +0200, Krzysztof Kozlowski wrote:
> On Sat, Jun 14, 2025 at 02:15:00AM GMT, Sukrut Bellary wrote:
> > The adcxx4s communicates with a host processor via an SPI/Microwire Bus
> > interface. The device family responds with 12-bit data, of which the LSB bits
> 
> You have checkpatch warnings.
> 
> My ACK was given with conditional - PASSING checkpatch.
> 
> Does it pass? No. Don't add tags if you do not fulfill the criteria.
> 
> Drop the tag.
> 

Thanks for the review.
Sorry about that. while rearraging the commit message, it slipped beyond
75 chars limit.
I will fix this in v5 and will use the tag with no checkpatch warnings.

> Best regards,
> Krzysztof
> 

Re: [PATCH v4 2/5] iio: adc: ti-adc128s052: Use shift and realbits

[Sukrut Bellary]

On Sat, Jun 14, 2025 at 02:27:43PM +0100, Jonathan Cameron wrote:
> On Sat, 14 Jun 2025 02:15:01 -0700
> Sukrut Bellary <sbellary@baylibre.com> wrote:
> 
> > This adcxx communicates with a host processor via an SPI/Microwire Bus
> > interface. The device family responds with 12-bit data, of which the LSB bits
> > are transmitted by the lower resolution devices as 0. The unavailable bits are
> > 0 in LSB. Shift is calculated per resolution and used in scaling and raw data
> > read.
> > 
> > Create a separate structure for each device type instead of an array.
> > These changes help to reuse the driver to support the family of devices with
> > name ADC<bb><c>S<sss>, where
> > * bb is the resolution in number of bits (8, 10, 12)
> > * c is the number of channels (1, 2, 4, 8)
> > * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
> > and 101 for 1 MSPS)
> > 
> > Complete datasheets are available at TI's website here:
> > https://www.ti.com/lit/ds/symlink/adc<bb><c>s<sss>.pdf
> > 
> > Co-developed-by: Nishanth Menon <nm@ti.com>
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > Signed-off-by: Sukrut Bellary <sbellary@baylibre.com>
> > ---
> >  drivers/iio/adc/ti-adc128s052.c | 115 ++++++++++++++++++--------------
> >  1 file changed, 66 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> > index 1b46a8155803..2b206745e53d 100644
> > --- a/drivers/iio/adc/ti-adc128s052.c
> > +++ b/drivers/iio/adc/ti-adc128s052.c
> > @@ -41,13 +41,14 @@ struct adc128 {
> >  	} __aligned(IIO_DMA_MINALIGN);
> >  };
> >  
> > -static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
> > +static int adc128_adc_conversion(struct adc128 *adc,
> > +				 struct iio_chan_spec const *channel)
> >  {
> >  	int ret;
> >  
> >  	guard(mutex)(&adc->lock);
> >  
> > -	adc->buffer[0] = channel << 3;
> > +	adc->buffer[0] = channel->channel << 3;
> >  	adc->buffer[1] = 0;
> >  
> >  	ret = spi_write(adc->spi, &adc->buffer, sizeof(adc->buffer));
> > @@ -58,7 +59,10 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	return be16_to_cpu(adc->buffer16) & 0xFFF;
> > +	ret = (be16_to_cpu(adc->buffer16) >> channel->scan_type.shift) &
> > +	       GENMASK(channel->scan_type.realbits - 1, 0);
> > +
> Even though it is a bit long I'd go with
> 
> 	return (be16_to_cpu(adc->buffer16) >> channel->scan_type.shift) &
> 		GENMASK();
>
Thanks for the review.
I will fix this in v5.

> > +	return ret;
> >  }
> >  
> >  static int adc128_read_raw(struct iio_dev *indio_dev,
> > @@ -71,7 +75,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_RAW:
> >  
> > -		ret = adc128_adc_conversion(adc, channel->channel);
> > +		ret = adc128_adc_conversion(adc, channel);
> >  		if (ret < 0)
> >  			return ret;
> >  
> > @@ -81,7 +85,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_SCALE:
> >  
> >  		*val = adc->vref_mv;
> > -		*val2 = 12;
> > +		*val2 = channel->scan_type.realbits;
> >  		return IIO_VAL_FRACTIONAL_LOG2;
> >  
> >  	default:
> > @@ -90,15 +94,24 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
> >  
> >  }
> >  
> > -#define ADC128_VOLTAGE_CHANNEL(num)	\
> > -	{ \
> > -		.type = IIO_VOLTAGE, \
> > -		.indexed = 1, \
> > -		.channel = (num), \
> > -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > -		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \
> > +#define _ADC128_VOLTAGE_CHANNEL(num, real_bits)				\
> > +	{								\
> 
> I would minimise the churn and stick to existing style of one space then \
> I don't think we have any specific style guidance around this.
>
I will fix this in v5.

> > +		.type = IIO_VOLTAGE,					\
> > +		.indexed = 1,						\
> > +		.channel = (num),					\
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > +		.scan_index = (num),					\
> > +		.scan_type = {						\
> > +			.sign = 'u',					\
> > +			.realbits = (real_bits),			\
> > +			.storagebits = 16,				\
> > +			.shift = (12 - real_bits),			\
> > +		},							\
> >  	}
> >  
> > +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12)
> 
> I wonder if it would be clearer to just have the 12 explicit in each entry
> and skip this two levels of macro thing?
>
Do you mean to pass realbits to
ADC128_VOLTAGE_CHANNEL/_ADC128_VOLTAGE_CHANNEL as e.g.,

static const struct iio_chan_spec adc122s021_channels[] = {
        ADC128_VOLTAGE_CHANNEL(0, 12),
        ADC128_VOLTAGE_CHANNEL(1, 12),
};

I think we added 2nd level macros as ADC082_VOLTAGE_CHANNEL,
ADC102_VOLTAGE_CHANNEL, etc., to have a visual distinction for a different
part nos.
But I am ok if you prefer ADC128_VOLTAGE_CHANNEL with a second parameter
as real_bits.

> > +
> >  static const struct iio_chan_spec adc128s052_channels[] = {
> >  	ADC128_VOLTAGE_CHANNEL(0),
> >  	ADC128_VOLTAGE_CHANNEL(1),
> > @@ -124,26 +137,30 @@ static const struct iio_chan_spec adc124s021_channels[] = {
> >  
> >  static const char * const bd79104_regulators[] = { "iovdd" };
> >  
> > -static const struct adc128_configuration adc128_config[] = {
> > -	{
> > -		.channels = adc128s052_channels,
> > -		.num_channels = ARRAY_SIZE(adc128s052_channels),
> > -		.refname = "vref",
> > -	}, {
> > -		.channels = adc122s021_channels,
> > -		.num_channels = ARRAY_SIZE(adc122s021_channels),
> > -		.refname = "vref",
> > -	}, {
> > -		.channels = adc124s021_channels,
> > -		.num_channels = ARRAY_SIZE(adc124s021_channels),
> > -		.refname = "vref",
> > -	}, {
> > -		.channels = adc128s052_channels,
> > -		.num_channels = ARRAY_SIZE(adc128s052_channels),
> > -		.refname = "vdd",
> > -		.other_regulators = &bd79104_regulators,
> > -		.num_other_regulators = 1,
> > -	},
> > +static const struct adc128_configuration adc122s021_config = {
> > +	.channels = adc122s021_channels,
> > +	.num_channels = ARRAY_SIZE(adc122s021_channels),
> > +	.refname = "vref",
> > +};
> 
> Ideal would be to have this as a precursor patch rather than adding complexity
> to this one which is focused on the bits related stuff.
> 
> It's a good change to have but does make it harder to spot the main
> content in here.
> 
>
I will split this in v5.

> > +
> > +static const struct adc128_configuration adc124s021_config = {
> > +	.channels = adc124s021_channels,
> > +	.num_channels = ARRAY_SIZE(adc124s021_channels),
> > +	.refname = "vref",
> > +};
> 

Re: [PATCH v4 4/5] iio: adc: ti-adc128s052: Add lower resolution devices support

[Sukrut Bellary]

On Sat, Jun 14, 2025 at 09:45:43PM +0300, Andy Shevchenko wrote:
> On Sat, Jun 14, 2025 at 12:15 PM Sukrut Bellary <sbellary@baylibre.com> wrote:
> >
> > The adcxx communicates with a host processor via an SPI/Microwire Bus
> > interface. The device family responds with 12-bit data, of which the LSB bits
> > are transmitted by the lower resolution devices as 0.
> > The unavailable bits are 0 in LSB.
> > Shift is calculated per resolution and used in scaling and raw data read.
> >
> > Lets reuse the driver to support the family of devices with name
> > ADC<bb><c>S<sss>, where
> 
> I believe it's incorrect, i.e. it's something like ...S<ss><?>, where
> <?> is something you need to clarify, and <ss> is definitely a speed
> in kSPS.
>
Thank you for the review.
I am not sure about the last s in <sss>.
It could be TI's silicon spins versioning.
I couldn't find any information about it in any of the datasheets.
I can drop the last s or mark it as <ssx> and specify the first two <ss> as
maximum speed.

> > * bb is the resolution in number of bits (8, 10, 12)
> > * c is the number of channels (1, 2, 4, 8)
> > * sss is the maximum conversion speed (021 for 200 kSPS, 051 for 500 kSPS
> > and 101 for 1 MSPS)
> >
> > Complete datasheets are available at TI's website here:
> > https://www.ti.com/lit/ds/symlink/adc<bb><c>s<sss>.pdf
> >
> > Tested only with ti-adc102s051 on BegalePlay SBC.
> > https://www.beagleboard.org/boards/beagleplay
> 
> ...
> 
> >   * https://www.ti.com/lit/ds/symlink/adc128s052.pdf
> >   * https://www.ti.com/lit/ds/symlink/adc122s021.pdf
> >   * https://www.ti.com/lit/ds/symlink/adc124s021.pdf
> 
> Forgot to sort out in the previous patch?
>
I will fix this in respin.

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 4/5] iio: adc: ti-adc128s052: Add lower resolution devices support

[David Lechner]

On 6/28/25 6:09 PM, Sukrut Bellary wrote:
> On Sat, Jun 14, 2025 at 09:45:43PM +0300, Andy Shevchenko wrote:
>> On Sat, Jun 14, 2025 at 12:15 PM Sukrut Bellary <sbellary@baylibre.com> wrote:
>>>
>>> The adcxx communicates with a host processor via an SPI/Microwire Bus
>>> interface. The device family responds with 12-bit data, of which the LSB bits
>>> are transmitted by the lower resolution devices as 0.
>>> The unavailable bits are 0 in LSB.
>>> Shift is calculated per resolution and used in scaling and raw data read.
>>>
>>> Lets reuse the driver to support the family of devices with name
>>> ADC<bb><c>S<sss>, where
>>
>> I believe it's incorrect, i.e. it's something like ...S<ss><?>, where
>> <?> is something you need to clarify, and <ss> is definitely a speed
>> in kSPS.
>>
> Thank you for the review.
> I am not sure about the last s in <sss>.
> It could be TI's silicon spins versioning.
> I couldn't find any information about it in any of the datasheets.
> I can drop the last s or mark it as <ssx> and specify the first two <ss> as
> maximum speed.
> 
I have a hunch that the last digit has to do with pinout/number of
power supplies. adc128s052 has two supplies V_A and V_D while the
others only have V_A.

If this sounds vaguely familiar, it is because it was discussed
today in this thread [1] that Jonathan CC'ed you in. :-)

[1]: https://lore.kernel.org/linux-iio/20250628162910.1256b220@jic23-huawei/

Re: [PATCH v4 4/5] iio: adc: ti-adc128s052: Add lower resolution devices support

[Andy Shevchenko]

On Sun, Jun 29, 2025 at 2:30 AM David Lechner <dlechner@baylibre.com> wrote:
>
> On 6/28/25 6:09 PM, Sukrut Bellary wrote:
> > On Sat, Jun 14, 2025 at 09:45:43PM +0300, Andy Shevchenko wrote:
> >> On Sat, Jun 14, 2025 at 12:15 PM Sukrut Bellary <sbellary@baylibre.com> wrote:
> >>>
> >>> The adcxx communicates with a host processor via an SPI/Microwire Bus
> >>> interface. The device family responds with 12-bit data, of which the LSB bits
> >>> are transmitted by the lower resolution devices as 0.
> >>> The unavailable bits are 0 in LSB.
> >>> Shift is calculated per resolution and used in scaling and raw data read.
> >>>
> >>> Lets reuse the driver to support the family of devices with name
> >>> ADC<bb><c>S<sss>, where
> >>
> >> I believe it's incorrect, i.e. it's something like ...S<ss><?>, where
> >> <?> is something you need to clarify, and <ss> is definitely a speed
> >> in kSPS.
> >>
> > Thank you for the review.
> > I am not sure about the last s in <sss>.
> > It could be TI's silicon spins versioning.
> > I couldn't find any information about it in any of the datasheets.
> > I can drop the last s or mark it as <ssx> and specify the first two <ss> as
> > maximum speed.
> >
> I have a hunch that the last digit has to do with pinout/number of
> power supplies. adc128s052 has two supplies V_A and V_D while the
> others only have V_A.
>
> If this sounds vaguely familiar, it is because it was discussed
> today in this thread [1] that Jonathan CC'ed you in. :-)

With all this being said, please, switch to <ss><p> and describe <p>,
but with the caveat that the <p> is empirically deducted based on what
community observes.

> [1]: https://lore.kernel.org/linux-iio/20250628162910.1256b220@jic23-huawei/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/5] iio: adc: ti-adc128s052: Use shift and realbits

[Jonathan Cameron]

On Sat, 28 Jun 2025 13:01:53 -0700

> > > +		.type = IIO_VOLTAGE,					\
> > > +		.indexed = 1,						\
> > > +		.channel = (num),					\
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > > +		.scan_index = (num),					\
> > > +		.scan_type = {						\
> > > +			.sign = 'u',					\
> > > +			.realbits = (real_bits),			\
> > > +			.storagebits = 16,				\
> > > +			.shift = (12 - real_bits),			\
> > > +		},							\
> > >  	}
> > >  
> > > +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12)  
> > 
> > I wonder if it would be clearer to just have the 12 explicit in each entry
> > and skip this two levels of macro thing?
> >  
> Do you mean to pass realbits to
> ADC128_VOLTAGE_CHANNEL/_ADC128_VOLTAGE_CHANNEL as e.g.,
> 
> static const struct iio_chan_spec adc122s021_channels[] = {
>         ADC128_VOLTAGE_CHANNEL(0, 12),
>         ADC128_VOLTAGE_CHANNEL(1, 12),
> };
> 
> I think we added 2nd level macros as ADC082_VOLTAGE_CHANNEL,
> ADC102_VOLTAGE_CHANNEL, etc., to have a visual distinction for a different
> part nos.

I think as we can have lots of parts with each resolution that will
end up confusing.

> But I am ok if you prefer ADC128_VOLTAGE_CHANNEL with a second parameter
> as real_bits.

I think that's going to be easier to follow.

Jonathan

Re: [PATCH v4 4/5] iio: adc: ti-adc128s052: Add lower resolution devices support

[Sukrut Bellary]

On Sun, Jun 29, 2025 at 09:06:47AM +0300, Andy Shevchenko wrote:
> On Sun, Jun 29, 2025 at 2:30 AM David Lechner <dlechner@baylibre.com> wrote:
> >
> > On 6/28/25 6:09 PM, Sukrut Bellary wrote:
> > > On Sat, Jun 14, 2025 at 09:45:43PM +0300, Andy Shevchenko wrote:
> > >> On Sat, Jun 14, 2025 at 12:15 PM Sukrut Bellary <sbellary@baylibre.com> wrote:
> > >>>
> > >>> The adcxx communicates with a host processor via an SPI/Microwire Bus
> > >>> interface. The device family responds with 12-bit data, of which the LSB bits
> > >>> are transmitted by the lower resolution devices as 0.
> > >>> The unavailable bits are 0 in LSB.
> > >>> Shift is calculated per resolution and used in scaling and raw data read.
> > >>>
> > >>> Lets reuse the driver to support the family of devices with name
> > >>> ADC<bb><c>S<sss>, where
> > >>
> > >> I believe it's incorrect, i.e. it's something like ...S<ss><?>, where
> > >> <?> is something you need to clarify, and <ss> is definitely a speed
> > >> in kSPS.
> > >>
> > > Thank you for the review.
> > > I am not sure about the last s in <sss>.
> > > It could be TI's silicon spins versioning.
> > > I couldn't find any information about it in any of the datasheets.
> > > I can drop the last s or mark it as <ssx> and specify the first two <ss> as
> > > maximum speed.
> > >
> > I have a hunch that the last digit has to do with pinout/number of
> > power supplies. adc128s052 has two supplies V_A and V_D while the
> > others only have V_A.
> >
> > If this sounds vaguely familiar, it is because it was discussed
> > today in this thread [1] that Jonathan CC'ed you in. :-)
> 
> With all this being said, please, switch to <ss><p> and describe <p>,
> but with the caveat that the <p> is empirically deducted based on what
> community observes.
> 
> > [1]: https://lore.kernel.org/linux-iio/20250628162910.1256b220@jic23-huawei/
> 
Ok, sure. I will use this in v5.

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 2/5] iio: adc: ti-adc128s052: Use shift and realbits

[Sukrut Bellary]

On Sun, Jun 29, 2025 at 05:26:55PM +0100, Jonathan Cameron wrote:
> On Sat, 28 Jun 2025 13:01:53 -0700
> 
> > > > +		.type = IIO_VOLTAGE,					\
> > > > +		.indexed = 1,						\
> > > > +		.channel = (num),					\
> > > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > > > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > > > +		.scan_index = (num),					\
> > > > +		.scan_type = {						\
> > > > +			.sign = 'u',					\
> > > > +			.realbits = (real_bits),			\
> > > > +			.storagebits = 16,				\
> > > > +			.shift = (12 - real_bits),			\
> > > > +		},							\
> > > >  	}
> > > >  
> > > > +#define ADC128_VOLTAGE_CHANNEL(num) _ADC128_VOLTAGE_CHANNEL(num, 12)  
> > > 
> > > I wonder if it would be clearer to just have the 12 explicit in each entry
> > > and skip this two levels of macro thing?
> > >  
> > Do you mean to pass realbits to
> > ADC128_VOLTAGE_CHANNEL/_ADC128_VOLTAGE_CHANNEL as e.g.,
> > 
> > static const struct iio_chan_spec adc122s021_channels[] = {
> >         ADC128_VOLTAGE_CHANNEL(0, 12),
> >         ADC128_VOLTAGE_CHANNEL(1, 12),
> > };
> > 
> > I think we added 2nd level macros as ADC082_VOLTAGE_CHANNEL,
> > ADC102_VOLTAGE_CHANNEL, etc., to have a visual distinction for a different
> > part nos.
> 
> I think as we can have lots of parts with each resolution that will
> end up confusing.
> 
> > But I am ok if you prefer ADC128_VOLTAGE_CHANNEL with a second parameter
> > as real_bits.
> 
> I think that's going to be easier to follow.
>
Ok, I will change it in v5.

> Jonathan
>