Re: [PATCH v2] iio: dac: ad7293: enable power before reset

[Nuno Sá]

On Mon, 2024-12-16 at 15:44 -0600, David Lechner wrote:
> Change the order of regulator enable and reset so that power supplies
> are turned on before touching the reset line. Generally, chips should
> have the VDRIVE supply enabled before applying voltage on any pins.
> 
> While we are at it, remove the voltage level checks. If the power
> supplies are not supplying the correct voltage, this is a hardware
> design problem, not a software problem.
> 
> Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
> ---

Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>

> Changes in v2:
> - Dropped patches from "iio: use devm_regulator_get_enable_read_voltage
>   round 6" that have already been applied.
> - New patch for ad7293 that just enables power supplies and no longer
>   reads the voltage.
> - Link to v1:
> https://lore.kernel.org/r/20241120-iio-regulator-cleanup-round-6-v1-0-d5a5360f7ec3@xxxxxxxxxxxx
> ---
>  drivers/iio/dac/ad7293.c | 68 +++++++----------------------------------------
> -
>  1 file changed, 9 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad7293.c b/drivers/iio/dac/ad7293.c
> index
> 1d403267048240be1bf3d8b2a59685409b9087fd..d3f49b5337d2f512363d50b434d99d4e9b95
> 059f 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,15 @@ 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;
> +
> +	ret = devm_regulator_get_enable(&spi->dev, "avdd");
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "failed to enable
> AVDD\n");
> +
> +	ret = devm_regulator_get_enable(&spi->dev, "vdrive");
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "failed to enable
> VDRIVE\n");
>  
>  	st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset",
>  						 GPIOD_OUT_HIGH);
> @@ -784,24 +791,9 @@ 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),
> -				     "failed to get the AVDD voltage\n");
> -
> -	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),
> -				     "failed to get the VDRIVE voltage\n");
> -
>  	return 0;
>  }
>  
> -static void ad7293_reg_disable(void *data)
> -{
> -	regulator_disable(data);
> -}
> -
>  static int ad7293_init(struct ad7293_state *st)
>  {
>  	int ret;
> @@ -816,48 +808,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)
> 
> ---
> base-commit: 01958cb8a00d9721ae56ad1eef9cd7b22b5a34bb
> change-id: 20241120-iio-regulator-cleanup-round-6-78b05be06718
> 
> Best regards,