| Message ID | babe1eac3de30aa22e09266de1f5521fa9e0decd.1742474322.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support ROHM BD79104 ADC | expand |
On Mon, 31 Mar 2025 11:02:55 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > ADCs supported by the ti-adc128s052 driver do return the ADC data in 16 > bits using big-endian format. The driver does unconditionally swap the > bytes. This leads to wrong values being reported to users on big endian > systems. > > Fix this by using the be16_to_cpu() instead of doing unconditional byte > swapping. It's not doing unconditional byte swap that I can see. The adc->buffer[0] << 8 | adc->buffer[1] will work on big or little endian systems as we are explicitly saying which byte represents higher bit values in a 16 bit output so on little endian it's a byte swap, but on big endian it's a noop (the compiler might noticed that and replace this code sequence with an assignment) Good cleanup, but not a fix as such unless I'm missing something. > > Fixes: 913b86468674 ("iio: adc: Add TI ADC128S052") > Cc: stable@vger.kernel.org > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > --- > I have no big endian machines on my hands to test this. Problem was > spotted by reading the code, which leaves some room for errors. > Careful reviewing is appreciated! > --- > drivers/iio/adc/ti-adc128s052.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c > index a456ea78462f..d1e31122ea0d 100644 > --- a/drivers/iio/adc/ti-adc128s052.c > +++ b/drivers/iio/adc/ti-adc128s052.c > @@ -28,19 +28,20 @@ struct adc128 { > struct regulator *reg; > struct mutex lock; > > - u8 buffer[2] __aligned(IIO_DMA_MINALIGN); > + __be16 buffer __aligned(IIO_DMA_MINALIGN); > }; > > static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > { > int ret; > + char *msg = (char *)&adc->buffer; > > - mutex_lock(&adc->lock); > + msg[0] = channel << 3; > + msg[1] = 0; Given you are writing shared state why move this out of the lock? Whilst here maybe using guard() would clean this driver up a little. Use a separate buffer (or a union) so we can avoid the casting here > > - adc->buffer[0] = channel << 3; > - adc->buffer[1] = 0; > + mutex_lock(&adc->lock); > > - ret = spi_write(adc->spi, &adc->buffer, 2); > + ret = spi_write(adc->spi, msg, 2); Given you are tidying this up, lets make the source of that size value obvious. sizeof(adc->buffer) > if (ret < 0) { > mutex_unlock(&adc->lock); > return ret; > @@ -53,7 +54,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > if (ret < 0) > return ret; > > - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); > + return be16_to_cpu(adc->buffer) & 0xFFF; > } > > static int adc128_read_raw(struct iio_dev *indio_dev,
On 31/03/2025 14:11, Jonathan Cameron wrote: > On Mon, 31 Mar 2025 11:02:55 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> ADCs supported by the ti-adc128s052 driver do return the ADC data in 16 >> bits using big-endian format. The driver does unconditionally swap the >> bytes. This leads to wrong values being reported to users on big endian >> systems. >> >> Fix this by using the be16_to_cpu() instead of doing unconditional byte >> swapping. Appears this was one of the patches I should've never written. Nothing went right :) Sorry for the noise. I'll try improving for the v2 > It's not doing unconditional byte swap that I can see. The > adc->buffer[0] << 8 | adc->buffer[1] > will work on big or little endian systems as we are explicitly saying > which byte represents higher bit values in a 16 bit output so on little > endian it's a byte swap, but on big endian it's a noop (the compiler might > noticed that and replace this code sequence with an assignment) > > Good cleanup, but not a fix as such unless I'm missing something. No, you're not missing anything. I am the one who just got confused. I am not exactly sure what I was thinking. :rolleyes: This definitely isn't a fix. And, as a not a fix needing porting, I may squash this with some other patch. I need to take another look at this :) >> Fixes: 913b86468674 ("iio: adc: Add TI ADC128S052") >> Cc: stable@vger.kernel.org >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >> --- >> I have no big endian machines on my hands to test this. Problem was >> spotted by reading the code, which leaves some room for errors. >> Careful reviewing is appreciated! >> --- >> drivers/iio/adc/ti-adc128s052.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c >> index a456ea78462f..d1e31122ea0d 100644 >> --- a/drivers/iio/adc/ti-adc128s052.c >> +++ b/drivers/iio/adc/ti-adc128s052.c >> @@ -28,19 +28,20 @@ struct adc128 { >> struct regulator *reg; >> struct mutex lock; >> >> - u8 buffer[2] __aligned(IIO_DMA_MINALIGN); >> + __be16 buffer __aligned(IIO_DMA_MINALIGN); >> }; >> >> static int adc128_adc_conversion(struct adc128 *adc, u8 channel) >> { >> int ret; >> + char *msg = (char *)&adc->buffer; >> >> - mutex_lock(&adc->lock); >> + msg[0] = channel << 3; >> + msg[1] = 0; > > Given you are writing shared state why move this out of the lock? Very Valid Point. I'm not 100% sure what I thought of, probably assumed IIO core would serialize the calls. That would've been nasty bug! I appreciate your sharp eyes :) Thanks! Yours, -- Matti
diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c index a456ea78462f..d1e31122ea0d 100644 --- a/drivers/iio/adc/ti-adc128s052.c +++ b/drivers/iio/adc/ti-adc128s052.c @@ -28,19 +28,20 @@ struct adc128 { struct regulator *reg; struct mutex lock; - u8 buffer[2] __aligned(IIO_DMA_MINALIGN); + __be16 buffer __aligned(IIO_DMA_MINALIGN); }; static int adc128_adc_conversion(struct adc128 *adc, u8 channel) { int ret; + char *msg = (char *)&adc->buffer; - mutex_lock(&adc->lock); + msg[0] = channel << 3; + msg[1] = 0; - adc->buffer[0] = channel << 3; - adc->buffer[1] = 0; + mutex_lock(&adc->lock); - ret = spi_write(adc->spi, &adc->buffer, 2); + ret = spi_write(adc->spi, msg, 2); if (ret < 0) { mutex_unlock(&adc->lock); return ret; @@ -53,7 +54,7 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) if (ret < 0) return ret; - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); + return be16_to_cpu(adc->buffer) & 0xFFF; } static int adc128_read_raw(struct iio_dev *indio_dev,
ADCs supported by the ti-adc128s052 driver do return the ADC data in 16 bits using big-endian format. The driver does unconditionally swap the bytes. This leads to wrong values being reported to users on big endian systems. Fix this by using the be16_to_cpu() instead of doing unconditional byte swapping. Fixes: 913b86468674 ("iio: adc: Add TI ADC128S052") Cc: stable@vger.kernel.org Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- I have no big endian machines on my hands to test this. Problem was spotted by reading the code, which leaves some room for errors. Careful reviewing is appreciated! --- drivers/iio/adc/ti-adc128s052.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)