| Message ID | e521bb5cb60d413edbcd1ea582fd81073218eaf5.1736201898.git.Jonathan.Santos@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7768-1: Add features, improvements, and fixes | expand |
On 1/7/25 9:25 AM, Jonathan Santos wrote: > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > The ad7768-1 is a fully differential ADC, meaning that the voltage > conversion result is a signed value. Since the value is a 24 bit one, > stored in a 32 bit variable, the sign should be extended in order to get > the correct representation. > > Also the channel description has been updated to signed representation, > to match the ADC specifications. > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > --- Reviewed-by: David Lechner <dlechner@baylibre.com>
On 01/07, Jonathan Santos wrote: > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > The ad7768-1 is a fully differential ADC, meaning that the voltage > conversion result is a signed value. Since the value is a 24 bit one, Hmm, I think the reason why we sign _raw values should be because of the ADC output code format. There are differential ADCs that can measure a negative difference between IN+ and IN- but outputting straight binary data format (not signed values). In those cases, the _offset attribute is used to "shift" the _raw value so that output codes that represent IN+ < IN- are adjusted to a negative decimal value (the _raw + _offset part of IIO ABI to get to mV units). For AD7768-1/ADAQ7768-1, the ADC output code is indeed two's complement and thus signed so the code change is correct for it. Since you are probably going to re-spin on the patch series, will be nice to adjust the message to something like: The ad7768-1 ADC output code is two's complement, meaning that the voltage conversion result is a signed value. ... With that, Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > stored in a 32 bit variable, the sign should be extended in order to get > the correct representation. > > Also the channel description has been updated to signed representation, > to match the ADC specifications. > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > --- > drivers/iio/adc/ad7768-1.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c > index 113703fb7245..c3cf04311c40 100644 > --- a/drivers/iio/adc/ad7768-1.c > +++ b/drivers/iio/adc/ad7768-1.c > @@ -142,7 +142,7 @@ static const struct iio_chan_spec ad7768_channels[] = { > .channel = 0, > .scan_index = 0, > .scan_type = { > - .sign = 'u', > + .sign = 's', > .realbits = 24, > .storagebits = 32, > .shift = 8, > @@ -371,7 +371,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev, > > ret = ad7768_scan_direct(indio_dev); > if (ret >= 0) > - *val = ret; > + *val = sign_extend32(ret, chan->scan_type.realbits - 1); > > iio_device_release_direct_mode(indio_dev); > if (ret < 0) > -- > 2.34.1 >
On 01/10, Marcelo Schmitt wrote: > On 01/07, Jonathan Santos wrote: > > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > > > The ad7768-1 is a fully differential ADC, meaning that the voltage > > conversion result is a signed value. Since the value is a 24 bit one, > Hmm, I think the reason why we sign _raw values should be because of the ADC > output code format. There are differential ADCs that can measure a negative > difference between IN+ and IN- but outputting straight binary data format (not > signed values). In those cases, the _offset attribute is used to "shift" the > _raw value so that output codes that represent IN+ < IN- are adjusted to a > negative decimal value (the _raw + _offset part of IIO ABI to get to mV units). > For AD7768-1/ADAQ7768-1, the ADC output code is indeed two's complement and thus > signed so the code change is correct for it. > Since you are probably going to re-spin on the patch series, will be nice > to adjust the message to something like: > The ad7768-1 ADC output code is two's complement, meaning that the voltage > conversion result is a signed value. ... > > With that, > Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com> > You are right, Thanks! will do that. > > stored in a 32 bit variable, the sign should be extended in order to get > > the correct representation. > > > > Also the channel description has been updated to signed representation, > > to match the ADC specifications. > > > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > --- > > drivers/iio/adc/ad7768-1.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c > > index 113703fb7245..c3cf04311c40 100644 > > --- a/drivers/iio/adc/ad7768-1.c > > +++ b/drivers/iio/adc/ad7768-1.c > > @@ -142,7 +142,7 @@ static const struct iio_chan_spec ad7768_channels[] = { > > .channel = 0, > > .scan_index = 0, > > .scan_type = { > > - .sign = 'u', > > + .sign = 's', > > .realbits = 24, > > .storagebits = 32, > > .shift = 8, > > @@ -371,7 +371,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev, > > > > ret = ad7768_scan_direct(indio_dev); > > if (ret >= 0) > > - *val = ret; > > + *val = sign_extend32(ret, chan->scan_type.realbits - 1); > > > > iio_device_release_direct_mode(indio_dev); > > if (ret < 0) > > -- > > 2.34.1 > >
On Tue, 7 Jan 2025 12:25:07 -0300 Jonathan Santos <Jonathan.Santos@analog.com> wrote: > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com> > > The ad7768-1 is a fully differential ADC, meaning that the voltage > conversion result is a signed value. Since the value is a 24 bit one, > stored in a 32 bit variable, the sign should be extended in order to get > the correct representation. > > Also the channel description has been updated to signed representation, > to match the ADC specifications. > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support") > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com> Looks good, but for v2, please pull fixes to the start of the patch set. Whilst it doesn't matter here as the earlier patches are all docs, it is still good practice as anyone looking for fixes tends to look there. Jonathan > --- > drivers/iio/adc/ad7768-1.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c > index 113703fb7245..c3cf04311c40 100644 > --- a/drivers/iio/adc/ad7768-1.c > +++ b/drivers/iio/adc/ad7768-1.c > @@ -142,7 +142,7 @@ static const struct iio_chan_spec ad7768_channels[] = { > .channel = 0, > .scan_index = 0, > .scan_type = { > - .sign = 'u', > + .sign = 's', > .realbits = 24, > .storagebits = 32, > .shift = 8, > @@ -371,7 +371,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev, > > ret = ad7768_scan_direct(indio_dev); > if (ret >= 0) > - *val = ret; > + *val = sign_extend32(ret, chan->scan_type.realbits - 1); > > iio_device_release_direct_mode(indio_dev); > if (ret < 0)
diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c index 113703fb7245..c3cf04311c40 100644 --- a/drivers/iio/adc/ad7768-1.c +++ b/drivers/iio/adc/ad7768-1.c @@ -142,7 +142,7 @@ static const struct iio_chan_spec ad7768_channels[] = { .channel = 0, .scan_index = 0, .scan_type = { - .sign = 'u', + .sign = 's', .realbits = 24, .storagebits = 32, .shift = 8, @@ -371,7 +371,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev, ret = ad7768_scan_direct(indio_dev); if (ret >= 0) - *val = ret; + *val = sign_extend32(ret, chan->scan_type.realbits - 1); iio_device_release_direct_mode(indio_dev); if (ret < 0)