On Fri, 31 May 2024 16:19:36 -0500
David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> This makes use of the new devm_regulator_get_enable_read_voltage()
> function to reduce boilerplate code.
>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
A possible corner case inline.
Patches 2-4 lgtm.
> ---
> drivers/iio/adc/ad7944.c | 62 +++++++++++++++---------------------------------
> 1 file changed, 19 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> index e2cb64cef476..42bbcb904778 100644
> --- a/drivers/iio/adc/ad7944.c
> +++ b/drivers/iio/adc/ad7944.c
> @@ -464,23 +464,16 @@ static const char * const ad7944_power_supplies[] = {
> "avdd", "dvdd", "bvdd", "vio"
> };
>
> -static void ad7944_ref_disable(void *ref)
> -{
> - regulator_disable(ref);
> -}
> -
> static int ad7944_probe(struct spi_device *spi)
> {
> const struct ad7944_chip_info *chip_info;
> struct device *dev = &spi->dev;
> struct iio_dev *indio_dev;
> struct ad7944_adc *adc;
> - bool have_refin = false;
> - struct regulator *ref;
> struct iio_chan_spec *chain_chan;
> unsigned long *chain_scan_masks;
> u32 n_chain_dev;
> - int ret;
> + int ret, ref_mv, refin_mv;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
> if (!indio_dev)
> @@ -531,47 +524,30 @@ static int ad7944_probe(struct spi_device *spi)
> * - external reference: REF is connected, REFIN is not connected
> */
>
> - ref = devm_regulator_get_optional(dev, "ref");
> - if (IS_ERR(ref)) {
> - if (PTR_ERR(ref) != -ENODEV)
> - return dev_err_probe(dev, PTR_ERR(ref),
> - "failed to get REF supply\n");
> -
> - ref = NULL;
> - }
> + ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> + if (ret == -ENODEV)
> + ref_mv = 0;
> + else if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to get REF voltage\n");
> + else
> + ref_mv = ret / 1000;
>
> - ret = devm_regulator_get_enable_optional(dev, "refin");
> - if (ret == 0)
> - have_refin = true;
> - else if (ret != -ENODEV)
> - return dev_err_probe(dev, ret,
> - "failed to get and enable REFIN supply\n");
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> + if (ret == -ENODEV)
> + refin_mv = 0;
> + else if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
> + else
> + refin_mv = ret / 1000;
How does refin_mv get used? Previously we never queried it's voltage (I assume
because it supplies an internal reference?
Are there any regulators that are real enough to enable but for which a voltage
can't be queried? I think fixed regulators with gpio control are in this
category...
>
> - if (have_refin && ref)
> + if (ref_mv && refin_mv)
> return dev_err_probe(dev, -EINVAL,
> "cannot have both refin and ref supplies\n");
>
> - if (ref) {
> - ret = regulator_enable(ref);
> - if (ret)
> - return dev_err_probe(dev, ret,
> - "failed to enable REF supply\n");
> -
> - ret = devm_add_action_or_reset(dev, ad7944_ref_disable, ref);
> - if (ret)
> - return ret;
> -
> - ret = regulator_get_voltage(ref);
> - if (ret < 0)
> - return dev_err_probe(dev, ret,
> - "failed to get REF voltage\n");
> -
> - /* external reference */
> - adc->ref_mv = ret / 1000;
> - } else {
> - /* internal reference */
> + if (ref_mv)
> + adc->ref_mv = ref_mv;
> + else
> adc->ref_mv = AD7944_INTERNAL_REF_MV;
> - }
>
> adc->cnv = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
> if (IS_ERR(adc->cnv))
>
