| Message ID | 20221202222305.8828-1-matt.ranostay@konsulko.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: chemical: atlas-ezo-sensor: refactor IIO_CHAN_INFO_SCALE checks | expand |
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;
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;
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(-)