| Message ID | 20211110111750.27263-2-alexandru.tachici@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad_sigma_delta: Add sequencer support | expand |
On Wed, 10 Nov 2021 13:17:46 +0200 <alexandru.tachici@analog.com> wrote: > From: Alexandru Tachici <alexandru.tachici@analog.com> > > In continuous mode neither sigma_delta.c nor ad7124.c > will disable previously enabled channels. > > Before this patch a channel stayed enabled indefinetly, > even when one another one was supposed to be sampled. > This causes mixed samples in continuous mode to be delivered > to the host. > > By adding an update_scan_mode callback, every time the > continuous mode is activated, channels will be enabled/disabled > accordingly. > > Fixes: b3af341bbd966 ("iio: adc: Add ad7124 support") > Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com> I'm a little confused about which paths this bug affects. If we have done a single channel read, then the channel is always disabled afterwards so we won't leave one on in that path. So the remaining path is on a previous buffer setup. In that path set_channel is called as part the post_enable callback and will enable the channel. Currently nothing disables it again, so I guess if you then change the enabled channels and restart the buffer you see the condition you are covering here. However, as I read the docs, this should also affect any single reads of a later channel than the one that was originally enabled. e.g. buffered capture enabled for channel 0 then disabled. Later read of channel 1 may get the data from channel 0. The fix you have here won't close that path because we don't call update_scan_mode() in the buffer disable path (we have considered it a few times, but so many devices don't have to do an explicit disable there that we never put it in). Perhaps with hindsight we should have always called it in the disable path, but maybe with a parameter to allow devices to opt out of doing anything. We could add core callback called something like disable_all() which is called in the remove path. Alternatively the issue can be closed in the ad_sigma_delta core by adding a callback to disable channels in predisable(). Jonathan > --- > drivers/iio/adc/ad7124.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c > index 11ce6a3729a5..30299b899799 100644 > --- a/drivers/iio/adc/ad7124.c > +++ b/drivers/iio/adc/ad7124.c > @@ -669,11 +669,32 @@ static const struct attribute_group ad7124_attrs_group = { > .attrs = ad7124_attributes, > }; > > +static int ad7124_update_scan_mode(struct iio_dev *indio_dev, > + const unsigned long *scan_mask) > +{ > + struct ad7124_state *st = iio_priv(indio_dev); > + bool bit_set; > + int ret; > + int i; > + > + for (i = 0; i < st->num_channels; i++) { > + bit_set = test_bit(i, scan_mask); > + ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), > + AD7124_CHANNEL_EN_MSK, > + AD7124_CHANNEL_EN(bit_set), > + 2); > + if (ret < 0) > + return ret; > + } > + return 0; > +} > + > static const struct iio_info ad7124_info = { > .read_raw = ad7124_read_raw, > .write_raw = ad7124_write_raw, > .debugfs_reg_access = &ad7124_reg_access, > .validate_trigger = ad_sd_validate_trigger, > + .update_scan_mode = ad7124_update_scan_mode, > .attrs = &ad7124_attrs_group, > }; >
diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c index 11ce6a3729a5..30299b899799 100644 --- a/drivers/iio/adc/ad7124.c +++ b/drivers/iio/adc/ad7124.c @@ -669,11 +669,32 @@ static const struct attribute_group ad7124_attrs_group = { .attrs = ad7124_attributes, }; +static int ad7124_update_scan_mode(struct iio_dev *indio_dev, + const unsigned long *scan_mask) +{ + struct ad7124_state *st = iio_priv(indio_dev); + bool bit_set; + int ret; + int i; + + for (i = 0; i < st->num_channels; i++) { + bit_set = test_bit(i, scan_mask); + ret = ad7124_spi_write_mask(st, AD7124_CHANNEL(i), + AD7124_CHANNEL_EN_MSK, + AD7124_CHANNEL_EN(bit_set), + 2); + if (ret < 0) + return ret; + } + return 0; +} + static const struct iio_info ad7124_info = { .read_raw = ad7124_read_raw, .write_raw = ad7124_write_raw, .debugfs_reg_access = &ad7124_reg_access, .validate_trigger = ad_sd_validate_trigger, + .update_scan_mode = ad7124_update_scan_mode, .attrs = &ad7124_attrs_group, };