Support ROHM BD79104 ADC

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.

I added myself as a maintainer for the driver, so I can stay on track of the changes to it. If anyone with experience with (and acces to) the TI's ICs has the energy ... feel free to add more maintainers.

This series was based on the v6.15-rc1

Revision history:

v2 = v3:
- Rebase on v6.15-rc1
- Minor improvements to the Vref handling
- Add MAINTAINERS entry
v1 => v2:
- Drop the claim that original driver is broken for BE and rename the patch 0001 accordingly
- Fix race when filling the SPI message
- Check return value for the devm_mutex_init()
- Add a RFC patch for dropping the support for variable Vref.

---

Matti Vaittinen (8):
dt-bindings: ROHM BD79104 ADC
iio: adc: ti-adc128s052: Simplify using be16_to_cpu()
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
MAINTAINERS: A driver for TI/ROHM ADCs
iio: ti-adc128s052: Drop variable vref

 .../bindings/iio/adc/rohm,bd79104.yaml        | 69 +++++++++++++
 MAINTAINERS                                   |  6 ++
 drivers/iio/adc/Kconfig                       |  2 +-
 drivers/iio/adc/ti-adc128s052.c               | 96 ++++++++++++-------
 4 files changed, 135 insertions(+), 38 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml

2.49.0

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.

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

---
 .../bindings/iio/adc/rohm,bd79104.yaml        | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml

new file mode 100644
index 000000000000..2a8ad4fdfc6b
@@ -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 <[email protected]>
+
+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.49.0

The register data is 12-bit big-endian data. Use be16_to_cpu() to do the conversion, and simple bitwise AND for masking to make it more obvious.

---
Revision history:
v2 => v3:
- Decrease amount of changed lines by keeping the old buffer name.
v1 => v2:
- Fix commit msg to reflect the fact there was no bug
- Drop Fixes tag
- Use union for rx / tx buffer to avoid casting
- Keep the shared message protected by the mutex

---
 drivers/iio/adc/ti-adc128s052.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

index a456ea78462f..c5b2374322e4 100644
@@ -28,7 +28,10 @@ struct adc128 {
 	struct regulator *reg;
 	struct mutex lock;
 
-	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
+	union {
+		__be16 buffer16;
+		u8 buffer[2];
+	} __aligned(IIO_DMA_MINALIGN);
 };
 
 static int adc128_adc_conversion(struct adc128 *adc, u8 channel)

@@ -40,20 +43,18 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
 	adc->buffer[0] = channel << 3;
 	adc->buffer[1] = 0;
 
-	ret = spi_write(adc->spi, &adc->buffer, 2);
+	ret = spi_write(adc->spi, &adc->buffer, sizeof(adc->buffer));
 	if (ret < 0) {
 		mutex_unlock(&adc->lock);
 		return ret;
 	}
 
-	ret = spi_read(adc->spi, &adc->buffer, 2);
-
+	ret = spi_read(adc->spi, &adc->buffer16, sizeof(adc->buffer16));
 	mutex_unlock(&adc->lock);
-
 	if (ret < 0)
 		return ret;
 
-	return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF);
+	return be16_to_cpu(adc->buffer16) & 0xFFF;
 }
 
 static int adc128_read_raw(struct iio_dev *indio_dev,

2.49.0

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.

---
Revision history:
v2 =>
- No changes
v1 => v2:
- Drop the comma from the end of the of_device_id list.

---
 drivers/iio/adc/ti-adc128s052.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c5b2374322e4..d0702d403fbe 100644
@@ -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.49.0

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.

---
v2 =>
- No changes
v1 => v2:
- Check the return value for the devm_mutex_init()

---
 drivers/iio/adc/ti-adc128s052.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index d0702d403fbe..d90a5caa028f 100644
@@ -172,7 +172,9 @@ static int adc128_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	mutex_init(&adc->lock);
+	ret = devm_mutex_init(&spi->dev, &adc->lock);
+	if (ret)
+		return ret;
 
 	return devm_iio_device_register(&spi->dev, indio_dev);
 }

2.49.0

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

Use guard(mutex) and document the mutex purpose.

---
Revision history:
v2 =>
- No changes
v1 => v2:
- Protect the shared message data
- Add message data protection to the mutex doc
- Reword the commit message

---
 drivers/iio/adc/ti-adc128s052.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

index d90a5caa028f..fa0099356be7 100644
@@ -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,10 @@ struct adc128 {
 	struct spi_device *spi;
 
 	struct regulator *reg;
+	/*
+	 * Serialize the SPI 'write-channel + read data' accesses and protect
+	 * the shared buffer.
+	 */
 	struct mutex lock;
 
 	union {

@@ -38,19 +43,16 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
 {
 	int ret;
 
-	mutex_lock(&adc->lock);
+	guard(mutex)(&adc->lock);
 
 	adc->buffer[0] = channel << 3;
 	adc->buffer[1] = 0;
 
 	ret = spi_write(adc->spi, &adc->buffer, sizeof(adc->buffer));
-	if (ret < 0) {
-		mutex_unlock(&adc->lock);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = spi_read(adc->spi, &adc->buffer16, sizeof(adc->buffer16));
-	mutex_unlock(&adc->lock);
 	if (ret < 0)
 		return ret;
 

2.49.0

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.

Datasheet: https://www.ti.com/lit/ds/symlink/adc128s052.pdf # [1]
Datasheet: https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79104fv-la-e.pdf # [2]

---
Revision history:
v2 =>
- No changes
v1 => v2:
- Use Datasheet tags in commit message.

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

index 6529df1a498c..bbad7b3d41bf 100644
@@ -1469,7 +1469,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.

index fa0099356be7..d4721ad90f2c 100644
@@ -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 {

@@ -124,10 +127,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 = {

@@ -162,7 +183,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);
 

@@ -174,6 +195,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");
+	}
+
 	ret = devm_mutex_init(&spi->dev, &adc->lock);
 	if (ret)
 		return ret;

@@ -189,6 +219,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);

@@ -201,6 +232,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.49.0

Add undersigned as a maintainer for the ti-adc128s052.c which supports a few TI's ADCs and the ROHM Semiconductor BD79704 ADC.

---
Revision history:
v2 => v3:
- New patch

I only have access to the ROHM BD79704. Maintainers with access to the TIs ICs (and time, energy and the knowledge) would be welcome :)

---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

index 96b827049501..bb705fac9279 100644
@@ -24088,6 +24088,12 @@ M:	Robert Richter <[email protected]>
 S:	Odd Fixes
 F:	drivers/gpio/gpio-thunderx.c
 
+TI ADC12xs and ROHM BD79104 ADC driver
+M:	Matti Vaittinen <[email protected]>
+S:	Maintained
+F:	drivers/iio/adc/ti-adc128s052.c
+L:	[email protected]
+
 TI ADS1119 ADC DRIVER
 M:	Francesco Dolcini <[email protected]>
 M:	João Paulo Gonçalves <[email protected]>

2.49.0

According to Jonathan, variable reference voltages are very rare. It is unlikely it is needed, and supporting it makes the code a bit more complex.

Simplify the driver and drop the variable vref support.

---
Revision History:
v2 => v3:
- Rename vref => vref_mv to make units visible
- Divide vref once in the probe to avoid division every time the scale is requested
v2:
- New patch

---
 drivers/iio/adc/ti-adc128s052.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

index d4721ad90f2c..c38468f299ce 100644
@@ -29,13 +29,12 @@ struct adc128_configuration {
 struct adc128 {
 	struct spi_device *spi;
 
-	struct regulator *reg;
 	/*
 	 * Serialize the SPI 'write-channel + read data' accesses and protect
 	 * the shared buffer.
 	 */
 	struct mutex lock;
-
+	int vref_mv;
 	union {
 		__be16 buffer16;
 		u8 buffer[2];

@@ -81,11 +80,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
 
 	case IIO_CHAN_INFO_SCALE:
 
-		ret = regulator_get_voltage(adc->reg);
-		if (ret < 0)
-			return ret;
-
-		*val = ret / 1000;
+		*val = adc->vref_mv;
 		*val2 = 12;
 		return IIO_VAL_FRACTIONAL_LOG2;
 

@@ -155,11 +150,6 @@ static const struct iio_info adc128_info = {
 	.read_raw = adc128_read_raw,
 };
 
-static void adc128_disable_regulator(void *reg)
-{
-	regulator_disable(reg);
-}
-
 static int adc128_probe(struct spi_device *spi)
 {
 	const struct adc128_configuration *config;

@@ -183,17 +173,12 @@ 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, config->refname);
-	if (IS_ERR(adc->reg))
-		return PTR_ERR(adc->reg);
+	adc->vref_mv = devm_regulator_get_enable_read_voltage(&spi->dev,
+							   config->refname);
+	if (adc->vref_mv < 0)
+		return adc->vref_mv;
 
-	ret = regulator_enable(adc->reg);
-	if (ret < 0)
-		return ret;
-	ret = devm_add_action_or_reset(&spi->dev, adc128_disable_regulator,
-				       adc->reg);
-	if (ret)
-		return ret;
+	adc->vref_mv /= 1000;
 
 	if (config->num_other_regulators) {
 		ret = devm_regulator_bulk_get_enable(&spi->dev,

2.49.0

...

	return dev_err_probe(dev, ret, "failed to read '%s' voltage, ...);

Good idea.
Applied patches 1-7. I did tweak this one as well but then couldn't make up my mind on whether to change the type of vref_mv so I'll avoid making a decision and will leave patch 8 for a v4 from Matti :)

Jonathan

Thanks for handling the 1-7 :) I'll re-spin this, but it's likely to take some time. I'm having 2 weeks off from work, and my motorbike and boat is requiring some attention ;)

I hope I have regained a lot of energy after 2 weeks ;)

Yours,
-- Matti

Good idea. Thanks.

I'm not convinced about the benefits. The "temporary holding" is not really an issue as it is only held unmodified for the duration of the error check. Furthermore, converting the voltage unsigned does not add much as the regulator framework does any way return the voltage as integer.

Still, even if I am not convinced about the benefits, I don't really see any downsides in your suggestions either :)

Yours,
-- Matti