| Message ID | 20250212105322.10243-7-u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7124: Implement calibration at probe time | expand |
On Wed, 12 Feb 2025 11:53:23 +0100 Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > The function ad_sd_calibrate() enables the channel to calibrate at > function entry but doesn't disable it on exit. This is problematic > because if two (or more) channels are calibrated in a row, the second > calibration isn't executed as intended as the first (still enabled) > channel is recalibrated and after the first irq (i.e. when the > calibration of the first channel completed) the calibration is aborted. > > To fix this, disable the calibrated channel after calibration. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> Whilst you said don't look in reply to patch 3 I ignored you. ;) This feels like it deserves a fixes tag. Jonathan > --- > drivers/iio/adc/ad_sigma_delta.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c > index 10e635fc4fa4..fbe241b90f37 100644 > --- a/drivers/iio/adc/ad_sigma_delta.c > +++ b/drivers/iio/adc/ad_sigma_delta.c > @@ -339,6 +339,7 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, > out: > sigma_delta->keep_cs_asserted = false; > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > + ad_sigma_delta_disable_one(sigma_delta, channel); > sigma_delta->bus_locked = false; > spi_bus_unlock(sigma_delta->spi->controller); >
Hello Jonathan, On Sun, Feb 16, 2025 at 04:36:00PM +0000, Jonathan Cameron wrote: > On Wed, 12 Feb 2025 11:53:23 +0100 > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > The function ad_sd_calibrate() enables the channel to calibrate at > > function entry but doesn't disable it on exit. This is problematic > > because if two (or more) channels are calibrated in a row, the second > > calibration isn't executed as intended as the first (still enabled) > > channel is recalibrated and after the first irq (i.e. when the > > calibration of the first channel completed) the calibration is aborted. > > > > To fix this, disable the calibrated channel after calibration. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > Whilst you said don't look in reply to patch 3 I ignored you. ;) > > This feels like it deserves a fixes tag. Maybe. The ad7124 driver currently doesn't use this function. And if the described behaviour isn't relevant for the other chips, that's fine. Needs some more research. Best regards Uwe
[adding Guillaume who added calibration support for ad7173 to Cc:] Hello, On Sun, Feb 16, 2025 at 06:17:02PM +0100, Uwe Kleine-König wrote: > On Sun, Feb 16, 2025 at 04:36:00PM +0000, Jonathan Cameron wrote: > > On Wed, 12 Feb 2025 11:53:23 +0100 > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > > > The function ad_sd_calibrate() enables the channel to calibrate at > > > function entry but doesn't disable it on exit. This is problematic > > > because if two (or more) channels are calibrated in a row, the second > > > calibration isn't executed as intended as the first (still enabled) > > > channel is recalibrated and after the first irq (i.e. when the > > > calibration of the first channel completed) the calibration is aborted. > > > > > > To fix this, disable the calibrated channel after calibration. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > > Whilst you said don't look in reply to patch 3 I ignored you. ;) > > > > This feels like it deserves a fixes tag. > > Maybe. The ad7124 driver currently doesn't use this function. And if the > described behaviour isn't relevant for the other chips, that's fine. > > Needs some more research. There are currently 3 drivers using ad_sd_calibrate(): - ad7173.c - ad7192.c - ad7793.c Reading through their data sheets and driver code: For ad7173 the issue exists. The data sheet requests "Only one channel can be active during calibration". For the other two there is no problem. The .set_channel() callback disables all channels but the requested one. (For ad7793 there can be only one active channel, for ad7192 several channels could be enabled, but that doesn't happen.) The first commit that is affected by this problem for ad7173 is 031bdc8aee01 ("iio: adc: ad7173: add calibration support") I didn't do any tests on ad7173, so my findings are only theoretic. So a second pair of eyes to confirm my findings or even a test for that problem would be great. Maybe Guillaume can share some hands-on experience? Best regards Uwe
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c index 10e635fc4fa4..fbe241b90f37 100644 --- a/drivers/iio/adc/ad_sigma_delta.c +++ b/drivers/iio/adc/ad_sigma_delta.c @@ -339,6 +339,7 @@ int ad_sd_calibrate(struct ad_sigma_delta *sigma_delta, out: sigma_delta->keep_cs_asserted = false; ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); + ad_sigma_delta_disable_one(sigma_delta, channel); sigma_delta->bus_locked = false; spi_bus_unlock(sigma_delta->spi->controller);
The function ad_sd_calibrate() enables the channel to calibrate at function entry but doesn't disable it on exit. This is problematic because if two (or more) channels are calibrated in a row, the second calibration isn't executed as intended as the first (still enabled) channel is recalibrated and after the first irq (i.e. when the calibration of the first channel completed) the calibration is aborted. To fix this, disable the calibrated channel after calibration. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/iio/adc/ad_sigma_delta.c | 1 + 1 file changed, 1 insertion(+)