linux-kernel - Re: [PATCH 05/11] iio: dac: ad7293: use devm_regulator_get_enable_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241123155043.45a4313b@jic23-huawei>
Date: Sat, 23 Nov 2024 15:50:43 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Antoniu Miclaus
 <antoniu.miclaus@...log.com>, Nuno Sá <nuno.sa@...log.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 05/11] iio: dac: ad7293: use
 devm_regulator_get_enable_read_voltage()

On Wed, 20 Nov 2024 15:33:28 -0600
David Lechner <dlechner@...libre.com> wrote:

> Simplify the code by using devm_regulator_get_enable_read_voltage().
> 
> A small change in behavior here due to moving the enable to the same
> place as the get: supplies now are enabled before the reset GPIO is
> toggled (which is arguably the correct order).
Agreed ordering is weird before this change so that should be fine.
> 
> Signed-off-by: David Lechner <dlechner@...libre.com>

This raises some questions for me (about the original code)
My gut feeling is drop the reading of the regulator voltage but
maybe I'm missing something.

> ---
>  drivers/iio/dac/ad7293.c | 66 ++++++++----------------------------------------
>  1 file changed, 11 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad7293.c b/drivers/iio/dac/ad7293.c
> index 1d4032670482..58f7926ec3f3 100644
> --- a/drivers/iio/dac/ad7293.c
> +++ b/drivers/iio/dac/ad7293.c
> @@ -141,8 +141,6 @@ struct ad7293_state {
>  	/* Protect against concurrent accesses to the device, page selection and data content */
>  	struct mutex lock;
>  	struct gpio_desc *gpio_reset;
> -	struct regulator *reg_avdd;
> -	struct regulator *reg_vdrive;
>  	u8 page_select;
>  	u8 data[3] __aligned(IIO_DMA_MINALIGN);
>  };
> @@ -777,6 +775,7 @@ static int ad7293_reset(struct ad7293_state *st)
>  static int ad7293_properties_parse(struct ad7293_state *st)
>  {
>  	struct spi_device *spi = st->spi;
> +	int ret;
>  
>  	st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset",
>  						 GPIOD_OUT_HIGH);
> @@ -784,24 +783,23 @@ static int ad7293_properties_parse(struct ad7293_state *st)
>  		return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_reset),
>  				     "failed to get the reset GPIO\n");
>  
> -	st->reg_avdd = devm_regulator_get(&spi->dev, "avdd");
> -	if (IS_ERR(st->reg_avdd))
> -		return dev_err_probe(&spi->dev, PTR_ERR(st->reg_avdd),
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "avdd");
> +	if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret,
>  				     "failed to get the AVDD voltage\n");
> +	if (ret > 5500000 || ret < 4500000)
> +		return -EINVAL;

Why is this a driver problem?


>  
> -	st->reg_vdrive = devm_regulator_get(&spi->dev, "vdrive");
> -	if (IS_ERR(st->reg_vdrive))
> -		return dev_err_probe(&spi->dev, PTR_ERR(st->reg_vdrive),
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vdrive");
> +	if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret,
>  				     "failed to get the VDRIVE voltage\n");
> +	if (ret > 5500000 || ret < 1700000)
> +		return -EINVAL;
Likewise.

Regulators might be fine and we can't read the voltages. If they are out
of spec, that's a design or board configuration problem.

To keep these in place rather than just dropping them, I'd like more
info on why!  May well be in the history but I'm busy / lazy  (take
your pick) so won't look today.



>  	return 0;
>  }
>  
> -static void ad7293_reg_disable(void *data)
> -{
> -	regulator_disable(data);
> -}
> -
>  static int ad7293_init(struct ad7293_state *st)
>  {
>  	int ret;
> @@ -816,48 +814,6 @@ static int ad7293_init(struct ad7293_state *st)
>  	if (ret)
>  		return ret;
>  
> -	ret = regulator_enable(st->reg_avdd);
> -	if (ret) {
> -		dev_err(&spi->dev,
> -			"Failed to enable specified AVDD Voltage!\n");
> -		return ret;
> -	}
> -
> -	ret = devm_add_action_or_reset(&spi->dev, ad7293_reg_disable,
> -				       st->reg_avdd);
> -	if (ret)
> -		return ret;
> -
> -	ret = regulator_enable(st->reg_vdrive);
> -	if (ret) {
> -		dev_err(&spi->dev,
> -			"Failed to enable specified VDRIVE Voltage!\n");
> -		return ret;
> -	}
> -
> -	ret = devm_add_action_or_reset(&spi->dev, ad7293_reg_disable,
> -				       st->reg_vdrive);
> -	if (ret)
> -		return ret;
> -
> -	ret = regulator_get_voltage(st->reg_avdd);
> -	if (ret < 0) {
> -		dev_err(&spi->dev, "Failed to read avdd regulator: %d\n", ret);
> -		return ret;
> -	}
> -
> -	if (ret > 5500000 || ret < 4500000)
> -		return -EINVAL;
> -
> -	ret = regulator_get_voltage(st->reg_vdrive);
> -	if (ret < 0) {
> -		dev_err(&spi->dev,
> -			"Failed to read vdrive regulator: %d\n", ret);
> -		return ret;
> -	}
> -	if (ret > 5500000 || ret < 1700000)
> -		return -EINVAL;
> -
>  	/* Check Chip ID */
>  	ret = __ad7293_spi_read(st, AD7293_REG_DEVICE_ID, &chip_id);
>  	if (ret)
>