| Message ID | 20241024171703.201436-8-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7124: Make it work on de10-nano | expand |
On Thu, 24 Oct 2024 19:17:05 +0200 Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > When during a measurement two channels are enabled, two measurements are > done that are reported sequencially in the DATA register. As the code > triggered by reading one of the sysfs properties expects that only one > channel is enabled it only reads the first data set which might or might > not belong to the intended channel. > > To prevent this situation disable all channels during probe. This fixes > a problem in practise because the reset default for channel 0 is > enabled. So all measurements before the first measurement on channel 0 > (which disables channel 0 at the end) might report wrong values. > > Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> Makes sense in general, but one comment inline. > --- > drivers/iio/adc/ad7124.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c > index a5d91933f505..912ba6592560 100644 > --- a/drivers/iio/adc/ad7124.c > +++ b/drivers/iio/adc/ad7124.c > @@ -917,6 +917,9 @@ static int ad7124_setup(struct ad7124_state *st) > * set all channels to this default value. > */ > ad7124_set_channel_odr(st, i, 10); > + > + /* Disable all channels to prevent unintended conversions. */ > + ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, 0x0001); Why 1? Build that default up from the register definitions rather than a magic constant. > } > > ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
On Sun, Oct 27, 2024 at 11:42:28AM +0000, Jonathan Cameron wrote: > On Thu, 24 Oct 2024 19:17:05 +0200 > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > When during a measurement two channels are enabled, two measurements are > > done that are reported sequencially in the DATA register. As the code > > triggered by reading one of the sysfs properties expects that only one > > channel is enabled it only reads the first data set which might or might > > not belong to the intended channel. > > > > To prevent this situation disable all channels during probe. This fixes > > a problem in practise because the reset default for channel 0 is > > enabled. So all measurements before the first measurement on channel 0 > > (which disables channel 0 at the end) might report wrong values. > > > > Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels") > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > Makes sense in general, but one comment inline. > > > --- > > drivers/iio/adc/ad7124.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c > > index a5d91933f505..912ba6592560 100644 > > --- a/drivers/iio/adc/ad7124.c > > +++ b/drivers/iio/adc/ad7124.c > > @@ -917,6 +917,9 @@ static int ad7124_setup(struct ad7124_state *st) > > * set all channels to this default value. > > */ > > ad7124_set_channel_odr(st, i, 10); > > + > > + /* Disable all channels to prevent unintended conversions. */ > > + ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, 0x0001); > Why 1? Build that default up from the register definitions rather than a magic > constant. I picked 0x0001 because that's the documented reset default values for the channels > 0. But agreed. Will fix in a v2. Best regards Uwe
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c index a5d91933f505..912ba6592560 100644 --- a/drivers/iio/adc/ad7124.c +++ b/drivers/iio/adc/ad7124.c @@ -917,6 +917,9 @@ static int ad7124_setup(struct ad7124_state *st) * set all channels to this default value. */ ad7124_set_channel_odr(st, i, 10); + + /* Disable all channels to prevent unintended conversions. */ + ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, 0x0001); } ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
When during a measurement two channels are enabled, two measurements are done that are reported sequencially in the DATA register. As the code triggered by reading one of the sysfs properties expects that only one channel is enabled it only reads the first data set which might or might not belong to the intended channel. To prevent this situation disable all channels during probe. This fixes a problem in practise because the reset default for channel 0 is enabled. So all measurements before the first measurement on channel 0 (which disables channel 0 at the end) might report wrong values. Fixes: 7b8d045e497a ("iio: adc: ad7124: allow more than 8 channels") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/iio/adc/ad7124.c | 3 +++ 1 file changed, 3 insertions(+)