Support ROHM BD79104 ADC
Subsystems (3)
Matti Vaittinen
maintainerv2 cover 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.
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
Signature · 1 line
2.49.0
Matti Vaittinen
maintainerPATCH v2 1/7 dt-bindings: ROHM BD79104 ADC
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
Documentation/devicetree/bindings/iio/adc/rohm,bd79104.yaml new file +69 -0
@@ -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>; + }; + }; +...
Signature · 1 line
2.49.0
Matti Vaittinen
maintainerPATCH v2 2/7 iio: adc: ti-adc128s052: Simplify using be16_to_cpu()
In reply to Matti VaittinenThe 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:
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(-)
drivers/iio/adc/ti-adc128s052.c +10 -8
@@ -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,
Signature · 1 line
2.49.0
David Lechner
reviewerRe: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()
Quoted 50 lines · Matti Vaittinen
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 <[email protected]>---
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(-)drivers/iio/adc/ti-adc128s052.c +8
-4@@ -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; }
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 58
Quoted 10 lines
drivers/iio/adc/ti-adc128s052.c +2
-4- 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;
Quoted 3 lines
drivers/iio/adc/ti-adc128s052.c +0
-0} static int adc128_read_raw(struct iio_dev *indio_dev,
Matti Vaittinen
maintainerRe: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()
Quoted 67 lines · David Lechner
Quoted 50 lines · Matti Vaittinen
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 <[email protected]>---
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(-)drivers/iio/adc/ti-adc128s052.c +8
-4@@ -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; }drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 58
Quoted 10 lines
drivers/iio/adc/ti-adc128s052.c +2
-4- 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.
Quoted 1 line
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?
Quoted 5 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 61
Quoted 4 lines
drivers/iio/adc/ti-adc128s052.c +0
-0} static int adc128_read_raw(struct iio_dev *indio_dev,
Yours,
-- Matti
Jonathan Cameron
maintainerRe: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()
Quoted 26 lines · On Thu, 3 Apr 2025 08:16:43 +0300 Matti Vaittinen <[email protected]>…
Quoted 25 lines · David Lechner
Quoted 24 lines · Matti Vaittinen
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 <[email protected]>---
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(-)drivers/iio/adc/ti-adc128s052.c +0
-0@@ -28,32 +28,34 @@ struct adc128 {struct regulator *reg;struct mutex lock;
Quoted 4 lines
Quoted 4 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 33
Quoted 4 lines
drivers/iio/adc/ti-adc128s052.c +3
-1-u8 buffer[2 ]__aligned(IIO_DMA_MINALIGN) ; +union { +__be16 rx_buffer; +u8 tx_buffer[2 ];
As below. Maybe
__be16 buffer16; u8 buffer[2];
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/iio/adc/ti-adc128s052.c +2
-0+ }__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);
Quoted 7 lines
Quoted 7 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 47
Quoted 7 lines
drivers/iio/adc/ti-adc128s052.c +3
-3- 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.
Quoted 8 lines
Quoted 8 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 53
Quoted 8 lines
drivers/iio/adc/ti-adc128s052.c +1
-2if (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)
Quoted 13 lines
Quoted 13 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 58
Quoted 7 lines
drivers/iio/adc/ti-adc128s052.c +1
-2mutex_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.
Quoted 1 line
Quoted 1 line
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.
Quoted 12 lines
Thanks again for the review David :)
I am unsure which way to go. I kind of like having the
__be16in 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__be16in the struct), and Jonathan suggested using union to avoid it...
Quoted 3 lines
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
Quoted 10 lines
Quoted 6 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 61
Quoted 5 lines
drivers/iio/adc/ti-adc128s052.c +0
-0} static int adc128_read_raw(struct iio_dev *indio_dev,Yours,
-- Matti
Matti Vaittinen
maintainerRe: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()
Quoted 29 lines · Jonathan Cameron
Quoted 26 lines · On Thu, 3 Apr 2025 08:16:43 +0300 Matti Vaittinen <[email protected]>…
Quoted 25 lines · David Lechner
Quoted 24 lines · Matti Vaittinen
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 <[email protected]>---
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(-)drivers/iio/adc/ti-adc128s052.c +0
-0@@ -28,32 +28,34 @@ struct adc128 {struct regulator *reg;struct mutex lock;
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 33
Quoted 4 lines
drivers/iio/adc/ti-adc128s052.c +3
-1-u8 buffer[2 ]__aligned(IIO_DMA_MINALIGN) ; +union { +__be16 rx_buffer; +u8 tx_buffer[2 ];As below. Maybe
__be16 buffer16; u8 buffer[2];
Ok.
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
drivers/iio/adc/ti-adc128s052.c +4
-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 ;
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 47
Quoted 2 lines
drivers/iio/adc/ti-adc128s052.c +1
-1- 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.
Quoted 10 lines
Quoted 8 lines
Quoted 8 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 53
Quoted 8 lines
drivers/iio/adc/ti-adc128s052.c +1
-2if (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 :)
Quoted 36 lines
Quoted 13 lines
Quoted 13 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 58
Quoted 7 lines
drivers/iio/adc/ti-adc128s052.c +1
-2mutex_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.
Quoted 1 line
Quoted 1 line
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.Quoted 12 lines
Thanks again for the review David :)
I am unsure which way to go. I kind of like having the
__be16in 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__be16in the struct), and Jonathan suggested using union to avoid it...Quoted 3 lines
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?
Quoted 5 lines
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
Jonathan Cameron
maintainerRe: [PATCH v2 2/7] iio: adc: ti-adc128s052: Simplify using be16_to_cpu()
Quoted 74 lines · On Mon, 7 Apr 2025 08:23:07 +0300 Matti Vaittinen <[email protected]>…
Quoted 29 lines · Jonathan Cameron
Quoted 26 lines · On Thu, 3 Apr 2025 08:16:43 +0300 Matti Vaittinen <[email protected]>…
Quoted 25 lines · David Lechner
Quoted 24 lines · Matti Vaittinen
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 <[email protected]>---
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(-)drivers/iio/adc/ti-adc128s052.c +0
-0@@ -28,32 +28,34 @@ struct adc128 {struct regulator *reg;struct mutex lock;Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 33
Quoted 4 lines
drivers/iio/adc/ti-adc128s052.c +3
-1-u8 buffer[2 ]__aligned(IIO_DMA_MINALIGN) ; +union { +__be16 rx_buffer; +u8 tx_buffer[2 ];As below. Maybe
__be16 buffer16; u8 buffer[2];Ok.
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
drivers/iio/adc/ti-adc128s052.c +4
-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 ;Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 47
Quoted 2 lines
drivers/iio/adc/ti-adc128s052.c +1
-1- 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.
Quoted 10 lines
Quoted 8 lines
Quoted 8 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 53
Quoted 8 lines
drivers/iio/adc/ti-adc128s052.c +1
-2if (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)
Quoted 8 lines
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.
Quoted 37 lines
Quoted 37 lines
Quoted 14 lines
Quoted 14 lines
drivers/iio/adc/ti-adc128s052.c struct adc128 { · line 58
Quoted 8 lines
drivers/iio/adc/ti-adc128s052.c +1
-2mutex_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.
Quoted 1 line
Quoted 1 line
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.Quoted 15 lines
Thanks again for the review David :)
I am unsure which way to go. I kind of like having the
__be16in 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__be16in 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?
Quoted 8 lines
Quoted 5 lines
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.
Quoted 3 lines
I'll go with (not)renaming the buffer and dropping the msg, to squeeze the diff.
Works for me.
J
Quoted 3 lines
Yours,
-- Matti
Matti Vaittinen
maintainerPATCH v2 3/7 iio: adc: ti-adc128s052: Be consistent with arrays
In reply to Matti VaittinenThe 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:
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(-)
drivers/iio/adc/ti-adc128s052.c +1 -1
@@ -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);
Signature · 1 line
2.49.0
Matti Vaittinen
maintainerPATCH v2 4/7 iio: adc: ti-adc128s052: Use devm_mutex_init()
In reply to Matti VaittinenQuoting 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.
---
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(-)
drivers/iio/adc/ti-adc128s052.c +3 -1
@@ -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); }
Signature · 1 line
2.49.0
Matti Vaittinen
maintainerPATCH v2 5/7 iio: adc: ti-adc128s052: Simplify using guard(mutex)
In reply to Matti VaittinenError path in ADC reading function can be slighly simplified using the guard(mutex).
Use guard(mutex) and document the mutex purpose.
---
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(-)
drivers/iio/adc/ti-adc128s052.c +7 -5
@@ -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;
Signature · 1 line
2.49.0
Matti Vaittinen
maintainerPATCH v2 6/7 iio: adc: ti-adc128s052: Support ROHM BD79104
In reply to Matti VaittinenThe 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:
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(-)
drivers/iio/adc/Kconfig +1 -1
@@ -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.
drivers/iio/adc/ti-adc128s052.c +36 -4
@@ -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);
Signature · 1 line
2.49.0
Matti Vaittinen
maintainerRFC PATCH v2 7/7 iio: ti-adc128s052: Drop variable vref
In reply to Matti VaittinenAccording 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:
- New patch
--- drivers/iio/adc/ti-adc128s052.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)
drivers/iio/adc/ti-adc128s052.c +6 -23
@@ -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,
Signature · 1 line
2.49.0
David Lechner
reviewerRe: [PATCH RFC v2 7/7] iio: ti-adc128s052: Drop variable vref
Quoted 3 lines · Matti Vaittinen
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
Quoted 19 lines
Simplify the driver and drop the variable vref support.
Suggested-by Jonathan Cameron <[email protected]>Signed-off-by Matti Vaittinen <[email protected]>---
Revision History:
v2:
- New patch--- drivers/iio/adc/ti-adc128s052.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)drivers/iio/adc/ti-adc128s052.c +0
-0@@ -29,13 +29,12 @@ struct adc128_configuration {struct adc128 {
drivers/iio/adc/ti-adc128s052.c struct adc128_configuration { · line 37
Quoted 10 lines
drivers/iio/adc/ti-adc128s052.c +1
-2struct 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.
Quoted 3 lines
drivers/iio/adc/ti-adc128s052.c +0
-0union {__be16 rx_buffer;u8 tx_buffer[2 ];
Matti Vaittinen
maintainerRe: [PATCH RFC v2 7/7] iio: ti-adc128s052: Drop variable vref
Quoted 40 lines · David Lechner
Quoted 3 lines · Matti Vaittinen
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
Quoted 19 lines
Simplify the driver and drop the variable vref support.
Suggested-by Jonathan Cameron <[email protected]>Signed-off-by Matti Vaittinen <[email protected]>---
Revision History:
v2:
- New patch--- drivers/iio/adc/ti-adc128s052.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)drivers/iio/adc/ti-adc128s052.c +0
-0@@ -29,13 +29,12 @@ struct adc128_configuration {struct adc128 {drivers/iio/adc/ti-adc128s052.c struct adc128_configuration { · line 37
Quoted 10 lines
drivers/iio/adc/ti-adc128s052.c +1
-2struct 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.
Quoted 2 lines
Could also consider doing division in probe and storing
vref_mvinstead since we never use the microvolts part.
Ah, thanks for the suggestions. I agree with both.
Quoted 5 lines
drivers/iio/adc/ti-adc128s052.c struct adc128_configuration { · line 40
Quoted 4 lines
drivers/iio/adc/ti-adc128s052.c +0
-0union {__be16 rx_buffer;u8 tx_buffer[2 ];
Yours,
-- Matti
Jonathan Cameron
maintainerRe: [PATCH RFC v2 7/7] iio: ti-adc128s052: Drop variable vref
In reply to David LechnerQuoted 7 lines · On Wed, 2 Apr 2025 15:49:01 -0500 David Lechner <[email protected]> wrote:
Quoted 3 lines · Matti Vaittinen
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
Quoted 40 lines
Quoted 20 lines
Simplify the driver and drop the variable vref support.
Suggested-by Jonathan Cameron <[email protected]>Signed-off-by Matti Vaittinen <[email protected]>---
Revision History:
v2:
- New patch--- drivers/iio/adc/ti-adc128s052.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)drivers/iio/adc/ti-adc128s052.c +0
-0@@ -29,13 +29,12 @@ struct adc128_configuration {struct adc128 {drivers/iio/adc/ti-adc128s052.c struct adc128_configuration { · line 37
Quoted 10 lines
drivers/iio/adc/ti-adc128s052.c +1
-2struct 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_mvinstead since we never use the microvolts part.Quoted 3 lines
drivers/iio/adc/ti-adc128s052.c +0
-0union {__be16 rx_buffer;u8 tx_buffer[2 ];
Matti Vaittinen
maintainerQuoted 5 lines · Matti Vaittinen
Finally, I didn't find maintainer information for this driver from the
MAINTAINERSfile. 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 ;)
Quoted 2 lines
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
Jonathan Cameron
maintainerQuoted 8 lines · On Wed, 2 Apr 2025 09:19:19 +0300 Matti Vaittinen <[email protected]>…
Quoted 5 lines · Matti Vaittinen
Finally, I didn't find maintainer information for this driver from the
MAINTAINERSfile. 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?
Quoted 4 lines
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
Quoted 12 lines
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 ;)
Quoted 2 lines
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
543da6252b48in the iio/testing. Nonetheless, I can still rebase if needed.Yours,
-- Matti