Currently the 'ad7606' driver supports parts with 18 and 16 bits
resolutions.
But when adding support for AD7607 (which has a 14-bit resolution) we
should check for the 'realbits' field, to be able to sign-extend correctly.
Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
---
drivers/iio/adc/ad7606.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index d0fe9fd65f3f..a1f0c2feb04a 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -568,7 +568,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
int *val)
{
struct ad7606_state *st = iio_priv(indio_dev);
- unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
+ unsigned int realbits = st->chip_info->channels[1].scan_type.realbits;
const struct iio_chan_spec *chan;
int ret;
@@ -603,15 +603,29 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
chan = &indio_dev->channels[ch + 1];
if (chan->scan_type.sign == 'u') {
- if (storagebits > 16)
+ switch (realbits) {
+ case 18:
*val = st->data.buf32[ch];
- else
+ break;
+ case 16:
*val = st->data.buf16[ch];
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
} else {
- if (storagebits > 16)
+ switch (realbits) {
+ case 18:
*val = sign_extend32(st->data.buf32[ch], 17);
- else
+ break;
+ case 16:
*val = sign_extend32(st->data.buf16[ch], 15);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
}
error_ret:
--
2.46.1On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> Currently the 'ad7606' driver supports parts with 18 and 16 bits
> resolutions.
> But when adding support for AD7607 (which has a 14-bit resolution) we
> should check for the 'realbits' field, to be able to sign-extend correctly.
>
> Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> ---
> drivers/iio/adc/ad7606.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index d0fe9fd65f3f..a1f0c2feb04a 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -568,7 +568,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> int *val)
> {
> struct ad7606_state *st = iio_priv(indio_dev);
> - unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
> + unsigned int realbits = st->chip_info->channels[1].scan_type.realbits;
> const struct iio_chan_spec *chan;
> int ret;
>
> @@ -603,15 +603,29 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
>
> chan = &indio_dev->channels[ch + 1];
> if (chan->scan_type.sign == 'u') {
> - if (storagebits > 16)
I think it would be simpler to keep this if statement for choosing
which data.bufXX to use since there are only 2 choices.
> + switch (realbits) {
> + case 18:
> *val = st->data.buf32[ch];
> - else
> + break;
> + case 16:
> *val = st->data.buf16[ch];
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> } else {
> - if (storagebits > 16)
> + switch (realbits) {
> + case 18:
> *val = sign_extend32(st->data.buf32[ch], 17);
> - else
> + break;
> + case 16:
> *val = sign_extend32(st->data.buf16[ch], 15);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
We can change this to:
*val = sign_extend32(st->data.buf16[ch], reablbits - 1);
Then no additional changes should be needed to support 14-bit chips.
And similarly, we could avoid the need to use the more verbose
switch statement.
> }
>
> error_ret:
On Mon, Oct 21, 2024 at 8:19 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 10/21/24 8:02 AM, Alexandru Ardelean wrote:
> > Currently the 'ad7606' driver supports parts with 18 and 16 bits
> > resolutions.
> > But when adding support for AD7607 (which has a 14-bit resolution) we
> > should check for the 'realbits' field, to be able to sign-extend correctly.
> >
> > Signed-off-by: Alexandru Ardelean <aardelean@baylibre.com>
> > ---
> > drivers/iio/adc/ad7606.c | 24 +++++++++++++++++++-----
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > index d0fe9fd65f3f..a1f0c2feb04a 100644
> > --- a/drivers/iio/adc/ad7606.c
> > +++ b/drivers/iio/adc/ad7606.c
> > @@ -568,7 +568,7 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> > int *val)
> > {
> > struct ad7606_state *st = iio_priv(indio_dev);
> > - unsigned int storagebits = st->chip_info->channels[1].scan_type.storagebits;
> > + unsigned int realbits = st->chip_info->channels[1].scan_type.realbits;
> > const struct iio_chan_spec *chan;
> > int ret;
> >
> > @@ -603,15 +603,29 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch,
> >
> > chan = &indio_dev->channels[ch + 1];
> > if (chan->scan_type.sign == 'u') {
> > - if (storagebits > 16)
>
> I think it would be simpler to keep this if statement for choosing
> which data.bufXX to use since there are only 2 choices.
>
>
> > + switch (realbits) {
> > + case 18:
> > *val = st->data.buf32[ch];
> > - else
> > + break;
> > + case 16:
> > *val = st->data.buf16[ch];
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > } else {
> > - if (storagebits > 16)
> > + switch (realbits) {
> > + case 18:
> > *val = sign_extend32(st->data.buf32[ch], 17);
> > - else
> > + break;
> > + case 16:
> > *val = sign_extend32(st->data.buf16[ch], 15);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
>
> We can change this to:
>
> *val = sign_extend32(st->data.buf16[ch], reablbits - 1);
>
> Then no additional changes should be needed to support 14-bit chips.
>
> And similarly, we could avoid the need to use the more verbose
> switch statement.
Ack.
>
> > }
> >
> > error_ret:
>
>
© 2016 - 2024 Red Hat, Inc.