| Message ID | 20241021130221.1469099-4-aardelean@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7606: add support for AD760{7,8,9} parts | expand |
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. > } > > 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: > >
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:
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(-)