On 21/06/2024 16:26, Alexandru Ardelean wrote:
> On Fri, Jun 21, 2024 at 3:05 PM Dumitru Ceclan <mitrutzceclan@xxxxxxxxx> wrote:
>>
>> The ADC ad7192 is a sigma delta ADC with a sequencer that does not
>> require a disable_one callback as all enable channel bits are within
>> the same register.
>>
>> Remove the requirement of the disable_one callback for sigma delta ADCs
>> with a sequencer.
>>
>> This patch could be squashed with the commit that it fixes from patch
>> series: Add support for AD411x
>>
>
> This fix looks fine.
> But, then this raises a question if this needs be to extended to the
> `disable_all` and maybe `indio_dev->info->update_scan_mode` check.
> And if so, how should this be handled?
>
> For example:
> drivers/iio/adc/ad7124.c: .disable_all = ad7124_disable_all,
> drivers/iio/adc/ad7173.c: .disable_all = ad7173_disable_all,
> drivers/iio/adc/ad7192.c: .disable_all = ad7192_disable_all,
> drivers/iio/adc/ad7192.c: .disable_all = ad7192_disable_all,
>
> And:
> drivers/iio/adc/ad7124.c: ret = ad_sd_init(&st->sd, indio_dev, spi,
> &ad7124_sigma_delta_info);
> drivers/iio/adc/ad7173.c: ret = ad_sd_init(&st->sd, indio_dev, spi,
> &ad7173_sigma_delta_info);
> drivers/iio/adc/ad7192.c: ret = ad_sd_init(&st->sd, indio_dev, spi,
> st->chip_info->sigma_delta_info);
> drivers/iio/adc/ad7780.c: ad_sd_init(&st->sd, indio_dev, spi,
> &ad7780_sigma_delta_info);
> drivers/iio/adc/ad7791.c: ad_sd_init(&st->sd, indio_dev, spi,
> &ad7791_sigma_delta_info);
> drivers/iio/adc/ad7793.c: ad_sd_init(&st->sd, indio_dev, spi,
> &ad7793_sigma_delta_info);
>
> At least the ad7791.c & ad7793.c drivers support parts with more than
> 1 channel, and there does not seem to be any `disable_all` hook
> defined (at least in iio/testing).
> I have not gone too deep with `indio_dev->info->update_scan_mode`, but
> it would be worth to do a check there as well
>
>
disable_all and update_scan_mode callbacks are required only when the ADC
features a sequencer, as in you can enable more than one channel at a time
and the ADC will automatically cycle conversions through all enabled channels.
This feature is signaled by setting the ad_sigma_delta_info.num_slots attribute
to a value >1. The only drivers that set .num_slot from what I see are:
ad7173, ad7192, ad7124. So only these 3 are relevant in this discussion.
For ad7192, disable_all() is useless as both ad7192_set_channel() and
ad7192_update_scan_mode() actually set to 0 other channel enable bits.
I do not see any reason at this moment to extend this to disable_all and
update_scan_mode.
>> Fixes: a25a0aab2187 ("iio: adc: ad_sigma_delta: add disable_one callback")
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@xxxxxxxxxx>
>> ---
>> drivers/iio/adc/ad_sigma_delta.c | 5 -----
>> 1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
>> index d6b5fca034a0..8c062b0d26e3 100644
>> --- a/drivers/iio/adc/ad_sigma_delta.c
>> +++ b/drivers/iio/adc/ad_sigma_delta.c
>> @@ -672,11 +672,6 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
>> dev_err(&spi->dev, "ad_sigma_delta_info lacks disable_all().\n");
>> return -EINVAL;
>> }
>> -
>> - if (!info->disable_one) {
>> - dev_err(&spi->dev, "ad_sigma_delta_info lacks disable_one().\n");
>> - return -EINVAL;
>> - }
>> }
>>
>> if (info->irq_line)
>> --
>> 2.43.0
>>
>>
