On 6/18/24 4:45 AM, Alisa-Dariana Roman wrote:
> On 17.06.2024 23:00, David Lechner wrote:
> ...
>
>>
>> We could make a ret2 local variable inside of the if block to avoid writing over ret.
>>
>
> Nice! If this looks alright, I will send it along my updated series.
>
> From f84206b85b04f89d57b9293dd93e017efe8b350c Mon Sep 17 00:00:00 2001
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Date: Wed, 12 Jun 2024 16:03:05 -0500
> Subject: [PATCH] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage
>
> This makes use of the new devm_regulator_get_enable_read_voltage()
> function to reduce boilerplate code.
>
> Error messages have changed slightly since there are now fewer places
> where we print an error. The rest of the logic of selecting which
> supply to use as the reference voltage remains the same.
>
> Also 1000 is replaced by MILLI in a few places for consistency.
>
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---
> drivers/iio/adc/ad7192.c | 101 +++++++++++++--------------------------
> 1 file changed, 34 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index c7fb51a90e87..2448b01e0d59 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -200,8 +200,6 @@ struct ad7192_chip_info {
>
> struct ad7192_state {
> const struct ad7192_chip_info *chip_info;
> - struct regulator *avdd;
> - struct regulator *vref;
> struct clk *mclk;
> u16 int_vref_mv;
> u32 aincom_mv;
> @@ -1189,18 +1187,12 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> },
> };
>
> -static void ad7192_reg_disable(void *reg)
> -{
> - regulator_disable(reg);
> -}
> -
> static int ad7192_probe(struct spi_device *spi)
> {
> struct device *dev = &spi->dev;
> struct ad7192_state *st;
> struct iio_dev *indio_dev;
> - struct regulator *aincom;
> - int ret;
> + int ret, ret2, avdd_mv;
>
> if (!spi->irq)
> return dev_err_probe(dev, -ENODEV, "Failed to get IRQ\n");
> @@ -1218,72 +1210,47 @@ static int ad7192_probe(struct spi_device *spi)
> * Newer firmware should provide a zero volt fixed supply if wired to
> * ground.
> */
> - aincom = devm_regulator_get_optional(dev, "aincom");
> - if (IS_ERR(aincom)) {
> - if (PTR_ERR(aincom) != -ENODEV)
> - return dev_err_probe(dev, PTR_ERR(aincom),
> - "Failed to get AINCOM supply\n");
> -
> - st->aincom_mv = 0;
> - } else {
> - ret = regulator_enable(aincom);
> - if (ret)
> - return dev_err_probe(dev, ret,
> - "Failed to enable specified AINCOM supply\n");
> -
> - ret = devm_add_action_or_reset(dev, ad7192_reg_disable, aincom);
> - if (ret)
> - return ret;
> -
> - ret = regulator_get_voltage(aincom);
> - if (ret < 0)
> - return dev_err_probe(dev, ret,
> - "Device tree error, AINCOM voltage undefined\n");
> - st->aincom_mv = ret / MILLI;
> + ret = devm_regulator_get_enable_read_voltage(dev, "aincom");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get AINCOM voltage\n");
> +
> + st->aincom_mv = ret == -ENODEV ? 0 : ret / MILLI;
> +
> + /* AVDD can optionally be used as reference voltage */
> + ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
> + if (ret == -ENODEV || ret == -EINVAL) {
int ret2;
I would declare ret2 here since it is the only place it is used.
> + /*
> + * We get -EINVAL if avdd is a supply with unknown voltage. We
> + * still need to enable it since it is also a power supply.
> + */
> + ret2 = devm_regulator_get_enable(dev, "avdd");
> + if (ret2)
> + return dev_err_probe(dev, ret2,
> + "Failed to enable AVDD supply\n");
> + } else if (ret < 0) {
> + return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
> }
>
> - st->avdd = devm_regulator_get(dev, "avdd");
> - if (IS_ERR(st->avdd))
> - return PTR_ERR(st->avdd);
> -
> - ret = regulator_enable(st->avdd);
> - if (ret)
> - return dev_err_probe(dev, ret,
> - "Failed to enable specified AVdd supply\n");
> -
> - ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->avdd);
> - if (ret)
> - return ret;
> + avdd_mv = (ret == -ENODEV) | (ret == -EINVAL) ? 0 : ret / MILLI;
This could be simplified to ret < 0.
Or if you want to leave it explicit, use || instead of |.
And () aren't really needed either.
>
> ret = devm_regulator_get_enable(dev, "dvdd");
> if (ret)
> return dev_err_probe(dev, ret, "Failed to enable specified DVdd supply\n");
>
> - st->vref = devm_regulator_get_optional(dev, "vref");
> - if (IS_ERR(st->vref)) {
> - if (PTR_ERR(st->vref) != -ENODEV)
> - return PTR_ERR(st->vref);
> -
> - ret = regulator_get_voltage(st->avdd);
> - if (ret < 0)
> - return dev_err_probe(dev, ret,
> - "Device tree error, AVdd voltage undefined\n");
> - } else {
> - ret = regulator_enable(st->vref);
> - if (ret)
> - return dev_err_probe(dev, ret,
> - "Failed to enable specified Vref supply\n");
> -
> - ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->vref);
> - if (ret)
> - return ret;
> -
> - ret = regulator_get_voltage(st->vref);
> - if (ret < 0)
> - return dev_err_probe(dev, ret,
> - "Device tree error, Vref voltage undefined\n");
> + /*
> + * This is either REFIN1 or REFIN2 depending on adi,refin2-pins-enable.
> + * If this supply is not present, fall back to AVDD as reference.
> + */
> + ret = devm_regulator_get_enable_read_voltage(dev, "vref");
> + if (ret == -ENODEV) {
> + if (avdd_mv == 0)
> + return dev_err_probe(dev, -ENODEV,
> + "No reference voltage available\n");
> + } else if (ret < 0) {
> + return ret;
> }
> - st->int_vref_mv = ret / 1000;
> +
> + st->int_vref_mv = ret == -ENODEV ? avdd_mv : ret / MILLI;
>
> st->chip_info = spi_get_device_match_data(spi);
> indio_dev->name = st->chip_info->name;
