[PATCH v2 0/7] Support ROHM BD79104 ADC

[PATCH v2 0/7] Support ROHM BD79104 ADC

[Matti Vaittinen]

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

Revision history:

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 (7):
  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
  iio: ti-adc128s052: Drop variable vref

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


base-commit: 543da6252b48bbfe13e194fb980bdd2b161f6838
-- 
2.49.0


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

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3417 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>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

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

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


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

[PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()

[Matti Vaittinen]

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

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.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
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 | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

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


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

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

[Matti Vaittinen]

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

---
Revision history:
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(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index 3e69a5fce010..861a35169196 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -186,7 +186,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


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

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

[Matti Vaittinen]

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

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

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index 861a35169196..bef2d29c06af 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -173,7 +173,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


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

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

[Matti Vaittinen]

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

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

Use guard(mutex) and document the mutex purpose.

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

---
Revision history:
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(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index bef2d29c06af..a15b75fc067e 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,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 {
@@ -39,19 +44,16 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
 	int ret;
 	char *msg = &adc->tx_buffer[0];
 
-	mutex_lock(&adc->lock);
+	guard(mutex)(&adc->lock);
 
 	msg[0] = channel << 3;
 	msg[1] = 0;
 
 	ret = spi_write(adc->spi, msg, sizeof(adc->tx_buffer));
-	if (ret < 0) {
-		mutex_unlock(&adc->lock);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = spi_read(adc->spi, &adc->rx_buffer, 2);
-	mutex_unlock(&adc->lock);
 	if (ret < 0)
 		return ret;
 
-- 
2.49.0


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

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

[Matti Vaittinen]

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

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

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 859c77f40f1d..045cd696a57d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1486,7 +1486,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 a15b75fc067e..0f93c6266527 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 {
@@ -125,10 +128,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 = {
@@ -163,7 +184,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);
 
@@ -175,6 +196,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;
@@ -190,6 +220,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);
@@ -202,6 +233,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


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

[PATCH RFC v2 7/7] iio: ti-adc128s052: Drop variable vref

[Matti Vaittinen]

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

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.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision History:
 v2:
  - New patch
---
 drivers/iio/adc/ti-adc128s052.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index 0f93c6266527..0bfe4e558c69 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -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;
 	union {
 		__be16 rx_buffer;
 		u8 tx_buffer[2];
@@ -82,11 +81,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 / 1000;
 		*val2 = 12;
 		return IIO_VAL_FRACTIONAL_LOG2;
 
@@ -156,11 +151,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;
@@ -184,17 +174,10 @@ 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);
-
-	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 = devm_regulator_get_enable_read_voltage(&spi->dev,
+							   config->refname);
+	if (adc->vref < 0)
+		return adc->vref;
 
 	if (config->num_other_regulators) {
 		ret = devm_regulator_bulk_get_enable(&spi->dev,
-- 
2.49.0


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

Re: [PATCH v2 0/7] Support ROHM BD79104 ADC

[Matti Vaittinen]

On 02/04/2025 09:07, Matti Vaittinen wrote:

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

Jonathan, I suppose this, by default,  falls under the umbrella of your 
IIO maintainership. Are you Okay with it if I'll add a maintainer entry 
which sets you explicitly as a maintainer for this, and list myself as a 
reviewer?

I suppose I could also add myself as a maintainer for this, but I am 
unsure how well it would be received by the TI people ;)

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

Just realized I forgot to update this. The series is now based on 
543da6252b48 in the iio/testing. Nonetheless, I can still rebase if needed.

Yours,
	-- Matti

Re: [PATCH RFC v2 7/7] iio: ti-adc128s052: Drop variable vref

[David Lechner]

On 4/2/25 1:10 AM, Matti Vaittinen wrote:
> 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.

There is also around 60 other drivers where we could do something like this
in case anyone is bored. :-p

> 
> Simplify the driver and drop the variable vref support.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> Revision History:
>  v2:
>   - New patch
> ---
>  drivers/iio/adc/ti-adc128s052.c | 29 ++++++-----------------------
>  1 file changed, 6 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index 0f93c6266527..0bfe4e558c69 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -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;

Units in the name are helpful: vref_uv.

Could also consider doing division in probe and storing vref_mv instead
since we never use the microvolts part.

>  	union {
>  		__be16 rx_buffer;
>  		u8 tx_buffer[2];

Re: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()

[David Lechner]

On 4/2/25 1:09 AM, Matti Vaittinen wrote:
> 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.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> Revision history:
> 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 | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index a456ea78462f..3e69a5fce010 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -28,32 +28,34 @@ struct adc128 {
>  	struct regulator *reg;
>  	struct mutex lock;
>  
> -	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
> +	union {
> +		__be16 rx_buffer;
> +		u8 tx_buffer[2];
> +	} __aligned(IIO_DMA_MINALIGN);
>  };
>  
>  static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
>  {
>  	int ret;
> +	char *msg = &adc->tx_buffer[0];
>  
>  	mutex_lock(&adc->lock);
>  
> -	adc->buffer[0] = channel << 3;
> -	adc->buffer[1] = 0;
> +	msg[0] = channel << 3;
> +	msg[1] = 0;
>  
> -	ret = spi_write(adc->spi, &adc->buffer, 2);
> +	ret = spi_write(adc->spi, msg, sizeof(adc->tx_buffer));
>  	if (ret < 0) {
>  		mutex_unlock(&adc->lock);
>  		return ret;
>  	}
>  
> -	ret = spi_read(adc->spi, &adc->buffer, 2);
> -
> +	ret = spi_read(adc->spi, &adc->rx_buffer, 2);
>  	mutex_unlock(&adc->lock);
> -
>  	if (ret < 0)
>  		return ret;
>  
> -	return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF);
> +	return be16_to_cpu(adc->rx_buffer) & 0xFFF;


The cast isn't exactly beautiful, but this would save a lot of
lines of diff and a few lines of code by avoiding the need for
the union and the local msg variable.

	return be16_to_cpup((__be16 *)adc->buffer) & 0xFFF;

>  }
>  
>  static int adc128_read_raw(struct iio_dev *indio_dev,

Re: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()

[Matti Vaittinen]

On 03/04/2025 00:04, David Lechner wrote:
> On 4/2/25 1:09 AM, Matti Vaittinen wrote:
>> 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.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>> Revision history:
>> 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 | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
>> index a456ea78462f..3e69a5fce010 100644
>> --- a/drivers/iio/adc/ti-adc128s052.c
>> +++ b/drivers/iio/adc/ti-adc128s052.c
>> @@ -28,32 +28,34 @@ struct adc128 {
>>   	struct regulator *reg;
>>   	struct mutex lock;
>>   
>> -	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
>> +	union {
>> +		__be16 rx_buffer;
>> +		u8 tx_buffer[2];
>> +	} __aligned(IIO_DMA_MINALIGN);
>>   };
>>   
>>   static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
>>   {
>>   	int ret;
>> +	char *msg = &adc->tx_buffer[0];
>>   
>>   	mutex_lock(&adc->lock);
>>   
>> -	adc->buffer[0] = channel << 3;
>> -	adc->buffer[1] = 0;
>> +	msg[0] = channel << 3;
>> +	msg[1] = 0;
>>   
>> -	ret = spi_write(adc->spi, &adc->buffer, 2);
>> +	ret = spi_write(adc->spi, msg, sizeof(adc->tx_buffer));
>>   	if (ret < 0) {
>>   		mutex_unlock(&adc->lock);
>>   		return ret;
>>   	}
>>   
>> -	ret = spi_read(adc->spi, &adc->buffer, 2);
>> -
>> +	ret = spi_read(adc->spi, &adc->rx_buffer, 2);
>>   	mutex_unlock(&adc->lock);
>> -
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF);
>> +	return be16_to_cpu(adc->rx_buffer) & 0xFFF;
> 
> 
> The cast isn't exactly beautiful, but this would save a lot of
> lines of diff and a few lines of code by avoiding the need for
> the union and the local msg variable.
> 
> 	return be16_to_cpup((__be16 *)adc->buffer) & 0xFFF;

Thanks again for the review David :)

I am unsure which way to go. I kind of like having the __be16 in the 
struct, as it immediately yells "data from device is big-endian". OTOH, 
I've never loved unions (and, it silences the above "yelling" quite a 
bit). I still think this might be the first time I really see a valid 
use-case for an union :) And, you're right this adds more lines, 
besides, the cast doesn't look that ugly to me. Yet, I originally had a 
cast probably as simple as this (and I also had the __be16 in the 
struct), and Jonathan suggested using union to avoid it...

At the end of the day, I suppose I am Okay with any of these 3 
approaches. Original cast, union or this cast you suggest. Jonathan, any 
preferences on your side?

> 
>>   }
>>   
>>   static int adc128_read_raw(struct iio_dev *indio_dev,
> 

Yours,
	-- Matti

Re: [PATCH RFC v2 7/7] iio: ti-adc128s052: Drop variable vref

[Matti Vaittinen]

On 02/04/2025 23:49, David Lechner wrote:
> On 4/2/25 1:10 AM, Matti Vaittinen wrote:
>> 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.
> 
> There is also around 60 other drivers where we could do something like this
> in case anyone is bored. :-p
> 
>>
>> Simplify the driver and drop the variable vref support.
>>
>> Suggested-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>> Revision History:
>>   v2:
>>    - New patch
>> ---
>>   drivers/iio/adc/ti-adc128s052.c | 29 ++++++-----------------------
>>   1 file changed, 6 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
>> index 0f93c6266527..0bfe4e558c69 100644
>> --- a/drivers/iio/adc/ti-adc128s052.c
>> +++ b/drivers/iio/adc/ti-adc128s052.c
>> @@ -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;
> 
> Units in the name are helpful: vref_uv.
> 
> Could also consider doing division in probe and storing vref_mv instead
> since we never use the microvolts part.

Ah, thanks for the suggestions. I agree with both.

> 
>>   	union {
>>   		__be16 rx_buffer;
>>   		u8 tx_buffer[2];
> 

Yours,
	-- Matti

Re: [PATCH RFC v2 7/7] iio: ti-adc128s052: Drop variable vref

[Jonathan Cameron]

On Wed, 2 Apr 2025 15:49:01 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/2/25 1:10 AM, Matti Vaittinen wrote:
> > 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.  
> 
> There is also around 60 other drivers where we could do something like this
> in case anyone is bored. :-p

Hmm. It would be a gamble but also a nice cleanup.

We 'might' meet a case where someone notices it but seems fairly unlikely...

J
> 
> > 
> > Simplify the driver and drop the variable vref support.
> > 
> > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > ---
> > Revision History:
> >  v2:
> >   - New patch
> > ---
> >  drivers/iio/adc/ti-adc128s052.c | 29 ++++++-----------------------
> >  1 file changed, 6 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> > index 0f93c6266527..0bfe4e558c69 100644
> > --- a/drivers/iio/adc/ti-adc128s052.c
> > +++ b/drivers/iio/adc/ti-adc128s052.c
> > @@ -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;  
> 
> Units in the name are helpful: vref_uv.
> 
> Could also consider doing division in probe and storing vref_mv instead
> since we never use the microvolts part.
> 
> >  	union {
> >  		__be16 rx_buffer;
> >  		u8 tx_buffer[2];  
> 

Re: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()

[Jonathan Cameron]

On Thu, 3 Apr 2025 08:16:43 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 03/04/2025 00:04, David Lechner wrote:
> > On 4/2/25 1:09 AM, Matti Vaittinen wrote:  
> >> 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.
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> >> ---
> >> Revision history:
> >> 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 | 18 ++++++++++--------
> >>   1 file changed, 10 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> >> index a456ea78462f..3e69a5fce010 100644
> >> --- a/drivers/iio/adc/ti-adc128s052.c
> >> +++ b/drivers/iio/adc/ti-adc128s052.c
> >> @@ -28,32 +28,34 @@ struct adc128 {
> >>   	struct regulator *reg;
> >>   	struct mutex lock;
> >>   
> >> -	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
> >> +	union {
> >> +		__be16 rx_buffer;
> >> +		u8 tx_buffer[2];
As below. Maybe
		__be16 buffer16;
		u8 buffer[2];

> >> +	} __aligned(IIO_DMA_MINALIGN);
> >>   };
> >>   
> >>   static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
> >>   {
> >>   	int ret;
> >> +	char *msg = &adc->tx_buffer[0];
> >>   
> >>   	mutex_lock(&adc->lock);
> >>   
> >> -	adc->buffer[0] = channel << 3;
> >> -	adc->buffer[1] = 0;
> >> +	msg[0] = channel << 3;
> >> +	msg[1] = 0;
> >>   
> >> -	ret = spi_write(adc->spi, &adc->buffer, 2);
> >> +	ret = spi_write(adc->spi, msg, sizeof(adc->tx_buffer));

I'd get rid of msg as it's now just confusing given we are
using the sizeof() here.

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

sizeof(adc->rx_buffer)

> >>   	mutex_unlock(&adc->lock);
> >> -
> >>   	if (ret < 0)
> >>   		return ret;
> >>   
> >> -	return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF);
> >> +	return be16_to_cpu(adc->rx_buffer) & 0xFFF;  
> > 
> > 
> > The cast isn't exactly beautiful, but this would save a lot of
> > lines of diff and a few lines of code by avoiding the need for
> > the union and the local msg variable.
> > 
> > 	return be16_to_cpup((__be16 *)adc->buffer) & 0xFFF;  

The cast only works because we have forced the alignment for DMA safety.
That to me is a little fragile.

You could do get_unaligned_be16() which doesn't need the cast then carry
on using the original buffer.  

> 
> Thanks again for the review David :)
> 
> I am unsure which way to go. I kind of like having the __be16 in the 
> struct, as it immediately yells "data from device is big-endian". OTOH, 
> I've never loved unions (and, it silences the above "yelling" quite a 
> bit). I still think this might be the first time I really see a valid 
> use-case for an union :) And, you're right this adds more lines, 
> besides, the cast doesn't look that ugly to me. Yet, I originally had a 
> cast probably as simple as this (and I also had the __be16 in the 
> struct), and Jonathan suggested using union to avoid it...
> 
> At the end of the day, I suppose I am Okay with any of these 3 
> approaches. Original cast, union or this cast you suggest. Jonathan, any 
> preferences on your side?

Majority of the diff is really about renaming buffer to tx_buffer.
Could just not bother doing that and instead have buffer and buffer16
as the two union elements. With msg gone as suggested above, then the diff
becomes only a few lines and you get to keep the nicety of it being either
a pair of u8s or a __be16.

Jonathan

> 
> >   
> >>   }
> >>   
> >>   static int adc128_read_raw(struct iio_dev *indio_dev,  
> >   
> 
> Yours,
> 	-- Matti
> 

Re: [PATCH v2 0/7] Support ROHM BD79104 ADC

[Jonathan Cameron]

On Wed, 2 Apr 2025 09:19:19 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 02/04/2025 09:07, Matti Vaittinen wrote:
> 
> > 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?  
> 
> Jonathan, I suppose this, by default,  falls under the umbrella of your 
> IIO maintainership. Are you Okay with it if I'll add a maintainer entry 
> which sets you explicitly as a maintainer for this, and list myself as a 
> reviewer?

Absolutely.  I'm find with you adding yourself as either R or M.
If you go with M and anyone objects, we can have more maintainers ;)
If you go with R the catch all is fine. I've never insisted on everyone
adding entries to maintainers on basis the catch all means I get
them all anyway.

Jonathan


> 
> I suppose I could also add myself as a maintainer for this, but I am 
> unsure how well it would be received by the TI people ;)
> 
> > This series was based on the v6.14, but it should apply cleanly on
> > iio/testing - please let me know if I should rebase.  
> 
> Just realized I forgot to update this. The series is now based on 
> 543da6252b48 in the iio/testing. Nonetheless, I can still rebase if needed.
> 
> Yours,
> 	-- Matti

Re: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()

[Matti Vaittinen]

On 05/04/2025 20:29, Jonathan Cameron wrote:
> On Thu, 3 Apr 2025 08:16:43 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 03/04/2025 00:04, David Lechner wrote:
>>> On 4/2/25 1:09 AM, Matti Vaittinen wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>> ---
>>>> Revision history:
>>>> 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 | 18 ++++++++++--------
>>>>    1 file changed, 10 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
>>>> index a456ea78462f..3e69a5fce010 100644
>>>> --- a/drivers/iio/adc/ti-adc128s052.c
>>>> +++ b/drivers/iio/adc/ti-adc128s052.c
>>>> @@ -28,32 +28,34 @@ struct adc128 {
>>>>    	struct regulator *reg;
>>>>    	struct mutex lock;
>>>>    
>>>> -	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
>>>> +	union {
>>>> +		__be16 rx_buffer;
>>>> +		u8 tx_buffer[2];
> As below. Maybe
> 		__be16 buffer16;
> 		u8 buffer[2];

Ok.

>>>> +	} __aligned(IIO_DMA_MINALIGN);
>>>>    };
>>>>    
>>>>    static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
>>>>    {
>>>>    	int ret;
>>>> +	char *msg = &adc->tx_buffer[0];
>>>>    
>>>>    	mutex_lock(&adc->lock);
>>>>    
>>>> -	adc->buffer[0] = channel << 3;
>>>> -	adc->buffer[1] = 0;
>>>> +	msg[0] = channel << 3;
>>>> +	msg[1] = 0;
>>>>    
>>>> -	ret = spi_write(adc->spi, &adc->buffer, 2);
>>>> +	ret = spi_write(adc->spi, msg, sizeof(adc->tx_buffer));
> 
> I'd get rid of msg as it's now just confusing given we are
> using the sizeof() here.

Ok.

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

I was thinking of this but went with raw 2 - because we need to read 
exactly 2 bytes from the device. Sizeof buffer is matter of software 
where as the 2 bytes is dictated by the device. (Sure the size of buffer 
needs to be large enough).

I don't care it that much though, so I can go with the sizeof() if 
that's what you prefer. Just explaining that the '2' here was a 
conscious choice :)

>>>>    	mutex_unlock(&adc->lock);
>>>> -
>>>>    	if (ret < 0)
>>>>    		return ret;
>>>>    
>>>> -	return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF);
>>>> +	return be16_to_cpu(adc->rx_buffer) & 0xFFF;
>>>
>>>
>>> The cast isn't exactly beautiful, but this would save a lot of
>>> lines of diff and a few lines of code by avoiding the need for
>>> the union and the local msg variable.
>>>
>>> 	return be16_to_cpup((__be16 *)adc->buffer) & 0xFFF;
> 
> The cast only works because we have forced the alignment for DMA safety.
> That to me is a little fragile.
> 
> You could do get_unaligned_be16() which doesn't need the cast then carry
> on using the original buffer.
>>
>> Thanks again for the review David :)
>>
>> I am unsure which way to go. I kind of like having the __be16 in the
>> struct, as it immediately yells "data from device is big-endian". OTOH,
>> I've never loved unions (and, it silences the above "yelling" quite a
>> bit). I still think this might be the first time I really see a valid
>> use-case for an union :) And, you're right this adds more lines,
>> besides, the cast doesn't look that ugly to me. Yet, I originally had a
>> cast probably as simple as this (and I also had the __be16 in the
>> struct), and Jonathan suggested using union to avoid it...
>>
>> At the end of the day, I suppose I am Okay with any of these 3
>> approaches. Original cast, union or this cast you suggest. Jonathan, any
>> preferences on your side?
> 
> Majority of the diff is really about renaming buffer to tx_buffer.
> Could just not bother doing that and instead have buffer and buffer16
> as the two union elements. With msg gone as suggested above, then the diff
> becomes only a few lines and you get to keep the nicety of it being either
> a pair of u8s or a __be16.

I was tempted to try using the spi_write_then_read() - but I suppose 
this may be kind of a hot path.

I'll go with (not)renaming the buffer and dropping the msg, to squeeze 
the diff.

Yours,
	-- Matti

Re: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()

[Jonathan Cameron]

On Mon, 7 Apr 2025 08:23:07 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 05/04/2025 20:29, Jonathan Cameron wrote:
> > On Thu, 3 Apr 2025 08:16:43 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> On 03/04/2025 00:04, David Lechner wrote:  
> >>> On 4/2/25 1:09 AM, Matti Vaittinen wrote:  
> >>>> 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.
> >>>>
> >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> >>>> ---
> >>>> Revision history:
> >>>> 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 | 18 ++++++++++--------
> >>>>    1 file changed, 10 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> >>>> index a456ea78462f..3e69a5fce010 100644
> >>>> --- a/drivers/iio/adc/ti-adc128s052.c
> >>>> +++ b/drivers/iio/adc/ti-adc128s052.c
> >>>> @@ -28,32 +28,34 @@ struct adc128 {
> >>>>    	struct regulator *reg;
> >>>>    	struct mutex lock;
> >>>>    
> >>>> -	u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
> >>>> +	union {
> >>>> +		__be16 rx_buffer;
> >>>> +		u8 tx_buffer[2];  
> > As below. Maybe
> > 		__be16 buffer16;
> > 		u8 buffer[2];  
> 
> Ok.
> 
> >>>> +	} __aligned(IIO_DMA_MINALIGN);
> >>>>    };
> >>>>    
> >>>>    static int adc128_adc_conversion(struct adc128 *adc, u8 channel)
> >>>>    {
> >>>>    	int ret;
> >>>> +	char *msg = &adc->tx_buffer[0];
> >>>>    
> >>>>    	mutex_lock(&adc->lock);
> >>>>    
> >>>> -	adc->buffer[0] = channel << 3;
> >>>> -	adc->buffer[1] = 0;
> >>>> +	msg[0] = channel << 3;
> >>>> +	msg[1] = 0;
> >>>>    
> >>>> -	ret = spi_write(adc->spi, &adc->buffer, 2);
> >>>> +	ret = spi_write(adc->spi, msg, sizeof(adc->tx_buffer));  
> > 
> > I'd get rid of msg as it's now just confusing given we are
> > using the sizeof() here.  
> 
> Ok.
> 
> >>>>    	if (ret < 0) {
> >>>>    		mutex_unlock(&adc->lock);
> >>>>    		return ret;
> >>>>    	}
> >>>>    
> >>>> -	ret = spi_read(adc->spi, &adc->buffer, 2);
> >>>> -
> >>>> +	ret = spi_read(adc->spi, &adc->rx_buffer, 2);  
> > 
> > sizeof(adc->rx_buffer)  
> 
> I was thinking of this but went with raw 2 - because we need to read 
> exactly 2 bytes from the device. Sizeof buffer is matter of software 
> where as the 2 bytes is dictated by the device. (Sure the size of buffer 
> needs to be large enough).
> 
> I don't care it that much though, so I can go with the sizeof() if 
> that's what you prefer. Just explaining that the '2' here was a 
> conscious choice :)

Hmm. If we have a case where we read less than 2 bytes into that buffer
then fair enough.  Otherwise it's correctly sized so having sizeof(buffer)
and having to check that size in only one place is a tiny bit preferable.


> 
> >>>>    	mutex_unlock(&adc->lock);
> >>>> -
> >>>>    	if (ret < 0)
> >>>>    		return ret;
> >>>>    
> >>>> -	return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF);
> >>>> +	return be16_to_cpu(adc->rx_buffer) & 0xFFF;  
> >>>
> >>>
> >>> The cast isn't exactly beautiful, but this would save a lot of
> >>> lines of diff and a few lines of code by avoiding the need for
> >>> the union and the local msg variable.
> >>>
> >>> 	return be16_to_cpup((__be16 *)adc->buffer) & 0xFFF;  
> > 
> > The cast only works because we have forced the alignment for DMA safety.
> > That to me is a little fragile.
> > 
> > You could do get_unaligned_be16() which doesn't need the cast then carry
> > on using the original buffer.  
> >>
> >> Thanks again for the review David :)
> >>
> >> I am unsure which way to go. I kind of like having the __be16 in the
> >> struct, as it immediately yells "data from device is big-endian". OTOH,
> >> I've never loved unions (and, it silences the above "yelling" quite a
> >> bit). I still think this might be the first time I really see a valid
> >> use-case for an union :) And, you're right this adds more lines,
> >> besides, the cast doesn't look that ugly to me. Yet, I originally had a
> >> cast probably as simple as this (and I also had the __be16 in the
> >> struct), and Jonathan suggested using union to avoid it...
> >>
> >> At the end of the day, I suppose I am Okay with any of these 3
> >> approaches. Original cast, union or this cast you suggest. Jonathan, any
> >> preferences on your side?  
> > 
> > Majority of the diff is really about renaming buffer to tx_buffer.
> > Could just not bother doing that and instead have buffer and buffer16
> > as the two union elements. With msg gone as suggested above, then the diff
> > becomes only a few lines and you get to keep the nicety of it being either
> > a pair of u8s or a __be16.  
> 
> I was tempted to try using the spi_write_then_read() - but I suppose 
> this may be kind of a hot path.

Given it's small, I doubt it would make any noticeable difference in
performance.

> 
> I'll go with (not)renaming the buffer and dropping the msg, to squeeze 
> the diff.

Works for me.

J
> 
> Yours,
> 	-- Matti