[PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[Andy Shevchenko]

Instead of checking for the specific error codes (that can be considered
a layering violation to some extent) check for the property existence first
and then either parse it, or apply a default value.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/adc/ad7191.c | 63 +++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
index d9cd903ffdd2..51ec199fb06f 100644
--- a/drivers/iio/adc/ad7191.c
+++ b/drivers/iio/adc/ad7191.c
@@ -154,27 +154,18 @@ static int ad7191_config_setup(struct iio_dev *indio_dev)
 	const u32 gain[4] = { 1, 8, 64, 128 };
 	static u32 scale_buffer[4][2];
 	int odr_value, odr_index = 0, pga_value, pga_index = 0, i, ret;
+	const char *propname;
 	u64 scale_uv;
 
 	st->samp_freq_index = 0;
 	st->scale_index = 0;
 
-	ret = device_property_read_u32(dev, "adi,odr-value", &odr_value);
-	if (ret && ret != -EINVAL)
-		return dev_err_probe(dev, ret, "Failed to get odr value.\n");
+	propname = "adi,odr-value";
+	if (device_property_present(dev, propname)) {
+		ret = device_property_read_u32(dev, propname, &odr_value);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to get %s.\n", propname);
 
-	if (ret == -EINVAL) {
-		st->odr_gpios = devm_gpiod_get_array(dev, "odr", GPIOD_OUT_LOW);
-		if (IS_ERR(st->odr_gpios))
-			return dev_err_probe(dev, PTR_ERR(st->odr_gpios),
-					     "Failed to get odr gpios.\n");
-
-		if (st->odr_gpios->ndescs != 2)
-			return dev_err_probe(dev, -EINVAL, "Expected 2 odr gpio pins.\n");
-
-		st->samp_freq_avail = samp_freq;
-		st->samp_freq_avail_size = ARRAY_SIZE(samp_freq);
-	} else {
 		for (i = 0; i < ARRAY_SIZE(samp_freq); i++) {
 			if (odr_value != samp_freq[i])
 				continue;
@@ -186,6 +177,17 @@ static int ad7191_config_setup(struct iio_dev *indio_dev)
 		st->samp_freq_avail_size = 1;
 
 		st->odr_gpios = NULL;
+	} else {
+		st->odr_gpios = devm_gpiod_get_array(dev, "odr", GPIOD_OUT_LOW);
+		if (IS_ERR(st->odr_gpios))
+			return dev_err_probe(dev, PTR_ERR(st->odr_gpios),
+					     "Failed to get odr gpios.\n");
+
+		if (st->odr_gpios->ndescs != 2)
+			return dev_err_probe(dev, -EINVAL, "Expected 2 odr gpio pins.\n");
+
+		st->samp_freq_avail = samp_freq;
+		st->samp_freq_avail_size = ARRAY_SIZE(samp_freq);
 	}
 
 	mutex_lock(&st->lock);
@@ -200,22 +202,12 @@ static int ad7191_config_setup(struct iio_dev *indio_dev)
 
 	mutex_unlock(&st->lock);
 
-	ret = device_property_read_u32(dev, "adi,pga-value", &pga_value);
-	if (ret && ret != -EINVAL)
-		return dev_err_probe(dev, ret, "Failed to get pga value.\n");
+	propname = "adi,pga-value";
+	if (device_property_present(dev, propname)) {
+		ret = device_property_read_u32(dev, propname, &pga_value);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to get %s.\n", propname);
 
-	if (ret == -EINVAL) {
-		st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
-		if (IS_ERR(st->pga_gpios))
-			return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
-					     "Failed to get pga gpios.\n");
-
-		if (st->pga_gpios->ndescs != 2)
-			return dev_err_probe(dev, -EINVAL, "Expected 2 pga gpio pins.\n");
-
-		st->scale_avail = scale_buffer;
-		st->scale_avail_size = ARRAY_SIZE(scale_buffer);
-	} else {
 		for (i = 0; i < ARRAY_SIZE(gain); i++) {
 			if (pga_value != gain[i])
 				continue;
@@ -227,6 +219,17 @@ static int ad7191_config_setup(struct iio_dev *indio_dev)
 		st->scale_avail_size = 1;
 
 		st->pga_gpios = NULL;
+	} else {
+		st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
+		if (IS_ERR(st->pga_gpios))
+			return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
+					     "Failed to get pga gpios.\n");
+
+		if (st->pga_gpios->ndescs != 2)
+			return dev_err_probe(dev, -EINVAL, "Expected 2 pga gpio pins.\n");
+
+		st->scale_avail = scale_buffer;
+		st->scale_avail_size = ARRAY_SIZE(scale_buffer);
 	}
 
 	st->temp_gpio = devm_gpiod_get(dev, "temp", GPIOD_OUT_LOW);
-- 
2.50.1

Re: [PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[Nuno Sá]

On Thu, 2026-02-19 at 15:39 +0100, Andy Shevchenko wrote:
> Instead of checking for the specific error codes (that can be considered
> a layering violation to some extent) check for the property existence first
> and then either parse it, or apply a default value.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

Not really sure how I feel about this one. Checking for specific errors is a very common
pattern and this change just makes it we check for the property presence twice. That said,
this makes it more "future proof" (though I find it very unlikely for ret value o change).

Anyways, even if we choose to go down this route, I don't see much benefit in starting
converting the drivers with the pattern below (which should be a considerable number).

- Nuno Sá
 
>  drivers/iio/adc/ad7191.c | 63 +++++++++++++++++++++-------------------
>  1 file changed, 33 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7191.c b/drivers/iio/adc/ad7191.c
> index d9cd903ffdd2..51ec199fb06f 100644
> --- a/drivers/iio/adc/ad7191.c
> +++ b/drivers/iio/adc/ad7191.c
> @@ -154,27 +154,18 @@ static int ad7191_config_setup(struct iio_dev *indio_dev)
>  	const u32 gain[4] = { 1, 8, 64, 128 };
>  	static u32 scale_buffer[4][2];
>  	int odr_value, odr_index = 0, pga_value, pga_index = 0, i, ret;
> +	const char *propname;
>  	u64 scale_uv;
>  
>  	st->samp_freq_index = 0;
>  	st->scale_index = 0;
>  
> -	ret = device_property_read_u32(dev, "adi,odr-value", &odr_value);
> -	if (ret && ret != -EINVAL)
> -		return dev_err_probe(dev, ret, "Failed to get odr value.\n");
> +	propname = "adi,odr-value";
> +	if (device_property_present(dev, propname)) {
> +		ret = device_property_read_u32(dev, propname, &odr_value);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to get %s.\n", propname);
>  
> -	if (ret == -EINVAL) {
> -		st->odr_gpios = devm_gpiod_get_array(dev, "odr", GPIOD_OUT_LOW);
> -		if (IS_ERR(st->odr_gpios))
> -			return dev_err_probe(dev, PTR_ERR(st->odr_gpios),
> -					     "Failed to get odr gpios.\n");
> -
> -		if (st->odr_gpios->ndescs != 2)
> -			return dev_err_probe(dev, -EINVAL, "Expected 2 odr gpio pins.\n");
> -
> -		st->samp_freq_avail = samp_freq;
> -		st->samp_freq_avail_size = ARRAY_SIZE(samp_freq);
> -	} else {
>  		for (i = 0; i < ARRAY_SIZE(samp_freq); i++) {
>  			if (odr_value != samp_freq[i])
>  				continue;
> @@ -186,6 +177,17 @@ static int ad7191_config_setup(struct iio_dev *indio_dev)
>  		st->samp_freq_avail_size = 1;
>  
>  		st->odr_gpios = NULL;
> +	} else {
> +		st->odr_gpios = devm_gpiod_get_array(dev, "odr", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->odr_gpios))
> +			return dev_err_probe(dev, PTR_ERR(st->odr_gpios),
> +					     "Failed to get odr gpios.\n");
> +
> +		if (st->odr_gpios->ndescs != 2)
> +			return dev_err_probe(dev, -EINVAL, "Expected 2 odr gpio pins.\n");
> +
> +		st->samp_freq_avail = samp_freq;
> +		st->samp_freq_avail_size = ARRAY_SIZE(samp_freq);
>  	}
>  
>  	mutex_lock(&st->lock);
> @@ -200,22 +202,12 @@ static int ad7191_config_setup(struct iio_dev *indio_dev)
>  
>  	mutex_unlock(&st->lock);
>  
> -	ret = device_property_read_u32(dev, "adi,pga-value", &pga_value);
> -	if (ret && ret != -EINVAL)
> -		return dev_err_probe(dev, ret, "Failed to get pga value.\n");
> +	propname = "adi,pga-value";
> +	if (device_property_present(dev, propname)) {
> +		ret = device_property_read_u32(dev, propname, &pga_value);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to get %s.\n", propname);
>  
> -	if (ret == -EINVAL) {
> -		st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
> -		if (IS_ERR(st->pga_gpios))
> -			return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
> -					     "Failed to get pga gpios.\n");
> -
> -		if (st->pga_gpios->ndescs != 2)
> -			return dev_err_probe(dev, -EINVAL, "Expected 2 pga gpio pins.\n");
> -
> -		st->scale_avail = scale_buffer;
> -		st->scale_avail_size = ARRAY_SIZE(scale_buffer);
> -	} else {
>  		for (i = 0; i < ARRAY_SIZE(gain); i++) {
>  			if (pga_value != gain[i])
>  				continue;
> @@ -227,6 +219,17 @@ static int ad7191_config_setup(struct iio_dev *indio_dev)
>  		st->scale_avail_size = 1;
>  
>  		st->pga_gpios = NULL;
> +	} else {
> +		st->pga_gpios = devm_gpiod_get_array(dev, "pga", GPIOD_OUT_LOW);
> +		if (IS_ERR(st->pga_gpios))
> +			return dev_err_probe(dev, PTR_ERR(st->pga_gpios),
> +					     "Failed to get pga gpios.\n");
> +
> +		if (st->pga_gpios->ndescs != 2)
> +			return dev_err_probe(dev, -EINVAL, "Expected 2 pga gpio pins.\n");
> +
> +		st->scale_avail = scale_buffer;
> +		st->scale_avail_size = ARRAY_SIZE(scale_buffer);
>  	}
>  
>  	st->temp_gpio = devm_gpiod_get(dev, "temp", GPIOD_OUT_LOW);

Re: [PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[Andy Shevchenko]

On Fri, Feb 20, 2026 at 12:04 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Thu, 2026-02-19 at 15:39 +0100, Andy Shevchenko wrote:
> > Instead of checking for the specific error codes (that can be considered
> > a layering violation to some extent) check for the property existence first
> > and then either parse it, or apply a default value.
>
> Not really sure how I feel about this one. Checking for specific errors is a very common
> pattern and this change just makes it we check for the property presence twice. That said,
> this makes it more "future proof" (though I find it very unlikely for ret value o change).

I already have an answer to this:
https://lore.kernel.org/r/aZcenabXYsOdBu84@smile.fi.intel.com

> Anyways, even if we choose to go down this route, I don't see much benefit in starting
> converting the drivers with the pattern below (which should be a considerable number).

There is not a big number of them, so I prefer to have common patterns
without exact error code checks.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[Andy Shevchenko]

On Fri, Feb 20, 2026 at 12:33:28PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 20, 2026 at 12:04 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > On Thu, 2026-02-19 at 15:39 +0100, Andy Shevchenko wrote:
> > > Instead of checking for the specific error codes (that can be considered
> > > a layering violation to some extent) check for the property existence first
> > > and then either parse it, or apply a default value.
> >
> > Not really sure how I feel about this one. Checking for specific errors is a very common
> > pattern and this change just makes it we check for the property presence twice. That said,
> > this makes it more "future proof" (though I find it very unlikely for ret value o change).
> 
> I already have an answer to this:
> https://lore.kernel.org/r/aZcenabXYsOdBu84@smile.fi.intel.com

Does it help?

> > Anyways, even if we choose to go down this route, I don't see much benefit in starting
> > converting the drivers with the pattern below (which should be a considerable number).
> 
> There is not a big number of them, so I prefer to have common patterns
> without exact error code checks.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[Jonathan Cameron]

On Tue, 17 Mar 2026 12:42:07 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Feb 20, 2026 at 12:33:28PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 20, 2026 at 12:04 PM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > On Thu, 2026-02-19 at 15:39 +0100, Andy Shevchenko wrote:  
> > > > Instead of checking for the specific error codes (that can be considered
> > > > a layering violation to some extent) check for the property existence first
> > > > and then either parse it, or apply a default value.  
> > >
> > > Not really sure how I feel about this one. Checking for specific errors is a very common
> > > pattern and this change just makes it we check for the property presence twice. That said,
> > > this makes it more "future proof" (though I find it very unlikely for ret value o change).  
> > 
> > I already have an answer to this:
> > https://lore.kernel.org/r/aZcenabXYsOdBu84@smile.fi.intel.com  
> 
> Does it help?
> 
> > > Anyways, even if we choose to go down this route, I don't see much benefit in starting
> > > converting the drivers with the pattern below (which should be a considerable number).  
> > 
> > There is not a big number of them, so I prefer to have common patterns
> > without exact error code checks.  
> 


I left this one for a while to see if the discussion would continue but seems not.
I'm not sure it is always the case, but in this particular example I think the
resulting code is a little nicer to read so applied.

So for me, case by case basis for this sort of change.

Thanks,

Jonathan

Re: [PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[Nuno Sá]

On Sun, Apr 12, 2026 at 03:30:40PM +0100, Jonathan Cameron wrote:
> On Tue, 17 Mar 2026 12:42:07 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Fri, Feb 20, 2026 at 12:33:28PM +0200, Andy Shevchenko wrote:
> > > On Fri, Feb 20, 2026 at 12:04 PM Nuno Sá <noname.nuno@gmail.com> wrote:  
> > > > On Thu, 2026-02-19 at 15:39 +0100, Andy Shevchenko wrote:  
> > > > > Instead of checking for the specific error codes (that can be considered
> > > > > a layering violation to some extent) check for the property existence first
> > > > > and then either parse it, or apply a default value.  
> > > >
> > > > Not really sure how I feel about this one. Checking for specific errors is a very common
> > > > pattern and this change just makes it we check for the property presence twice. That said,
> > > > this makes it more "future proof" (though I find it very unlikely for ret value o change).  
> > > 
> > > I already have an answer to this:
> > > https://lore.kernel.org/r/aZcenabXYsOdBu84@smile.fi.intel.com  
> > 
> > Does it help?
> > 
> > > > Anyways, even if we choose to go down this route, I don't see much benefit in starting
> > > > converting the drivers with the pattern below (which should be a considerable number).  
> > > 
> > > There is not a big number of them, so I prefer to have common patterns
> > > without exact error code checks.  
> > 
> 
> 
> I left this one for a while to see if the discussion would continue but seems not.
> I'm not sure it is always the case, but in this particular example I think the
> resulting code is a little nicer to read so applied.
> 
> So for me, case by case basis for this sort of change.

Yeah, I missed this one! Anyways seems the way will be to explicitly
use device_property_present() to check property presence. Still don't
love the dual call thing so I might just stop checking for -EINVAL (was
not doing it religiously anyways).

Maybe we could have a set of optional variants of the API like
device_property_read_*_optional() kind of thing where we just return 0
if the property is not present. But might also be too noisy...

- Nuno Sá

> 
> Thanks,
> 
> Jonathan

Re: [PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[Andy Shevchenko]

On Tue, Apr 14, 2026 at 10:10:38AM +0100, Nuno Sá wrote:
> On Sun, Apr 12, 2026 at 03:30:40PM +0100, Jonathan Cameron wrote:
> > On Tue, 17 Mar 2026 12:42:07 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > I left this one for a while to see if the discussion would continue but seems not.
> > I'm not sure it is always the case, but in this particular example I think the
> > resulting code is a little nicer to read so applied.
> > 
> > So for me, case by case basis for this sort of change.
> 
> Yeah, I missed this one! Anyways seems the way will be to explicitly
> use device_property_present() to check property presence. Still don't
> love the dual call thing so I might just stop checking for -EINVAL (was
> not doing it religiously anyways).
> 
> Maybe we could have a set of optional variants of the API like
> device_property_read_*_optional() kind of thing where we just return 0
> if the property is not present. But might also be too noisy...

I think that's what usually Sakari likes (tons of wrappers for each case :-).
I prefer be on the compromise side.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[Andy Shevchenko]

On Tue, Apr 14, 2026 at 12:31:14PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 14, 2026 at 10:10:38AM +0100, Nuno Sá wrote:
> > On Sun, Apr 12, 2026 at 03:30:40PM +0100, Jonathan Cameron wrote:
> > > On Tue, 17 Mar 2026 12:42:07 +0200
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > > I left this one for a while to see if the discussion would continue but seems not.
> > > I'm not sure it is always the case, but in this particular example I think the
> > > resulting code is a little nicer to read so applied.
> > > 
> > > So for me, case by case basis for this sort of change.
> > 
> > Yeah, I missed this one! Anyways seems the way will be to explicitly
> > use device_property_present() to check property presence. Still don't
> > love the dual call thing so I might just stop checking for -EINVAL (was
> > not doing it religiously anyways).
> > 
> > Maybe we could have a set of optional variants of the API like
> > device_property_read_*_optional() kind of thing where we just return 0
> > if the property is not present. But might also be too noisy...
> 
> I think that's what usually Sakari likes (tons of wrappers for each case :-).
> I prefer be on the compromise side.

Ah, and note, now we have kernel-doc updated to point out that _present is
recommended way of unambiguously checking for the property presence.

70fa0c308aa2 ("device property: Document how to check for the property presence")


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] iio: adc: ad7191: Don't check for specific errors when parsing properties

[Nuno Sá]

On Tue, Apr 14, 2026 at 12:33:50PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 14, 2026 at 12:31:14PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 14, 2026 at 10:10:38AM +0100, Nuno Sá wrote:
> > > On Sun, Apr 12, 2026 at 03:30:40PM +0100, Jonathan Cameron wrote:
> > > > On Tue, 17 Mar 2026 12:42:07 +0200
> > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> ...
> 
> > > > I left this one for a while to see if the discussion would continue but seems not.
> > > > I'm not sure it is always the case, but in this particular example I think the
> > > > resulting code is a little nicer to read so applied.
> > > > 
> > > > So for me, case by case basis for this sort of change.
> > > 
> > > Yeah, I missed this one! Anyways seems the way will be to explicitly
> > > use device_property_present() to check property presence. Still don't
> > > love the dual call thing so I might just stop checking for -EINVAL (was
> > > not doing it religiously anyways).
> > > 
> > > Maybe we could have a set of optional variants of the API like
> > > device_property_read_*_optional() kind of thing where we just return 0
> > > if the property is not present. But might also be too noisy...
> > 
> > I think that's what usually Sakari likes (tons of wrappers for each case :-).
> > I prefer be on the compromise side.
> 
> Ah, and note, now we have kernel-doc updated to point out that _present is
> recommended way of unambiguously checking for the property presence.
> 
> 70fa0c308aa2 ("device property: Document how to check for the property presence")

Yes, I did looked at it! That's why I said

"Anyways seems the way will be to explicitly use device_property_present()..."

- Nuno Sá

> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>