| Message ID | 20240621120539.31351-1-dumitru.ceclan@analog.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: adc: ad_sigma_delta: fix disable_one callback | expand |
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 > >
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 > > > >
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 >> >>
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 > >> > >> >
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)
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(-)