[PATCH 0/6] Support ROHM BD79104 ADC

[PATCH 0/6] Support ROHM BD79104 ADC

[Matti Vaittinen]

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

The ROHM BD79104 ADC is a 12 bit, 8-channel ADC controlled via SPI.

The communication over SPI uses similar protocol as the ti-adc128s052.
In order to avoid code duplication, the support for the ROHM IC is
added in the same driver.

The driver side differencies between the ti-adc128s052 and the ROHM
BD79104 are related to the supply regulator handling. The BD79104
requires supplies for the VDD and IOVDD. The ti-adc128s052 driver
handles only the reference voltage supply, even though the TI data-sheet
indicates it can also have separate supplies.

Hardware vise the ROHM BD79104 requires SPI MODE 3, and has also some SPI
frequency limitations.

I decided to add own binding document for the ROHM BD79104 so it is
easier to document the SPI limitations. It also allows using the supply
names from the data sheet. And finally, it gives users of this IC a
better hint that it is supported.

Finally, I didn't find maintainer information for this driver from the
MAINTAINERS file. I would like to add myself as a reviewer for the
driver, so I can stay on track of the changes to it. AFAIR, having
R-entry without M-entry was not appreciated. Any suggestions how to
handle this?

This series was based on the v6.14, but it should apply cleanly on
iio/testing - please let me know if I should rebase.

---

Matti Vaittinen (6):
  dt-bindings: ROHM BD79104 ADC
  iio: adc: ti-adc128s052: Fix ADC value on BE systems
  iio: adc: ti-adc128s052: Be consistent with arrays
  iio: adc: ti-adc128s052: Use devm_mutex_init()
  iio: adc: ti-adc128s052: Simplify using guard(mutex)
  iio: adc: ti-adc128s052: Support ROHM BD79104

 .../bindings/iio/adc/rohm,bd79104.yaml        | 69 +++++++++++++++++++
 drivers/iio/adc/Kconfig                       |  2 +-
 drivers/iio/adc/ti-adc128s052.c               | 66 +++++++++++++-----
 3 files changed, 118 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml


base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
-- 
2.48.1


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

[PATCH 1/6] dt-bindings: ROHM BD79104 ADC

[Matti Vaittinen]

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

The ROHM BD79104 is a 12-bit, 8-channel ADC with two power supply pins,
connected to SPI. It's worth noting the IC requires SPI MODE 3, (CPHA =
1, CPOL = 1).

I used an evaluation board "BD79104FV-EVK-001" from ROHM. With this
board I had problems to have things working correctly with higher SPI
clock frequencies. I didn't do thorough testing for maximum frequency
though. First attempt was 40M, then 20M and finally 4M. With 20M it
seemed as if the read values were shifted by 1 bit. With 4M it worked
fine.

The component data-sheet is not exact what comes to the maximum SPI
frequency. It says SPI frequency is 20M - "unless othervice specified".
Additionally, it says that maximum sampling rate is 1Mhz, and since
reading a sample requires writing the channel (16 bits) and reading
data (16 bits) - we get some upper limit from this.

From the "frequency is 20M, unless othervice specified" I picked the
maximum frequency 20M - and did assumption that my problems with 20M
weren't related to the BD79104 - but to the evaluation board
"BD79104FV-EVK-001".

Add bindings for the ROHM BD79104 ADC.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 .../bindings/iio/adc/rohm,bd79104.yaml        | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml b/Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml
new file mode 100644
index 000000000000..2a8ad4fdfc6b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/rohm,bd79104.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM Semiconductor BD79104 ADC
+
+maintainers:
+  - Matti Vaittinen <mazziesaccount@gmail.com>
+
+description: |
+  12 bit SPI ADC with 8 channels.
+
+properties:
+  compatible:
+    const: rohm,bd79104
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+  iovdd-supply: true
+
+# The component data-sheet says the frequency is 20M. I, however, found
+# that the ROHM evaluation board BD79104FV-EVK-001 had problems with 20M.
+# I have successfully used it with 4M. My _assumption_ is that this is not
+# the limitation of the component itself, but a limitation of the EVK.
+  spi-max-frequency:
+    maximum: 20000000
+
+  "#io-channel-cells":
+    const: 1
+
+  spi-cpha: true
+  spi-cpol: true
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - iovdd-supply
+  - spi-cpha
+  - spi-cpol
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "rohm,bd79104";
+            reg = <0>;
+            vdd-supply = <&vdd_supply>;
+            iovdd-supply = <&iovdd_supply>;
+            spi-max-frequency = <4000000>;
+            spi-cpha;
+            spi-cpol;
+            #io-channel-cells = <1>;
+        };
+    };
+...
-- 
2.48.1


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

[PATCH 2/6] iio: adc: ti-adc128s052: Fix ADC value on BE systems

[Matti Vaittinen]

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

ADCs supported by the ti-adc128s052 driver do return the ADC data in 16
bits using big-endian format. The driver does unconditionally swap the
bytes. This leads to wrong values being reported to users on big endian
systems.

Fix this by using the be16_to_cpu() instead of doing unconditional byte
swapping.

Fixes: 913b86468674 ("iio: adc: Add TI ADC128S052")
Cc: stable@vger.kernel.org
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
I have no big endian machines on my hands to test this. Problem was
spotted by reading the code, which leaves some room for errors.
Careful reviewing is appreciated!
---
 drivers/iio/adc/ti-adc128s052.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index a456ea78462f..d1e31122ea0d 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -28,19 +28,20 @@ struct adc128 {
 	struct regulator *reg;
 	struct mutex lock;
 
-	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
+	__be16 buffer __aligned(IIO_DMA_MINALIGN);
 };
 
 static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
 {
 	int ret;
+	char *msg = (char *)&adc->buffer;
 
-	mutex_lock(&adc->lock);
+	msg[0] = channel << 3;
+	msg[1] = 0;
 
-	adc->buffer[0] = channel << 3;
-	adc->buffer[1] = 0;
+	mutex_lock(&adc->lock);
 
-	ret = spi_write(adc->spi, &adc->buffer, 2);
+	ret = spi_write(adc->spi, msg, 2);
 	if (ret < 0) {
 		mutex_unlock(&adc->lock);
 		return ret;
@@ -53,7 +54,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
 	if (ret < 0)
 		return ret;
 
-	return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF);
+	return be16_to_cpu(adc->buffer) & 0xFFF;
 }
 
 static int adc128_read_raw(struct iio_dev *indio_dev,
-- 
2.48.1


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

[PATCH 3/6] iio: adc: ti-adc128s052: Be consistent with arrays

[Matti Vaittinen]

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

The ti-adc128s052 driver has NULL terminated ID arrays for the
of_device_id, spi_device_id and acpi_device_id. All of these are
terminated by having an empty string as the last member of an array.
Only the of_device_id array uses the /* sentinel */ comment.

It's better to be consistent.

This /* sentinel */ comment serves no real purpose these days as people
are used to seeing these ID lists terminated with an empty array
element.

Drop the /* sentinel */ from the of_device_id.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/adc/ti-adc128s052.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index d1e31122ea0d..dd1e405bf172 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -185,7 +185,7 @@ static const struct of_device_id adc128_of_match[] = {
 	{ .compatible = "ti,adc124s021", .data = &adc128_config[2] },
 	{ .compatible = "ti,adc124s051", .data = &adc128_config[2] },
 	{ .compatible = "ti,adc124s101", .data = &adc128_config[2] },
-	{ /* sentinel */ },
+	{ },
 };
 MODULE_DEVICE_TABLE(of, adc128_of_match);
 
-- 
2.48.1


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

[PATCH 4/6] iio: adc: ti-adc128s052: Use devm_mutex_init()

[Matti Vaittinen]

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

Quoting Jonathan:
"Whilst it doesn't bring huge advantage, now we have devm_mutex_init()
it seems reasonable to use it and maybe catch a use after free for the
lock"

Switch to use devm_mutex_init() while working on this file.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/adc/ti-adc128s052.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index dd1e405bf172..90b23c68daea 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -172,7 +172,7 @@ static int adc128_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	mutex_init(&adc->lock);
+	devm_mutex_init(&spi->dev, &adc->lock);
 
 	return devm_iio_device_register(&spi->dev, indio_dev);
 }
-- 
2.48.1


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

[PATCH 5/6] iio: adc: ti-adc128s052: Simplify using guard(mutex)

[Matti Vaittinen]

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

Error path in ADC reading function can be slighly simplified using the
guard(mutex). Do just that.

Also, document the mutex purpose while at it.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/adc/ti-adc128s052.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index 90b23c68daea..c68ee4e17a03 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -9,6 +9,7 @@
  * https://www.ti.com/lit/ds/symlink/adc124s021.pdf
  */
 
+#include <linux/cleanup.h>
 #include <linux/err.h>
 #include <linux/iio/iio.h>
 #include <linux/mod_devicetable.h>
@@ -26,6 +27,7 @@ struct adc128 {
 	struct spi_device *spi;
 
 	struct regulator *reg;
+	/* Serialize the SPI 'write-channel + read data' accesses */
 	struct mutex lock;
 
 	__be16 buffer __aligned(IIO_DMA_MINALIGN);
@@ -39,18 +41,13 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
 	msg[0] = channel << 3;
 	msg[1] = 0;
 
-	mutex_lock(&adc->lock);
+	guard(mutex)(&adc->lock);
 
 	ret = spi_write(adc->spi, msg, 2);
-	if (ret < 0) {
-		mutex_unlock(&adc->lock);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = spi_read(adc->spi, &adc->buffer, 2);
-
-	mutex_unlock(&adc->lock);
-
 	if (ret < 0)
 		return ret;
 
-- 
2.48.1


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

[PATCH 6/6] iio: adc: ti-adc128s052: Support ROHM BD79104

[Matti Vaittinen]

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

The ROHM BD79104 ADC has identical SPI communication logic as the
ti-adc128s052. Eg, SPI transfer should be 16 clk cycles, conversion is
started when the CS is pulled low, and channel selection is done by
writing the channel ID after two zero bits. Data is contained in
big-endian format in the last 12 bits.

The BD79104 has two input voltage pins. Data sheet uses terms "vdd" and
"iovdd". The "vdd" is used also as an analog reference voltage. Hence
the driver expects finding these from the device-tree, instead of having
the "vref" only as TI's driver.

NOTE: The TI's data sheet[1] does show that the TI's IC does actually
have two voltage inputs as well. Pins are called Va (analog reference)
and Vd (digital supply pin) - but I keep the existing driver behaviour
for the TI's IC "as is", because I have no HW to test changes, and
because I have no real need to touch it.

NOTE II: The BD79104 requires SPI MODE 3.

NOTE III: I used evaluation board "BD79104FV-EVK-001" made by ROHM. With
this board I had to drop the SPI speed below the 20M which is mentioned
in the data-sheet [2]. This, however, may be a limitation of the EVK
board, not the component itself.

[1]: https://www.ti.com/lit/ds/symlink/adc128s052.pdf

[2]:
https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79104fv-la-e.pdf

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

---
---
 drivers/iio/adc/Kconfig         |  2 +-
 drivers/iio/adc/ti-adc128s052.c | 40 +++++++++++++++++++++++++++++----
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 849c90203071..148a52b3db94 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1425,7 +1425,7 @@ config TI_ADC128S052
 	depends on SPI
 	help
 	  If you say yes here you get support for Texas Instruments ADC128S052,
-	  ADC122S021 and ADC124S021 chips.
+	  ADC122S021, ADC124S021 and ROHM Semiconductor BD79104 chips.
 
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-adc128s052.
diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index c68ee4e17a03..c7283d606d2e 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -21,6 +21,9 @@
 struct adc128_configuration {
 	const struct iio_chan_spec	*channels;
 	u8				num_channels;
+	const char			*refname;
+	int				num_other_regulators;
+	const char * const		(*other_regulators)[];
 };
 
 struct adc128 {
@@ -119,10 +122,28 @@ static const struct iio_chan_spec adc124s021_channels[] = {
 	ADC128_VOLTAGE_CHANNEL(3),
 };
 
+static const char * const bd79104_regulators[] = { "iovdd" };
+
 static const struct adc128_configuration adc128_config[] = {
-	{ adc128s052_channels, ARRAY_SIZE(adc128s052_channels) },
-	{ adc122s021_channels, ARRAY_SIZE(adc122s021_channels) },
-	{ adc124s021_channels, ARRAY_SIZE(adc124s021_channels) },
+	{
+		.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 iio_info adc128_info = {
@@ -157,7 +178,7 @@ static int adc128_probe(struct spi_device *spi)
 	indio_dev->channels = config->channels;
 	indio_dev->num_channels = config->num_channels;
 
-	adc->reg = devm_regulator_get(&spi->dev, "vref");
+	adc->reg = devm_regulator_get(&spi->dev, config->refname);
 	if (IS_ERR(adc->reg))
 		return PTR_ERR(adc->reg);
 
@@ -169,6 +190,15 @@ static int adc128_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	if (config->num_other_regulators) {
+		ret = devm_regulator_bulk_get_enable(&spi->dev,
+						config->num_other_regulators,
+						*config->other_regulators);
+		if (ret)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to enable regulators\n");
+	}
+
 	devm_mutex_init(&spi->dev, &adc->lock);
 
 	return devm_iio_device_register(&spi->dev, indio_dev);
@@ -182,6 +212,7 @@ static const struct of_device_id adc128_of_match[] = {
 	{ .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] },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, adc128_of_match);
@@ -194,6 +225,7 @@ static const struct spi_device_id adc128_id[] = {
 	{ "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] },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, adc128_id);
-- 
2.48.1


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

Re: [PATCH 2/6] iio: adc: ti-adc128s052: Fix ADC value on BE systems

[Jonathan Cameron]

On Mon, 31 Mar 2025 11:02:55 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> ADCs supported by the ti-adc128s052 driver do return the ADC data in 16
> bits using big-endian format. The driver does unconditionally swap the
> bytes. This leads to wrong values being reported to users on big endian
> systems.
> 
> Fix this by using the be16_to_cpu() instead of doing unconditional byte
> swapping.

It's not doing unconditional byte swap that I can see. The
adc->buffer[0] << 8 | adc->buffer[1]
will work on big or little endian systems as we are explicitly saying
which byte represents higher bit values in a 16 bit output so on little
endian it's a byte swap, but on big endian it's a noop (the compiler might
noticed that and replace this code sequence with an assignment)

Good cleanup, but not a fix as such unless I'm missing something.
> 
> Fixes: 913b86468674 ("iio: adc: Add TI ADC128S052")
> Cc: stable@vger.kernel.org
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> I have no big endian machines on my hands to test this. Problem was
> spotted by reading the code, which leaves some room for errors.
> Careful reviewing is appreciated!
> ---
>  drivers/iio/adc/ti-adc128s052.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index a456ea78462f..d1e31122ea0d 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -28,19 +28,20 @@ struct adc128 {
>  	struct regulator *reg;
>  	struct mutex lock;
>  
> -	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
> +	__be16 buffer __aligned(IIO_DMA_MINALIGN);
>  };
>  
>  static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
>  {
>  	int ret;
> +	char *msg = (char *)&adc->buffer;
>  
> -	mutex_lock(&adc->lock);
> +	msg[0] = channel << 3;
> +	msg[1] = 0;

Given you are writing shared state why move this out of the lock?
Whilst here maybe using guard() would clean this driver up a little.


Use a separate buffer (or a union) so we can avoid the casting here

>  
> -	adc->buffer[0] = channel << 3;
> -	adc->buffer[1] = 0;
> +	mutex_lock(&adc->lock);
>  
> -	ret = spi_write(adc->spi, &adc->buffer, 2);
> +	ret = spi_write(adc->spi, msg, 2);

Given you are tidying this up, lets make the source of that size value obvious.

sizeof(adc->buffer)

>  	if (ret < 0) {
>  		mutex_unlock(&adc->lock);
>  		return ret;
> @@ -53,7 +54,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
>  	if (ret < 0)
>  		return ret;
>  
> -	return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF);
> +	return be16_to_cpu(adc->buffer) & 0xFFF;
>  }
>  
>  static int adc128_read_raw(struct iio_dev *indio_dev,

Re: [PATCH 3/6] iio: adc: ti-adc128s052: Be consistent with arrays

[Jonathan Cameron]

On Mon, 31 Mar 2025 11:03:13 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The ti-adc128s052 driver has NULL terminated ID arrays for the
> of_device_id, spi_device_id and acpi_device_id. All of these are
> terminated by having an empty string as the last member of an array.
> Only the of_device_id array uses the /* sentinel */ comment.
> 
> It's better to be consistent.
> 
> This /* sentinel */ comment serves no real purpose these days as people
> are used to seeing these ID lists terminated with an empty array
> element.
> 
> Drop the /* sentinel */ from the of_device_id.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>  drivers/iio/adc/ti-adc128s052.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index d1e31122ea0d..dd1e405bf172 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -185,7 +185,7 @@ static const struct of_device_id adc128_of_match[] = {
>  	{ .compatible = "ti,adc124s021", .data = &adc128_config[2] },
>  	{ .compatible = "ti,adc124s051", .data = &adc128_config[2] },
>  	{ .compatible = "ti,adc124s101", .data = &adc128_config[2] },
> -	{ /* sentinel */ },
> +	{ },

Whilst here, drop the trailing ,

More 'modern' style in kernel tends not to use them as we don't want to make it
easy to put anything after it.

If there is another such case drop the comma on that as well as part of this
patch.

I don't care that much about the /* sentinel */ though as I've let at least
one more of those in over the weekend. Still consistency is a valid argument.


>  };
>  MODULE_DEVICE_TABLE(of, adc128_of_match);
>  

Re: [PATCH 4/6] iio: adc: ti-adc128s052: Use devm_mutex_init()

[Jonathan Cameron]

On Mon, 31 Mar 2025 11:03:30 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Quoting Jonathan:
> "Whilst it doesn't bring huge advantage, now we have devm_mutex_init()
> it seems reasonable to use it and maybe catch a use after free for the
> lock"
> 
> Switch to use devm_mutex_init() while working on this file.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>  drivers/iio/adc/ti-adc128s052.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index dd1e405bf172..90b23c68daea 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -172,7 +172,7 @@ static int adc128_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> -	mutex_init(&adc->lock);
> +	devm_mutex_init(&spi->dev, &adc->lock);
	ret = devm_mutex_init(&spi->dev, &adc->lock);
	if (ret)
		return ret;

>  
>  	return devm_iio_device_register(&spi->dev, indio_dev);
>  }

Re: [PATCH 5/6] iio: adc: ti-adc128s052: Simplify using guard(mutex)

[Jonathan Cameron]

On Mon, 31 Mar 2025 11:03:44 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Error path in ADC reading function can be slighly simplified using the
> guard(mutex). Do just that.
> 
> Also, document the mutex purpose while at it.
Ah. I should have read on before earlier comment!

That's what I get for an efficient linear pass of patches :)
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>  drivers/iio/adc/ti-adc128s052.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index 90b23c68daea..c68ee4e17a03 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -9,6 +9,7 @@
>   * https://www.ti.com/lit/ds/symlink/adc124s021.pdf
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/err.h>
>  #include <linux/iio/iio.h>
>  #include <linux/mod_devicetable.h>
> @@ -26,6 +27,7 @@ struct adc128 {
>  	struct spi_device *spi;
>  
>  	struct regulator *reg;
> +	/* Serialize the SPI 'write-channel + read data' accesses */
>  	struct mutex lock;
>  
>  	__be16 buffer __aligned(IIO_DMA_MINALIGN);
> @@ -39,18 +41,13 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
>  	msg[0] = channel << 3;
>  	msg[1] = 0;
>  
> -	mutex_lock(&adc->lock);
> +	guard(mutex)(&adc->lock);
As per earlier comment, I think you need to protect msg as well.

>  
>  	ret = spi_write(adc->spi, msg, 2);
> -	if (ret < 0) {
> -		mutex_unlock(&adc->lock);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	ret = spi_read(adc->spi, &adc->buffer, 2);
> -
> -	mutex_unlock(&adc->lock);
> -
>  	if (ret < 0)
>  		return ret;
>  

Re: [PATCH 6/6] iio: adc: ti-adc128s052: Support ROHM BD79104

[Jonathan Cameron]

On Mon, 31 Mar 2025 11:03:58 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The ROHM BD79104 ADC has identical SPI communication logic as the
> ti-adc128s052. Eg, SPI transfer should be 16 clk cycles, conversion is
> started when the CS is pulled low, and channel selection is done by
> writing the channel ID after two zero bits. Data is contained in
> big-endian format in the last 12 bits.

Nicely found match.  Sometimes these are tricky to spot.

> 
> The BD79104 has two input voltage pins. Data sheet uses terms "vdd" and
> "iovdd". The "vdd" is used also as an analog reference voltage. Hence
> the driver expects finding these from the device-tree, instead of having
> the "vref" only as TI's driver.
> 
> NOTE: The TI's data sheet[1] does show that the TI's IC does actually
> have two voltage inputs as well. Pins are called Va (analog reference)
> and Vd (digital supply pin) - but I keep the existing driver behaviour
> for the TI's IC "as is", because I have no HW to test changes, and
> because I have no real need to touch it.
> 
> NOTE II: The BD79104 requires SPI MODE 3.
> 
> NOTE III: I used evaluation board "BD79104FV-EVK-001" made by ROHM. With
> this board I had to drop the SPI speed below the 20M which is mentioned
> in the data-sheet [2]. This, however, may be a limitation of the EVK
> board, not the component itself.
> 
> [1]: https://www.ti.com/lit/ds/symlink/adc128s052.pdf
> 
> [2]:
> https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79104fv-la-e.pdf
> 
Prefer Datasheet tags with # [1] 
after them for the cross references.  

Those belong here in the tag block (no blank lines)
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

One request for an additional cleanup precursor patch given you are
touching the relevant code anyway.   It's a small one that you can
test so hope you don't mind doing that whilst here.

I'm relying on the incredibly small chance anyone has a variable
regulator wired up to the reference that they are modifying at runtime.
I have seen that done (once long ago on a crazy dev board for a really
noisy humidity sensor) when the reference was VDD but not on a separate
reference pin.  That means we almost certainly won't break the existing
parts and can't have a regression on your new one so we should be fine
to make the change.

Thanks,

Jonathan

> 
> ---
> ---
>  drivers/iio/adc/Kconfig         |  2 +-
>  drivers/iio/adc/ti-adc128s052.c | 40 +++++++++++++++++++++++++++++----
>  2 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 849c90203071..148a52b3db94 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1425,7 +1425,7 @@ config TI_ADC128S052
>  	depends on SPI
>  	help
>  	  If you say yes here you get support for Texas Instruments ADC128S052,
> -	  ADC122S021 and ADC124S021 chips.
> +	  ADC122S021, ADC124S021 and ROHM Semiconductor BD79104 chips.
>  
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-adc128s052.
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index c68ee4e17a03..c7283d606d2e 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -21,6 +21,9 @@
>  struct adc128_configuration {
>  	const struct iio_chan_spec	*channels;
>  	u8				num_channels;
> +	const char			*refname;
> +	int				num_other_regulators;
> +	const char * const		(*other_regulators)[];
>  };
>  
>  struct adc128 {
> @@ -119,10 +122,28 @@ static const struct iio_chan_spec adc124s021_channels[] = {
>  	ADC128_VOLTAGE_CHANNEL(3),
>  };
>  
> +static const char * const bd79104_regulators[] = { "iovdd" };
> +
>  static const struct adc128_configuration adc128_config[] = {
> -	{ adc128s052_channels, ARRAY_SIZE(adc128s052_channels) },
> -	{ adc122s021_channels, ARRAY_SIZE(adc122s021_channels) },
> -	{ adc124s021_channels, ARRAY_SIZE(adc124s021_channels) },
> +	{
> +		.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 iio_info adc128_info = {
> @@ -157,7 +178,7 @@ static int adc128_probe(struct spi_device *spi)
>  	indio_dev->channels = config->channels;
>  	indio_dev->num_channels = config->num_channels;
>  
> -	adc->reg = devm_regulator_get(&spi->dev, "vref");
> +	adc->reg = devm_regulator_get(&spi->dev, config->refname);

Hmm. It is very unusual for a reference regulator to be variable
after power up.  As such, maybe refactor the driver to use
devm_regulator_get_enable_read_voltage() which will save a few lines of
code and generally be easier to read.

That would need to be a separate precursor patch though.


>  	if (IS_ERR(adc->reg))
>  		return PTR_ERR(adc->reg);
>  
> @@ -169,6 +190,15 @@ static int adc128_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	if (config->num_other_regulators) {
> +		ret = devm_regulator_bulk_get_enable(&spi->dev,
> +						config->num_other_regulators,
> +						*config->other_regulators);
> +		if (ret)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to enable regulators\n");
> +	}
> +
>  	devm_mutex_init(&spi->dev, &adc->lock);
>  
>  	return devm_iio_device_register(&spi->dev, indio_dev);
> @@ -182,6 +212,7 @@ static const struct of_device_id adc128_of_match[] = {
>  	{ .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] },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, adc128_of_match);
> @@ -194,6 +225,7 @@ static const struct spi_device_id adc128_id[] = {
>  	{ "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] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, adc128_id);

Re: [PATCH 2/6] iio: adc: ti-adc128s052: Fix ADC value on BE systems

[Matti Vaittinen]

On 31/03/2025 14:11, Jonathan Cameron wrote:
> On Mon, 31 Mar 2025 11:02:55 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> ADCs supported by the ti-adc128s052 driver do return the ADC data in 16
>> bits using big-endian format. The driver does unconditionally swap the
>> bytes. This leads to wrong values being reported to users on big endian
>> systems.
>>
>> Fix this by using the be16_to_cpu() instead of doing unconditional byte
>> swapping.

Appears this was one of the patches I should've never written. Nothing 
went right :) Sorry for the noise. I'll try improving for the v2

> It's not doing unconditional byte swap that I can see. The
> adc->buffer[0] << 8 | adc->buffer[1]
> will work on big or little endian systems as we are explicitly saying
> which byte represents higher bit values in a 16 bit output so on little
> endian it's a byte swap, but on big endian it's a noop (the compiler might
> noticed that and replace this code sequence with an assignment)
> 
> Good cleanup, but not a fix as such unless I'm missing something.

No, you're not missing anything. I am the one who just got confused. I 
am not exactly sure what I was thinking. :rolleyes: This definitely 
isn't a fix. And, as a not a fix needing porting, I may squash this with 
some other patch. I need to take another look at this :)

>> Fixes: 913b86468674 ("iio: adc: Add TI ADC128S052")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>> I have no big endian machines on my hands to test this. Problem was
>> spotted by reading the code, which leaves some room for errors.
>> Careful reviewing is appreciated!
>> ---
>>   drivers/iio/adc/ti-adc128s052.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
>> index a456ea78462f..d1e31122ea0d 100644
>> --- a/drivers/iio/adc/ti-adc128s052.c
>> +++ b/drivers/iio/adc/ti-adc128s052.c
>> @@ -28,19 +28,20 @@ struct adc128 {
>>   	struct regulator *reg;
>>   	struct mutex lock;
>>   
>> -	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
>> +	__be16 buffer __aligned(IIO_DMA_MINALIGN);
>>   };
>>   
>>   static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
>>   {
>>   	int ret;
>> +	char *msg = (char *)&adc->buffer;
>>   
>> -	mutex_lock(&adc->lock);
>> +	msg[0] = channel << 3;
>> +	msg[1] = 0;
> 
> Given you are writing shared state why move this out of the lock?

Very Valid Point. I'm not 100% sure what I thought of, probably assumed 
IIO core would serialize the calls. That would've been nasty bug! I 
appreciate your sharp eyes :)

Thanks!

Yours,
	-- Matti

Re: [PATCH 1/6] dt-bindings: ROHM BD79104 ADC

[Conor Dooley]

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

On Mon, Mar 31, 2025 at 11:02:20AM +0300, Matti Vaittinen wrote:
> The ROHM BD79104 is a 12-bit, 8-channel ADC with two power supply pins,
> connected to SPI. It's worth noting the IC requires SPI MODE 3, (CPHA =
> 1, CPOL = 1).
> 
> I used an evaluation board "BD79104FV-EVK-001" from ROHM. With this
> board I had problems to have things working correctly with higher SPI
> clock frequencies. I didn't do thorough testing for maximum frequency
> though. First attempt was 40M, then 20M and finally 4M. With 20M it
> seemed as if the read values were shifted by 1 bit. With 4M it worked
> fine.
> 
> The component data-sheet is not exact what comes to the maximum SPI
> frequency. It says SPI frequency is 20M - "unless othervice specified".
> Additionally, it says that maximum sampling rate is 1Mhz, and since
> reading a sample requires writing the channel (16 bits) and reading
> data (16 bits) - we get some upper limit from this.
> 
> From the "frequency is 20M, unless othervice specified" I picked the
> maximum frequency 20M - and did assumption that my problems with 20M
> weren't related to the BD79104 - but to the evaluation board
> "BD79104FV-EVK-001".
> 
> Add bindings for the ROHM BD79104 ADC.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

> ---
>  .../bindings/iio/adc/rohm,bd79104.yaml        | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml b/Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml
> new file mode 100644
> index 000000000000..2a8ad4fdfc6b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/rohm,bd79104.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ROHM Semiconductor BD79104 ADC
> +
> +maintainers:
> +  - Matti Vaittinen <mazziesaccount@gmail.com>
> +
> +description: |
> +  12 bit SPI ADC with 8 channels.
> +
> +properties:
> +  compatible:
> +    const: rohm,bd79104
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +  iovdd-supply: true
> +
> +# The component data-sheet says the frequency is 20M. I, however, found
> +# that the ROHM evaluation board BD79104FV-EVK-001 had problems with 20M.
> +# I have successfully used it with 4M. My _assumption_ is that this is not
> +# the limitation of the component itself, but a limitation of the EVK.
> +  spi-max-frequency:
> +    maximum: 20000000
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  spi-cpha: true
> +  spi-cpol: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +  - iovdd-supply
> +  - spi-cpha
> +  - spi-cpol
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +            compatible = "rohm,bd79104";
> +            reg = <0>;
> +            vdd-supply = <&vdd_supply>;
> +            iovdd-supply = <&iovdd_supply>;
> +            spi-max-frequency = <4000000>;
> +            spi-cpha;
> +            spi-cpol;
> +            #io-channel-cells = <1>;
> +        };
> +    };
> +...
> -- 
> 2.48.1
> 



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

Re: [PATCH 6/6] iio: adc: ti-adc128s052: Support ROHM BD79104

[Matti Vaittinen]

On 31/03/2025 14:22, Jonathan Cameron wrote:
> On Mon, 31 Mar 2025 11:03:58 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The ROHM BD79104 ADC has identical SPI communication logic as the
>> ti-adc128s052. Eg, SPI transfer should be 16 clk cycles, conversion is
>> started when the CS is pulled low, and channel selection is done by
>> writing the channel ID after two zero bits. Data is contained in
>> big-endian format in the last 12 bits.
> 
> Nicely found match.  Sometimes these are tricky to spot.
> 
>>
>> The BD79104 has two input voltage pins. Data sheet uses terms "vdd" and
>> "iovdd". The "vdd" is used also as an analog reference voltage. Hence
>> the driver expects finding these from the device-tree, instead of having
>> the "vref" only as TI's driver.
>>
>> NOTE: The TI's data sheet[1] does show that the TI's IC does actually
>> have two voltage inputs as well. Pins are called Va (analog reference)
>> and Vd (digital supply pin) - but I keep the existing driver behaviour
>> for the TI's IC "as is", because I have no HW to test changes, and
>> because I have no real need to touch it.
>>
>> NOTE II: The BD79104 requires SPI MODE 3.
>>
>> NOTE III: I used evaluation board "BD79104FV-EVK-001" made by ROHM. With
>> this board I had to drop the SPI speed below the 20M which is mentioned
>> in the data-sheet [2]. This, however, may be a limitation of the EVK
>> board, not the component itself.
>>
>> [1]: https://www.ti.com/lit/ds/symlink/adc128s052.pdf
>>
>> [2]:
>> https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79104fv-la-e.pdf
>>
> Prefer Datasheet tags with # [1]
> after them for the cross references.
> 
> Those belong here in the tag block (no blank lines)
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> One request for an additional cleanup precursor patch given you are
> touching the relevant code anyway.   It's a small one that you can
> test so hope you don't mind doing that whilst here.
> 
> I'm relying on the incredibly small chance anyone has a variable
> regulator wired up to the reference that they are modifying at runtime.
> I have seen that done (once long ago on a crazy dev board for a really
> noisy humidity sensor) when the reference was VDD but not on a separate
> reference pin.  That means we almost certainly won't break the existing
> parts and can't have a regression on your new one so we should be fine
> to make the change.

The change you ask for is indeed small. I have no real objections 
against implementing it (and I actually wrote it already) - but I am 
still somewhat hesitant. As you say, (it seems like) the idea of the 
original code is to allow changing the vref at runtime. It looks to me 
this might've been intentional choice. I am not terribly happy about 
dropping the working functionality, when the gained simplification isn't 
particularly massive.

Because of this, I am thinking of adding the patch dropping the 
functionality as an RFC. Leaving that floating on the list for a while 
would at least have my ass partially covered ;)

I'd rather not delayed the support for the BD79104 though. So - would it 
be okay if I didn't implement the clean-up as a precursory patch, but 
did it as a last patch of the series? That will make it a tad more 
complex to review, but it'd allow taking the BD79104 changes in while 
leaving the RFC to float on a list. (Also, I'm not sure if you can push 
an RFC in next without taking it in for the cycle?)

Yours,
	-- Matti

Re: [PATCH 6/6] iio: adc: ti-adc128s052: Support ROHM BD79104

[Jonathan Cameron]

On Tue, 1 Apr 2025 15:33:15 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 31/03/2025 14:22, Jonathan Cameron wrote:
> > On Mon, 31 Mar 2025 11:03:58 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> The ROHM BD79104 ADC has identical SPI communication logic as the
> >> ti-adc128s052. Eg, SPI transfer should be 16 clk cycles, conversion is
> >> started when the CS is pulled low, and channel selection is done by
> >> writing the channel ID after two zero bits. Data is contained in
> >> big-endian format in the last 12 bits.  
> > 
> > Nicely found match.  Sometimes these are tricky to spot.
> >   
> >>
> >> The BD79104 has two input voltage pins. Data sheet uses terms "vdd" and
> >> "iovdd". The "vdd" is used also as an analog reference voltage. Hence
> >> the driver expects finding these from the device-tree, instead of having
> >> the "vref" only as TI's driver.
> >>
> >> NOTE: The TI's data sheet[1] does show that the TI's IC does actually
> >> have two voltage inputs as well. Pins are called Va (analog reference)
> >> and Vd (digital supply pin) - but I keep the existing driver behaviour
> >> for the TI's IC "as is", because I have no HW to test changes, and
> >> because I have no real need to touch it.
> >>
> >> NOTE II: The BD79104 requires SPI MODE 3.
> >>
> >> NOTE III: I used evaluation board "BD79104FV-EVK-001" made by ROHM. With
> >> this board I had to drop the SPI speed below the 20M which is mentioned
> >> in the data-sheet [2]. This, however, may be a limitation of the EVK
> >> board, not the component itself.
> >>
> >> [1]: https://www.ti.com/lit/ds/symlink/adc128s052.pdf
> >>
> >> [2]:
> >> https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79104fv-la-e.pdf
> >>  
> > Prefer Datasheet tags with # [1]
> > after them for the cross references.
> > 
> > Those belong here in the tag block (no blank lines)  
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> > 
> > One request for an additional cleanup precursor patch given you are
> > touching the relevant code anyway.   It's a small one that you can
> > test so hope you don't mind doing that whilst here.
> > 
> > I'm relying on the incredibly small chance anyone has a variable
> > regulator wired up to the reference that they are modifying at runtime.
> > I have seen that done (once long ago on a crazy dev board for a really
> > noisy humidity sensor) when the reference was VDD but not on a separate
> > reference pin.  That means we almost certainly won't break the existing
> > parts and can't have a regression on your new one so we should be fine
> > to make the change.  
> 
> The change you ask for is indeed small. I have no real objections 
> against implementing it (and I actually wrote it already) - but I am 
> still somewhat hesitant. As you say, (it seems like) the idea of the 
> original code is to allow changing the vref at runtime. It looks to me 
> this might've been intentional choice. I am not terribly happy about 
> dropping the working functionality, when the gained simplification isn't 
> particularly massive.

Hmm. I suspect this was added at my request (or copied from where I requested
it)  Back when we did this there was no advantage in doing it at probe
as it was just a question of store a value or store a pointer we had
to get anyway.  So I tended to advocate what I now think was a bit silly,
that someone elses board might have it changing...

User space wise, what code checks for random scaling changes?  So it
was best effort at best anyway!
 
> 
> Because of this, I am thinking of adding the patch dropping the 
> functionality as an RFC. Leaving that floating on the list for a while 
> would at least have my ass partially covered ;)
> 
> I'd rather not delayed the support for the BD79104 though. So - would it 
> be okay if I didn't implement the clean-up as a precursory patch, but 
> did it as a last patch of the series? That will make it a tad more 
> complex to review, but it'd allow taking the BD79104 changes in while 
> leaving the RFC to float on a list. (Also, I'm not sure if you can push 
> an RFC in next without taking it in for the cycle?)

I'll probably just merge it even as an RFC :)  That way it's my
fault if we break someone and they shout!

Jonathan

> 
> Yours,
> 	-- Matti
> 

Re: [PATCH 6/6] iio: adc: ti-adc128s052: Support ROHM BD79104

[Matti Vaittinen]

On 05/04/2025 20:43, Jonathan Cameron wrote:
> On Tue, 1 Apr 2025 15:33:15 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 31/03/2025 14:22, Jonathan Cameron wrote:
>>> On Mon, 31 Mar 2025 11:03:58 +0300
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> The ROHM BD79104 ADC has identical SPI communication logic as the
>>>> ti-adc128s052. Eg, SPI transfer should be 16 clk cycles, conversion is
>>>> started when the CS is pulled low, and channel selection is done by
>>>> writing the channel ID after two zero bits. Data is contained in
>>>> big-endian format in the last 12 bits.
>>>
>>> Nicely found match.  Sometimes these are tricky to spot.
>>>    
>>>>
>>>> The BD79104 has two input voltage pins. Data sheet uses terms "vdd" and
>>>> "iovdd". The "vdd" is used also as an analog reference voltage. Hence
>>>> the driver expects finding these from the device-tree, instead of having
>>>> the "vref" only as TI's driver.
>>>>
>>>> NOTE: The TI's data sheet[1] does show that the TI's IC does actually
>>>> have two voltage inputs as well. Pins are called Va (analog reference)
>>>> and Vd (digital supply pin) - but I keep the existing driver behaviour
>>>> for the TI's IC "as is", because I have no HW to test changes, and
>>>> because I have no real need to touch it.
>>>>
>>>> NOTE II: The BD79104 requires SPI MODE 3.
>>>>
>>>> NOTE III: I used evaluation board "BD79104FV-EVK-001" made by ROHM. With
>>>> this board I had to drop the SPI speed below the 20M which is mentioned
>>>> in the data-sheet [2]. This, however, may be a limitation of the EVK
>>>> board, not the component itself.
>>>>
>>>> [1]: https://www.ti.com/lit/ds/symlink/adc128s052.pdf
>>>>
>>>> [2]:
>>>> https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79104fv-la-e.pdf
>>>>   
>>> Prefer Datasheet tags with # [1]
>>> after them for the cross references.
>>>
>>> Those belong here in the tag block (no blank lines)
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>
>>> One request for an additional cleanup precursor patch given you are
>>> touching the relevant code anyway.   It's a small one that you can
>>> test so hope you don't mind doing that whilst here.
>>>
>>> I'm relying on the incredibly small chance anyone has a variable
>>> regulator wired up to the reference that they are modifying at runtime.
>>> I have seen that done (once long ago on a crazy dev board for a really
>>> noisy humidity sensor) when the reference was VDD but not on a separate
>>> reference pin.  That means we almost certainly won't break the existing
>>> parts and can't have a regression on your new one so we should be fine
>>> to make the change.
>>
>> The change you ask for is indeed small. I have no real objections
>> against implementing it (and I actually wrote it already) - but I am
>> still somewhat hesitant. As you say, (it seems like) the idea of the
>> original code is to allow changing the vref at runtime. It looks to me
>> this might've been intentional choice. I am not terribly happy about
>> dropping the working functionality, when the gained simplification isn't
>> particularly massive.
> 
> Hmm. I suspect this was added at my request (or copied from where I requested
> it)  Back when we did this there was no advantage in doing it at probe
> as it was just a question of store a value or store a pointer we had
> to get anyway.  So I tended to advocate what I now think was a bit silly,
> that someone elses board might have it changing...
> 
> User space wise, what code checks for random scaling changes?  So it
> was best effort at best anyway!

Ah, right. I suppose this should've been accompanied with scale setting 
which could've changed the regulator voltage - and I have no idea if 
such hardware would make any sense.

The slim chance I can imagine is that the reference voltage can't be 
set/known at probe time.

>> Because of this, I am thinking of adding the patch dropping the
>> functionality as an RFC. Leaving that floating on the list for a while
>> would at least have my ass partially covered ;)
>>
>> I'd rather not delayed the support for the BD79104 though. So - would it
>> be okay if I didn't implement the clean-up as a precursory patch, but
>> did it as a last patch of the series? That will make it a tad more
>> complex to review, but it'd allow taking the BD79104 changes in while
>> leaving the RFC to float on a list. (Also, I'm not sure if you can push
>> an RFC in next without taking it in for the cycle?)
> 
> I'll probably just merge it even as an RFC :)  That way it's my
> fault if we break someone and they shout!

That's fine for me. Well, doing it this way around (as a last patch) 
should ease reverting, should it be needed.

Yours
	-- Matti

Re: [PATCH 6/6] iio: adc: ti-adc128s052: Support ROHM BD79104

[Jonathan Cameron]

On Mon, 7 Apr 2025 09:10:05 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 05/04/2025 20:43, Jonathan Cameron wrote:
> > On Tue, 1 Apr 2025 15:33:15 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> On 31/03/2025 14:22, Jonathan Cameron wrote:  
> >>> On Mon, 31 Mar 2025 11:03:58 +0300
> >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >>>      
> >>>> The ROHM BD79104 ADC has identical SPI communication logic as the
> >>>> ti-adc128s052. Eg, SPI transfer should be 16 clk cycles, conversion is
> >>>> started when the CS is pulled low, and channel selection is done by
> >>>> writing the channel ID after two zero bits. Data is contained in
> >>>> big-endian format in the last 12 bits.  
> >>>
> >>> Nicely found match.  Sometimes these are tricky to spot.
> >>>      
> >>>>
> >>>> The BD79104 has two input voltage pins. Data sheet uses terms "vdd" and
> >>>> "iovdd". The "vdd" is used also as an analog reference voltage. Hence
> >>>> the driver expects finding these from the device-tree, instead of having
> >>>> the "vref" only as TI's driver.
> >>>>
> >>>> NOTE: The TI's data sheet[1] does show that the TI's IC does actually
> >>>> have two voltage inputs as well. Pins are called Va (analog reference)
> >>>> and Vd (digital supply pin) - but I keep the existing driver behaviour
> >>>> for the TI's IC "as is", because I have no HW to test changes, and
> >>>> because I have no real need to touch it.
> >>>>
> >>>> NOTE II: The BD79104 requires SPI MODE 3.
> >>>>
> >>>> NOTE III: I used evaluation board "BD79104FV-EVK-001" made by ROHM. With
> >>>> this board I had to drop the SPI speed below the 20M which is mentioned
> >>>> in the data-sheet [2]. This, however, may be a limitation of the EVK
> >>>> board, not the component itself.
> >>>>
> >>>> [1]: https://www.ti.com/lit/ds/symlink/adc128s052.pdf
> >>>>
> >>>> [2]:
> >>>> https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79104fv-la-e.pdf
> >>>>     
> >>> Prefer Datasheet tags with # [1]
> >>> after them for the cross references.
> >>>
> >>> Those belong here in the tag block (no blank lines)  
> >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>  
> >>>
> >>> One request for an additional cleanup precursor patch given you are
> >>> touching the relevant code anyway.   It's a small one that you can
> >>> test so hope you don't mind doing that whilst here.
> >>>
> >>> I'm relying on the incredibly small chance anyone has a variable
> >>> regulator wired up to the reference that they are modifying at runtime.
> >>> I have seen that done (once long ago on a crazy dev board for a really
> >>> noisy humidity sensor) when the reference was VDD but not on a separate
> >>> reference pin.  That means we almost certainly won't break the existing
> >>> parts and can't have a regression on your new one so we should be fine
> >>> to make the change.  
> >>
> >> The change you ask for is indeed small. I have no real objections
> >> against implementing it (and I actually wrote it already) - but I am
> >> still somewhat hesitant. As you say, (it seems like) the idea of the
> >> original code is to allow changing the vref at runtime. It looks to me
> >> this might've been intentional choice. I am not terribly happy about
> >> dropping the working functionality, when the gained simplification isn't
> >> particularly massive.  
> > 
> > Hmm. I suspect this was added at my request (or copied from where I requested
> > it)  Back when we did this there was no advantage in doing it at probe
> > as it was just a question of store a value or store a pointer we had
> > to get anyway.  So I tended to advocate what I now think was a bit silly,
> > that someone elses board might have it changing...
> > 
> > User space wise, what code checks for random scaling changes?  So it
> > was best effort at best anyway!  
> 
> Ah, right. I suppose this should've been accompanied with scale setting 
> which could've changed the regulator voltage - and I have no idea if 
> such hardware would make any sense.

In theory possible but I suspect expensive as a controllable precision
reference would be needed (a DAC probably wouldn't have enough current?)

> 
> The slim chance I can imagine is that the reference voltage can't be 
> set/known at probe time.

Agreed. It can in theory happen and did on one ancient board I had
where the reference voltage was wired to a pair of devices and one
had a higher minimum voltage than the other. That was pre DT times though
and I suspect now we'd just put the voltage that works for both in DT.

> 
> >> Because of this, I am thinking of adding the patch dropping the
> >> functionality as an RFC. Leaving that floating on the list for a while
> >> would at least have my ass partially covered ;)
> >>
> >> I'd rather not delayed the support for the BD79104 though. So - would it
> >> be okay if I didn't implement the clean-up as a precursory patch, but
> >> did it as a last patch of the series? That will make it a tad more
> >> complex to review, but it'd allow taking the BD79104 changes in while
> >> leaving the RFC to float on a list. (Also, I'm not sure if you can push
> >> an RFC in next without taking it in for the cycle?)  
> > 
> > I'll probably just merge it even as an RFC :)  That way it's my
> > fault if we break someone and they shout!  
> 
> That's fine for me. Well, doing it this way around (as a last patch) 
> should ease reverting, should it be needed.
Absolutely.

> 
> Yours
> 	-- Matti