Date: Sat, 1 Jun 2024 13:48:28 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Marcelo Schmitt <marcelo.schmitt1@...il.com>, Nuno Sá
 <nuno.sa@...log.com>, Michael Hennerich <Michael.Hennerich@...log.com>,
 Mark Brown <broonie@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] iio: adc: ad7192: use
 devm_regulator_get_enable_read_voltage

On Fri, 31 May 2024 16:19:32 -0500
David Lechner <dlechner@...libre.com> wrote:

> 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@...libre.com>
Ouch diff didn't like this one much and it is a bit hard to read.

One case where I think this has an unintended side effect.
See below.

> ---
>  drivers/iio/adc/ad7192.c | 98 +++++++++++++++++-------------------------------
>  1 file changed, 35 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 0789121236d6..e08bf066b3f6 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,17 +1187,11 @@ 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 ad7192_state *st;
>  	struct iio_dev *indio_dev;
> -	struct regulator *aincom;
> -	int ret;
> +	int ret, avdd_mv;
>  
>  	if (!spi->irq) {
>  		dev_err(&spi->dev, "no IRQ?\n");
> @@ -1219,74 +1211,54 @@ 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(&spi->dev, "aincom");
> -	if (IS_ERR(aincom)) {
> -		if (PTR_ERR(aincom) != -ENODEV)
> -			return dev_err_probe(&spi->dev, PTR_ERR(aincom),
> -					     "Failed to get AINCOM supply\n");
> -
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "aincom");
> +	if (ret == -ENODEV)
>  		st->aincom_mv = 0;
> -	} else {
> -		ret = regulator_enable(aincom);
> -		if (ret)
> -			return dev_err_probe(&spi->dev, ret,
> -					     "Failed to enable specified AINCOM supply\n");
> +	else if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret, "Failed to get AINCOM voltage\n");
> +	else
> +		st->aincom_mv = ret / MILLI;
>  
> -		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
> +	/* AVDD can optionally be used as reference voltage */
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "avdd");
> +	if (ret == -EINVAL) {
> +		/*
> +		 * We get -EINVAL if avdd is a supply with unknown voltage. We
> +		 * still need to enable it since it is also a power supply.
> +		 */
> +		ret = devm_regulator_get_enable(&spi->dev, "avdd");

What happens if the entry simply isn't there in the DT.
Previously I think the regulator framework would supply a stub whereas
the devm_regulator_get_enable_read_voltage() returns -ENODEV so isn't
caught by the handling and I think it should be.

>  		if (ret)
> -			return ret;
> -
> -		ret = regulator_get_voltage(aincom);
> -		if (ret < 0)
>  			return dev_err_probe(&spi->dev, ret,
> -					     "Device tree error, AINCOM voltage undefined\n");
> -		st->aincom_mv = ret / MILLI;
> -	}
> +					     "Failed to enable AVDD supply\n");
>  
> -	st->avdd = devm_regulator_get(&spi->dev, "avdd");
> -	if (IS_ERR(st->avdd))
> -		return PTR_ERR(st->avdd);
> -
> -	ret = regulator_enable(st->avdd);
> -	if (ret) {
> -		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
> -		return ret;
> +		avdd_mv = 0;
> +	} else if (ret < 0) {
> +		return dev_err_probe(&spi->dev, ret, "Failed to get AVDD voltage\n");
> +	} else {
> +		avdd_mv = ret / MILLI;
>  	}
>  
> -	ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->avdd);
> -	if (ret)
> -		return ret;
>  
>  	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
>  	if (ret)
>  		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
>  
> -	st->vref = devm_regulator_get_optional(&spi->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(&spi->dev, ret,
> -					     "Device tree error, AVdd 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(&spi->dev, "vref");
> +	if (ret == -ENODEV) {
> +		if (avdd_mv == 0)
> +			return dev_err_probe(&spi->dev, -ENODEV,
> +					     "No reference voltage available\n");
> +
> +		st->int_vref_mv = avdd_mv;
> +	} else if (ret < 0) {
> +		return ret;
>  	} else {
> -		ret = regulator_enable(st->vref);
> -		if (ret) {
> -			dev_err(&spi->dev, "Failed to enable specified Vref supply\n");
> -			return ret;
> -		}
> -
> -		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
> -		if (ret)
> -			return ret;
> -
> -		ret = regulator_get_voltage(st->vref);
> -		if (ret < 0)
> -			return dev_err_probe(&spi->dev, ret,
> -					     "Device tree error, Vref voltage undefined\n");
> +		st->int_vref_mv = ret / MILLI;
>  	}
> -	st->int_vref_mv = ret / 1000;
>  
>  	st->chip_info = spi_get_device_match_data(spi);
>  	indio_dev->name = st->chip_info->name;
> 

Powered by blists - more mailing lists