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
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,
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
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
>
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
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
© 2016 - 2026 Red Hat, Inc.