diff mbox series

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

Message ID 20241216-iio-regulator-cleanup-round-6-v2-1-9482164b68cb@baylibre.com (mailing list archive)
State Accepted
Headers show
Series [v2] iio: dac: ad7293: enable power before reset | expand

Commit Message

David Lechner Dec. 16, 2024, 9:44 p.m. UTC 
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@baylibre.com>
---
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@baylibre.com
---
 drivers/iio/dac/ad7293.c | 68 +++++++-----------------------------------------
 1 file changed, 9 insertions(+), 59 deletions(-)


---
base-commit: 01958cb8a00d9721ae56ad1eef9cd7b22b5a34bb
change-id: 20241120-iio-regulator-cleanup-round-6-78b05be06718

Best regards,

Comments

Nuno Sá Dec. 17, 2024, 10:05 a.m. UTC | #1 
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@baylibre.com>
> ---

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

> 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@baylibre.com
> ---
>  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,
Jonathan Cameron Dec. 19, 2024, 4:37 p.m. UTC | #2 
On Tue, 17 Dec 2024 11:05:38 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> 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@baylibre.com>
> > ---  
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Applied. Thanks
diff mbox series

Patch

diff --git a/drivers/iio/dac/ad7293.c b/drivers/iio/dac/ad7293.c
index 1d403267048240be1bf3d8b2a59685409b9087fd..d3f49b5337d2f512363d50b434d99d4e9b95059f 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)