Re: [PATCH v3 3/3] iio: adc: ad7380: new driver for AD7380 ADCs

From: David Lechner <dlechner@baylibre.com>
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: broonie@kernel.org, conor+dt@kernel.org,
	devicetree@vger.kernel.org,  jic23@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, lgirdwood@gmail.com,
	 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	 linux-spi@vger.kernel.org, michael.hennerich@analog.com,
	nuno.sa@analog.com,  robh+dt@kernel.org, stefan.popa@analog.com
Subject: Re: [PATCH v3 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
Date: Fri, 15 Dec 2023 18:31:46 +0100	[thread overview]
Message-ID: <CAMknhBEPxYtZps2cFk0ZPckbcHenXJ_v4Dv+82ENg47J52gHxQ@mail.gmail.com> (raw)
In-Reply-To: <66e9fe7a-927b-465f-aafe-8aea0e5998a4@wanadoo.fr>

On Fri, Dec 15, 2023 at 5:53 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 15/12/2023 à 11:32, David Lechner a écrit :
> > This adds a new driver for the AD7380 family ADCs.
> >
> > The driver currently implements basic support for the AD7380, AD7381,
> > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > single-ended and 4-channel chips that use the same register map as well
> > as additional features of the chip will be added in future patches.
> >
> > Co-developed-by: Stefan Popa <stefan.popa-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Stefan Popa <stefan.popa-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> > Reviewed-by: Nuno Sa <nuno.sa-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: David Lechner <dlechner-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> > ---
>
> ...
>
> > +static void ad7380_regulator_disable(void *p)
> > +{
> > +     regulator_disable(p);
> > +}
> > +
> > +static int ad7380_probe(struct spi_device *spi)
> > +{
> > +     struct iio_dev *indio_dev;
> > +     struct ad7380_state *st;
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     st = iio_priv(indio_dev);
> > +     st->spi = spi;
> > +     st->chip_info = spi_get_device_match_data(spi);
> > +     if (!st->chip_info)
> > +             return dev_err_probe(&spi->dev, -EINVAL, "missing match data\n");
> > +
> > +     st->vref = devm_regulator_get_optional(&spi->dev, "refio");
>
> Hi,
>
> devm_regulator_get_enable_optional()?
> to save some LoC below and ad7380_regulator_disable()

It would be nice if we could, but we need the pointer to the regulator
to read the voltage of the regulator (it is the reference voltage for
an ADC). So we can't use devm_regulator_get_enable_optional() because
it only an int and not the pointer to the regulator.

>
> CJ
>
> > +     if (IS_ERR(st->vref)) {
> > +             /*
> > +              * If there is no REFIO supply, then it means that we are using
> > +              * the internal 2.5V reference.
> > +              */
> > +             if (PTR_ERR(st->vref) == -ENODEV)
> > +                     st->vref = NULL;
> > +             else
> > +                     return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
> > +                                          "Failed to get refio regulator\n");
> > +     }
> > +
> > +     if (st->vref) {
> > +             ret = regulator_enable(st->vref);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = devm_add_action_or_reset(&spi->dev, ad7380_regulator_disable,
> > +                                            st->vref);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     st->regmap = devm_regmap_init(&spi->dev, NULL, st, &ad7380_regmap_config);
> > +     if (IS_ERR(st->regmap))
> > +             return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> > +                                  "failed to allocate register map\n");
> > +
> > +     indio_dev->channels = st->chip_info->channels;
> > +     indio_dev->num_channels = st->chip_info->num_channels;
> > +     indio_dev->name = st->chip_info->name;
> > +     indio_dev->info = &ad7380_info;
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +     indio_dev->available_scan_masks = ad7380_2_channel_scan_masks;
> > +
> > +     ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> > +                                           iio_pollfunc_store_time,
> > +                                           ad7380_trigger_handler, NULL);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = ad7380_init(st);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return devm_iio_device_register(&spi->dev, indio_dev);
> > +}
>
> ...
>