linux-kernel - Re: [PATCH] iio: adc: ad_sigma_delta: fix disable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 23 Jun 2024 11:19:16 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Alexandru Ardelean <aardelean@...libre.com>
Cc: Dumitru Ceclan <mitrutzceclan@...il.com>, dlechner@...libre.com,
 dumitru.ceclan@...log.com0, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, Dumitru Ceclan <dumitru.ceclan@...log.com>
Subject: Re: [PATCH] iio: adc: ad_sigma_delta: fix disable_one callback

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

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