| Message ID | 20221214120202.4658-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,1/2] iio: adc: ti-adc128s052: Use get_unaligned_beXX() | expand |
On Wed, 14 Dec 2022 14:02:01 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > This makes the driver code slightly easier to read. > While at it, use GENMASK() as well. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Hi Andy, Patch looks good, but it got me thinking. Why not just flip this driver over to using spi_write_then_read(). Would let us drop the DMA safety protections and simplify the code + use a __be16 as the read buffer and this would then be the slightly nicer aligned form. Worth doing whilst you are here? Jonathan > --- > drivers/iio/adc/ti-adc128s052.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c > index fc09ee6bb174..7c4e8025861c 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/bits.h> > #include <linux/err.h> > #include <linux/spi/spi.h> > #include <linux/module.h> > @@ -17,6 +18,8 @@ > #include <linux/property.h> > #include <linux/regulator/consumer.h> > > +#include <asm/unaligned.h> > + > struct adc128_configuration { > const struct iio_chan_spec *channels; > u8 num_channels; > @@ -33,6 +36,7 @@ struct adc128 { > > static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > { > + u16 value; > int ret; > > mutex_lock(&adc->lock); > @@ -53,7 +57,8 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) > if (ret < 0) > return ret; > > - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); > + value = get_unaligned_be16(&adc->buffer); > + return value & GENMASK(11, 0); > } > > static int adc128_read_raw(struct iio_dev *indio_dev,
On Fri, Dec 23, 2022 at 03:17:50PM +0000, Jonathan Cameron wrote: > On Wed, 14 Dec 2022 14:02:01 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > This makes the driver code slightly easier to read. > > While at it, use GENMASK() as well. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Hi Andy, > > Patch looks good, but it got me thinking. Why not just flip this driver > over to using spi_write_then_read(). Would let us drop the DMA safety > protections and simplify the code + use a __be16 as the read buffer > and this would then be the slightly nicer aligned form. > > Worth doing whilst you are here? OK, I will consider when I have time for this.
diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c index fc09ee6bb174..7c4e8025861c 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/bits.h> #include <linux/err.h> #include <linux/spi/spi.h> #include <linux/module.h> @@ -17,6 +18,8 @@ #include <linux/property.h> #include <linux/regulator/consumer.h> +#include <asm/unaligned.h> + struct adc128_configuration { const struct iio_chan_spec *channels; u8 num_channels; @@ -33,6 +36,7 @@ struct adc128 { static int adc128_adc_conversion(struct adc128 *adc, u8 channel) { + u16 value; int ret; mutex_lock(&adc->lock); @@ -53,7 +57,8 @@ static int adc128_adc_conversion(struct adc128 *adc, u8 channel) if (ret < 0) return ret; - return ((adc->buffer[0] << 8 | adc->buffer[1]) & 0xFFF); + value = get_unaligned_be16(&adc->buffer); + return value & GENMASK(11, 0); } static int adc128_read_raw(struct iio_dev *indio_dev,
This makes the driver code slightly easier to read. While at it, use GENMASK() as well. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/iio/adc/ti-adc128s052.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)