iio: fetch and enable regulators unconditionally

PATCH Superseded Crt Mori 2 participants 4 messages Sep 2, 2016

Subsystems (3)

Arm i.MX and Layerscape IIO Analog Devices IIO drivers

lore

Share & tools

lore

Crt Mori

author

2016-09-02 13:28 UTC #0 lore raw

PATCH 1/1 iio: fetch and enable regulators unconditionally

This patch is inspired by a comment of Jonathan Cameron on patch of
Linus Walleij commit aeb55fff3891834e07a3144159a7298a19696af8 ("iio: st_sensors: fetch and enable regulators unconditionally").

The explanation for this change is same as in that patch:
"Supplies are *not* optional (optional means that the supply is optional in the electrical sense, not the software sense) so we need to get the and enable them at all times.

If the device tree or board file does not define suitable regulators for the component, it will be substituted by a dummy regulator, or, if regulators are disabled altogether, by stubs. There is no need to use the IS_ERR_OR_NULL() check that is considered harmful.

Reported-by Linus Wallerij <[email protected]>

Signed-off-by Crt Mori <[email protected]>

---
 drivers/iio/adc/ad7266.c           | 8 +++-----
 drivers/iio/adc/fsl-imx25-gcq.c    | 6 +++---
 drivers/iio/adc/ti-ads8688.c       | 9 +++------
 drivers/iio/dac/ad5761.c           | 9 +++------
 drivers/iio/pressure/ms5611_core.c | 6 ++----
 5 files changed, 14 insertions(+), 24 deletions(-)

drivers/iio/adc/ad7266.c +3 -5

index c0f6a98..196b29a 100644
@@ -395,7 +395,7 @@ static int ad7266_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 
-	st->reg = devm_regulator_get_optional(&spi->dev, "vref");
+	st->reg = devm_regulator_get(&spi->dev, "vref");
 	if (!IS_ERR(st->reg)) {
 		ret = regulator_enable(st->reg);
 		if (ret)

@@ -481,8 +481,7 @@ error_free_gpios:
 	if (!st->fixed_addr)
 		gpio_free_array(st->gpios, ARRAY_SIZE(st->gpios));
 error_disable_reg:
-	if (!IS_ERR_OR_NULL(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->reg);
 
 	return ret;
 }

@@ -496,8 +495,7 @@ static int ad7266_remove(struct spi_device *spi)
 	iio_triggered_buffer_cleanup(indio_dev);
 	if (!st->fixed_addr)
 		gpio_free_array(st->gpios, ARRAY_SIZE(st->gpios));
-	if (!IS_ERR_OR_NULL(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->reg);
 
 	return 0;
 }

drivers/iio/adc/fsl-imx25-gcq.c +3 -3

index 72b32c1..8aaf026 100644
@@ -195,11 +195,11 @@ static int mx25_gcq_setup_cfgs(struct platform_device *pdev,
 	 */
 	priv->vref[MX25_ADC_REFP_INT] = NULL;
 	priv->vref[MX25_ADC_REFP_EXT] =
-		devm_regulator_get_optional(&pdev->dev, "vref-ext");
+		devm_regulator_get(&pdev->dev, "vref-ext");
 	priv->vref[MX25_ADC_REFP_XP] =
-		devm_regulator_get_optional(&pdev->dev, "vref-xp");
+		devm_regulator_get(&pdev->dev, "vref-xp");
 	priv->vref[MX25_ADC_REFP_YP] =
-		devm_regulator_get_optional(&pdev->dev, "vref-yp");
+		devm_regulator_get(&pdev->dev, "vref-yp");
 
 	for_each_child_of_node(np, child) {
 		u32 reg;

drivers/iio/adc/ti-ads8688.c +3 -6

index c400439..39bc9e4 100644
@@ -395,7 +395,7 @@ static int ads8688_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 
-	st->reg = devm_regulator_get_optional(&spi->dev, "vref");
+	st->reg = devm_regulator_get(&spi->dev, "vref");
 	if (!IS_ERR(st->reg)) {
 		ret = regulator_enable(st->reg);
 		if (ret)

@@ -438,9 +438,7 @@ static int ads8688_probe(struct spi_device *spi)
 	return 0;
 
 error_out:
-	if (!IS_ERR_OR_NULL(st->reg))
-		regulator_disable(st->reg);
-
+	regulator_disable(st->reg);
 	return ret;
 }

@@ -451,8 +449,7 @@ static int ads8688_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 
-	if (!IS_ERR_OR_NULL(st->reg))
-		regulator_disable(st->reg);
+	regulator_disable(st->reg);
 
 	return 0;
 }

drivers/iio/dac/ad5761.c +3 -6

index d6510d6..70d88c4 100644
@@ -292,7 +292,7 @@ static int ad5761_get_vref(struct ad5761_state *st,
 {
 	int ret;
 
-	st->vref_reg = devm_regulator_get_optional(&st->spi->dev, "vref");
+	st->vref_reg = devm_regulator_get(&st->spi->dev, "vref");
 	if (PTR_ERR(st->vref_reg) == -ENODEV) {
 		/* Use Internal regulator */
 		if (!chip_info->int_vref) {

@@ -387,9 +387,7 @@ static int ad5761_probe(struct spi_device *spi)
 	return 0;
 
 disable_regulator_err:
-	if (!IS_ERR_OR_NULL(st->vref_reg))
-		regulator_disable(st->vref_reg);
-
+	regulator_disable(st->vref_reg);
 	return ret;
 }

@@ -400,8 +398,7 @@ static int ad5761_remove(struct spi_device *spi)
 
 	iio_device_unregister(iio_dev);
 
-	if (!IS_ERR_OR_NULL(st->vref_reg))
-		regulator_disable(st->vref_reg);
+	regulator_disable(st->vref_reg);
 
 	return 0;
 }

drivers/iio/pressure/ms5611_core.c +2 -4

index feb41f8..a74ed1f 100644
@@ -416,8 +416,7 @@ static int ms5611_init(struct iio_dev *indio_dev)
 	return 0;
 
 err_regulator_disable:
-	if (!IS_ERR_OR_NULL(st->vdd))
-		regulator_disable(st->vdd);
+	regulator_disable(st->vdd);
 	return ret;
 }

@@ -425,8 +424,7 @@ static void ms5611_fini(const struct iio_dev *indio_dev)
 {
 	const struct ms5611_state *st = iio_priv(indio_dev);
 
-	if (!IS_ERR_OR_NULL(st->vdd))
-		regulator_disable(st->vdd);
+	regulator_disable(st->vdd);
 }
 
 int ms5611_probe(struct iio_dev *indio_dev, struct device *dev,

Signature · 1 line

2.9.3

Lars-Peter Clausen

2016-09-02 13:40 UTC #1 lore raw

Hi,

Thanks for the patch.

Quoted 7 lines · Crt Mori

This patch is inspired by a comment of Jonathan Cameron on patch of
Linus Walleij commit aeb55fff3891834e07a3144159a7298a19696af8 ("iio: st_sensors: fetch and enable regulators unconditionally").
The explanation for this change is same as in that patch:
"Supplies are *not* optional (optional means that the supply is optional in the electrical sense, not the software sense) so we need to get the and enable them at all times.

Some supplies are optional. I.e. the part is able to operate (potentially with limited functionality) even if the optional external supply has not been connected to the pin.

E.g. many converters have a built-in voltage reference but are able to optionally accept an external reference. The driver needs to know if the external reference is not connected or not to be able to allow or disallow applications to switch the part into external reference mode. This is the case for the ad7266 and ad5761 drivers and using regulator_get_optional() is correct here.

Same seems to be true for the ads8688 and the fsl-imx-gcq, judging by how their code is structured.

- Lars

Crt Mori

author

2016-09-02 13:48 UTC #2 lore raw

Quoted 24 lines · Lars-Peter Clausen

Hi,
Thanks for the patch.
Quoted 7 lines · Crt Mori
This patch is inspired by a comment of Jonathan Cameron on patch of
Linus Walleij commit aeb55fff3891834e07a3144159a7298a19696af8 ("iio: st_sensors: fetch and enable regulators unconditionally").
The explanation for this change is same as in that patch:
"Supplies are *not* optional (optional means that the supply is optional in the electrical sense, not the software sense) so we need to get the and enable them at all times.
Some supplies are optional. I.e. the part is able to operate (potentially with limited functionality) even if the optional external supply has not been connected to the pin.
E.g. many converters have a built-in voltage reference but are able to optionally accept an external reference. The driver needs to know if the external reference is not connected or not to be able to allow or disallow applications to switch the part into external reference mode. This is the case for the ad7266 and ad5761 drivers and using regulator_get_optional() is correct here.

Quoted 4 lines

Same seems to be true for the ads8688 and the fsl-imx-gcq, judging by how their code is structured.
- Lars

OK, so the voltage reference part can have optional regulator, but if it is supply voltage then it cannot.

What about the IS_ERR_OR_NULL() check? Is that suitable for the optional regulators as well?

Lars-Peter Clausen

2016-09-02 14:48 UTC #3 lore raw

Quoted 28 lines · Crt Mori

Quoted 27 lines · Lars-Peter Clausen
Hi,
Thanks for the patch.
Quoted 7 lines · Crt Mori
This patch is inspired by a comment of Jonathan Cameron on patch of
Linus Walleij commit aeb55fff3891834e07a3144159a7298a19696af8 ("iio: st_sensors: fetch and enable regulators unconditionally").
The explanation for this change is same as in that patch:
"Supplies are *not* optional (optional means that the supply is optional in the electrical sense, not the software sense) so we need to get the and enable them at all times.
Some supplies are optional. I.e. the part is able to operate (potentially with limited functionality) even if the optional external supply has not been connected to the pin.
E.g. many converters have a built-in voltage reference but are able to optionally accept an external reference. The driver needs to know if the external reference is not connected or not to be able to allow or disallow applications to switch the part into external reference mode. This is the case for the ad7266 and ad5761 drivers and using regulator_get_optional() is correct here.
Same seems to be true for the ads8688 and the fsl-imx-gcq, judging by how their code is structured.

Quoted 6 lines

Quoted 1 line
- Lars
OK, so the voltage reference part can have optional regulator, but if it is supply voltage then it cannot.
What about the IS_ERR_OR_NULL() check? Is that suitable for the optional regulators as well?

I think regulator_get_optional() should never return NULL, so replacing them with just IS_ERR() should be OK. But please check that the driver itself does not set the regulator explicitly to NULL.

- Lars