[PATCH 1/3] iio:ad7266: Fix broken regulator error handling

[PATCH 1/3] iio:ad7266: Fix broken regulator error handling

[Mark Brown]

All regulator_get() variants return either a pointer to a regulator or an
ERR_PTR() so testing for NULL makes no sense and may lead to bugs if we
use NULL as a valid regulator. Fix this by using IS_ERR() as expected.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/iio/adc/ad7266.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index 21e19b60e2b9..835d45db258f 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -397,7 +397,7 @@ static int ad7266_probe(struct spi_device *spi)
 	st = iio_priv(indio_dev);
 
 	st->reg = devm_regulator_get(&spi->dev, "vref");
-	if (!IS_ERR_OR_NULL(st->reg)) {
+	if (!IS_ERR(st->reg)) {
 		ret = regulator_enable(st->reg);
 		if (ret)
 			return ret;
-- 
2.8.1

[PATCH 2/3] iio:ad7266: Fix support for optional regulators

[Mark Brown]

The ad7266 driver attempts to support deciding between the use of internal
and external power supplies by checking to see if an error is returned when
requesting the regulator. This doesn't work with the current code since the
driver uses a normal regulator_get() which is for non-optional supplies
and so assumes that if a regulator is not provided by the platform then
this is a bug in the platform integration and so substitutes a dummy
regulator. Use regulator_get_optional() instead which indicates to the
framework that the regulator may be absent and provides a dummy regulator
instead.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/iio/adc/ad7266.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index 835d45db258f..655b36b4e9cb 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -396,7 +396,7 @@ static int ad7266_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 
-	st->reg = devm_regulator_get(&spi->dev, "vref");
+	st->reg = devm_regulator_get_optional(&spi->dev, "vref");
 	if (!IS_ERR(st->reg)) {
 		ret = regulator_enable(st->reg);
 		if (ret)
-- 
2.8.1

[PATCH 3/3] iio:ad7266: Fix probe deferral for vref

[Mark Brown]

Currently the ad7266 driver treats any failure to get vref as though the
regulator were not present but this means that if probe deferral is
triggered the driver will act as though the regulator were not present.
Instead only use the internal reference if we explicitly got -ENODEV which
is what is returned for absent regulators.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/iio/adc/ad7266.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index 655b36b4e9cb..2123f0ac2e2a 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -408,6 +408,9 @@ static int ad7266_probe(struct spi_device *spi)
 
 		st->vref_mv = ret / 1000;
 	} else {
+		/* Any other error indicates that the regulator does exist */
+		if (PTR_ERR(st->reg) != -ENODEV)
+			return PTR_ERR(st->reg);
 		/* Use internal reference */
 		st->vref_mv = 2500;
 	}
-- 
2.8.1

Re: [PATCH 1/3] iio:ad7266: Fix broken regulator error handling

[Jonathan Cameron]

On 20/06/16 13:53, Mark Brown wrote:
> All regulator_get() variants return either a pointer to a regulator or an
> ERR_PTR() so testing for NULL makes no sense and may lead to bugs if we
> use NULL as a valid regulator. Fix this by using IS_ERR() as expected.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks Mark,

Jonathan
> ---
>  drivers/iio/adc/ad7266.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> index 21e19b60e2b9..835d45db258f 100644
> --- a/drivers/iio/adc/ad7266.c
> +++ b/drivers/iio/adc/ad7266.c
> @@ -397,7 +397,7 @@ static int ad7266_probe(struct spi_device *spi)
>  	st = iio_priv(indio_dev);
>  
>  	st->reg = devm_regulator_get(&spi->dev, "vref");
> -	if (!IS_ERR_OR_NULL(st->reg)) {
> +	if (!IS_ERR(st->reg)) {
>  		ret = regulator_enable(st->reg);
>  		if (ret)
>  			return ret;
> 

Re: [PATCH 2/3] iio:ad7266: Fix support for optional regulators

[Jonathan Cameron]

On 20/06/16 13:53, Mark Brown wrote:
> The ad7266 driver attempts to support deciding between the use of internal
> and external power supplies by checking to see if an error is returned when
> requesting the regulator. This doesn't work with the current code since the
> driver uses a normal regulator_get() which is for non-optional supplies
> and so assumes that if a regulator is not provided by the platform then
> this is a bug in the platform integration and so substitutes a dummy
> regulator. Use regulator_get_optional() instead which indicates to the
> framework that the regulator may be absent and provides a dummy regulator
> instead.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ad7266.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> index 835d45db258f..655b36b4e9cb 100644
> --- a/drivers/iio/adc/ad7266.c
> +++ b/drivers/iio/adc/ad7266.c
> @@ -396,7 +396,7 @@ static int ad7266_probe(struct spi_device *spi)
>  
>  	st = iio_priv(indio_dev);
>  
> -	st->reg = devm_regulator_get(&spi->dev, "vref");
> +	st->reg = devm_regulator_get_optional(&spi->dev, "vref");
>  	if (!IS_ERR(st->reg)) {
>  		ret = regulator_enable(st->reg);
>  		if (ret)
> 

Re: [PATCH 3/3] iio:ad7266: Fix probe deferral for vref

[Jonathan Cameron]

On 20/06/16 13:53, Mark Brown wrote:
> Currently the ad7266 driver treats any failure to get vref as though the
> regulator were not present but this means that if probe deferral is
> triggered the driver will act as though the regulator were not present.
> Instead only use the internal reference if we explicitly got -ENODEV which
> is what is returned for absent regulators.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Lars, these all seems obviously correct to me, but I'll hold that
tree for a day or two so still time to comment.

I dread to think how many other missuses we have of these interfaces
in the tree...

Jonathan
> ---
>  drivers/iio/adc/ad7266.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> index 655b36b4e9cb..2123f0ac2e2a 100644
> --- a/drivers/iio/adc/ad7266.c
> +++ b/drivers/iio/adc/ad7266.c
> @@ -408,6 +408,9 @@ static int ad7266_probe(struct spi_device *spi)
>  
>  		st->vref_mv = ret / 1000;
>  	} else {
> +		/* Any other error indicates that the regulator does exist */
> +		if (PTR_ERR(st->reg) != -ENODEV)
> +			return PTR_ERR(st->reg);
>  		/* Use internal reference */
>  		st->vref_mv = 2500;
>  	}
> 

Re: [PATCH 3/3] iio:ad7266: Fix probe deferral for vref

[Linus Walleij]

On Sun, Jun 26, 2016 at 6:41 PM, Jonathan Cameron <jic23@kernel.org> wrote:

> I dread to think how many other missuses we have of these interfaces
> in the tree...

I have agreed with Mark to take a sweep with the broomstick
across IIO and make sure we get this right everywhere here.

Yours,
Linus Walleij