[PATCH] iio: adc: ad_sigma_delta: fix disable_one callback

[PATCH] iio: adc: ad_sigma_delta: fix disable_one callback

[Dumitru Ceclan]

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

Fixes: a25a0aab2187 ("iio: adc: ad_sigma_delta: add disable_one callback")
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 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

Re: [PATCH] iio: adc: ad_sigma_delta: fix disable_one callback

[Alexandru Ardelean]

On Fri, Jun 21, 2024 at 3:05 PM Dumitru Ceclan <mitrutzceclan@gmail.com> 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


> Fixes: a25a0aab2187 ("iio: adc: ad_sigma_delta: add disable_one callback")
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
>  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
>
>

Re: [PATCH] iio: adc: ad_sigma_delta: fix disable_one callback

[Jonathan Cameron]

On Fri, 21 Jun 2024 16:26:29 +0300
Alexandru Ardelean <aardelean@baylibre.com> wrote:

> On Fri, Jun 21, 2024 at 3:05 PM Dumitru Ceclan <mitrutzceclan@gmail.com> 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
> >  

I'm going to treat Alexandru's suggestion as a potential separate issue.
I've squashed this patch into the one if fixes. That was particularly necessary
as the ID isn't stable yet.  I may need to rebase in order to get some
fixes before I send another pull request.

> 
> 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
> 
> 
> > Fixes: a25a0aab2187 ("iio: adc: ad_sigma_delta: add disable_one callback")
> > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > ---
> >  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
> >
> >  

Re: [PATCH] iio: adc: ad_sigma_delta: fix disable_one callback

[Ceclan, Dumitru]

On 21/06/2024 16:26, Alexandru Ardelean wrote:
> On Fri, Jun 21, 2024 at 3:05 PM Dumitru Ceclan <mitrutzceclan@gmail.com> 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@analog.com>
>> ---
>>  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
>>
>>

Re: [PATCH] iio: adc: ad_sigma_delta: fix disable_one callback

[Alexandru Ardelean]

On Tue, Jun 25, 2024 at 12:45 PM Ceclan, Dumitru
<mitrutzceclan@gmail.com> wrote:
>
> On 21/06/2024 16:26, Alexandru Ardelean wrote:
> > On Fri, Jun 21, 2024 at 3:05 PM Dumitru Ceclan <mitrutzceclan@gmail.com> 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.

Ack.
Thanks for the explanation :)
I think I also misread the code there; I was thinking num_slots is
related to the number of channels.

>
> >> Fixes: a25a0aab2187 ("iio: adc: ad_sigma_delta: add disable_one callback")
> >> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >> ---
> >>  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
> >>
> >>
>