[PATCH] iio: chemical: atlas-ezo-sensor: refactor IIO_CHAN_INFO_SCALE checks

[PATCH] iio: chemical: atlas-ezo-sensor: refactor IIO_CHAN_INFO_SCALE checks

[Matt Ranostay]

Additional modifiers in IIO_CHAN_INFO_SCALE check will mostly have a ppm
unit and the switch statement is more confusing, and adds unneeded code.

Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/iio/chemical/atlas-ezo-sensor.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/chemical/atlas-ezo-sensor.c b/drivers/iio/chemical/atlas-ezo-sensor.c
index 307c3488f4bd..5fae1c4087ee 100644
--- a/drivers/iio/chemical/atlas-ezo-sensor.c
+++ b/drivers/iio/chemical/atlas-ezo-sensor.c
@@ -165,17 +165,19 @@ static int atlas_ezo_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 
-		/* IIO_CONCENTRATION modifiers */
-		switch (chan->channel2) {
-		case IIO_MOD_CO2:
-			*val = 0;
-			*val2 = 100; /* 0.0001 */
-			return IIO_VAL_INT_PLUS_MICRO;
-		case IIO_MOD_O2:
+		if (chan->channel2 == IIO_NO_MOD)
+			return -EINVAL;
+
+		// IIO_CONCENTRATION modifier for percent
+		if (chan->channel2 == IIO_MOD_O2) {
 			*val = 100;
 			return IIO_VAL_INT;
 		}
-		return -EINVAL;
+
+		// IIO_CONCENTRATION modifier for PPM - 0.0001
+		*val = 0;
+		*val2 = 100;
+		return IIO_VAL_INT_PLUS_MICRO;
 	}
 
 	return 0;
-- 
2.30.2

Re: [PATCH] iio: chemical: atlas-ezo-sensor: refactor IIO_CHAN_INFO_SCALE checks

[Jonathan Cameron]

On Fri,  2 Dec 2022 14:23:05 -0800
Matt Ranostay <matt.ranostay@konsulko.com> wrote:

> Additional modifiers in IIO_CHAN_INFO_SCALE check will mostly have a ppm
> unit and the switch statement is more confusing, and adds unneeded code.
> 
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
Hi Matt,

Is this a precursor to more support where it doesn't just make sense to match on
the modifier, but rather have a default path?

Also, can we avoid the oddity of this code being reached by the fact
that only one case in the switch statement above doesn't return.
The code would be more readable to just do this stuff in 
case IIO_CONCENTRATION: 
and have slightly long lines.

> ---
>  drivers/iio/chemical/atlas-ezo-sensor.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/chemical/atlas-ezo-sensor.c b/drivers/iio/chemical/atlas-ezo-sensor.c
> index 307c3488f4bd..5fae1c4087ee 100644
> --- a/drivers/iio/chemical/atlas-ezo-sensor.c
> +++ b/drivers/iio/chemical/atlas-ezo-sensor.c
> @@ -165,17 +165,19 @@ static int atlas_ezo_read_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  
> -		/* IIO_CONCENTRATION modifiers */
> -		switch (chan->channel2) {
> -		case IIO_MOD_CO2:
> -			*val = 0;
> -			*val2 = 100; /* 0.0001 */
> -			return IIO_VAL_INT_PLUS_MICRO;
> -		case IIO_MOD_O2:
> +		if (chan->channel2 == IIO_NO_MOD)
> +			return -EINVAL;

This relies rather more on what values we actually have than the original
code.  Given we can't get to this code at all unless we have
a modifier (only applies to concentration channels which always do)
there isn't a lot of point in this check.

A sanity check on what we do have as a modifier and return -EINVAL
if none match would be better.

> +
> +		// IIO_CONCENTRATION modifier for percent
/* ... */
style preferred for IIO comments.

> +		if (chan->channel2 == IIO_MOD_O2) {
>  			*val = 100;
>  			return IIO_VAL_INT;
>  		}
> -		return -EINVAL;
> +
> +		// IIO_CONCENTRATION modifier for PPM - 0.0001
> +		*val = 0;
> +		*val2 = 100;
> +		return IIO_VAL_INT_PLUS_MICRO;
So this matches the IIO_MOD_
>  	}
>  
>  	return 0;