| Message ID | 20230421124122.324820-3-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for IIO devices in ASoC | expand |
On Fri, 21 Apr 2023 14:41:20 +0200 Herve Codina <herve.codina@bootlin.com> wrote: > A helper, iio_read_max_channel_raw() exists to read the available > maximum raw value of a channel but nothing similar exists to read the > available minimum raw value. > > This new helper, iio_read_min_channel_raw(), fills the hole and can be > used for reading the available minimum raw value of a channel. > It is fully based on the existing iio_read_max_channel_raw(). > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> Hi Herve, All the comments on this are really comments on the existing code. If you don't mind fixing the first one about checking the error code whilst you are here that would be great. Don't worry about the docs comment. There are lots of instances of that and the point is rather subtle and probably post dates this code being written. In a few cases raw doesn't mean ADC counts but rather something slightly modified... Long story for why! Jonathan > --- > drivers/iio/inkern.c | 67 ++++++++++++++++++++++++++++++++++++ > include/linux/iio/consumer.h | 11 ++++++ > 2 files changed, 78 insertions(+) > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c > index 872fd5c24147..914fc69c718a 100644 > --- a/drivers/iio/inkern.c > +++ b/drivers/iio/inkern.c > @@ -912,6 +912,73 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val) > } > EXPORT_SYMBOL_GPL(iio_read_max_channel_raw); > > +static int iio_channel_read_min(struct iio_channel *chan, > + int *val, int *val2, int *type, > + enum iio_chan_info_enum info) > +{ > + int unused; > + const int *vals; > + int length; > + int ret; > + > + if (!val2) > + val2 = &unused; > + > + ret = iio_channel_read_avail(chan, &vals, type, &length, info); Obviously this is copied from *_read_max() but look at it here... We should check for an error first with if (ret < 0) return ret; then the switch. Currently a different positive ret would result in that value being returned which would be odd. Not a problem today, but if we add other iio_avail_type enum entries in future and don't keep up with all the utility functions then a mess may result. If you agree with change and wouldn't mind adding another patch to this series tidying that up for the _max case that would be great! Otherwise I'll get to fixing that at some point but not anytime soon. > + switch (ret) { > + case IIO_AVAIL_RANGE: > + switch (*type) { > + case IIO_VAL_INT: > + *val = vals[0]; > + break; > + default: > + *val = vals[0]; > + *val2 = vals[1]; > + } > + return 0; > + > + case IIO_AVAIL_LIST: > + if (length <= 0) > + return -EINVAL; > + switch (*type) { > + case IIO_VAL_INT: > + *val = vals[--length]; > + while (length) { > + if (vals[--length] < *val) > + *val = vals[length]; > + } > + break; > + default: > + /* FIXME: learn about min for other iio values */ > + return -EINVAL; > + } > + return 0; > + > + default: > + return ret; > + } > +} > + > +int iio_read_min_channel_raw(struct iio_channel *chan, int *val) > +{ > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev); > + int ret; > + int type; > + > + mutex_lock(&iio_dev_opaque->info_exist_lock); > + if (!chan->indio_dev->info) { > + ret = -ENODEV; > + goto err_unlock; > + } > + > + ret = iio_channel_read_min(chan, val, NULL, &type, IIO_CHAN_INFO_RAW); > +err_unlock: > + mutex_unlock(&iio_dev_opaque->info_exist_lock); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(iio_read_min_channel_raw); > + > int iio_get_channel_type(struct iio_channel *chan, enum iio_chan_type *type) > { > struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev); > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h > index 6802596b017c..956120d8b5a3 100644 > --- a/include/linux/iio/consumer.h > +++ b/include/linux/iio/consumer.h > @@ -297,6 +297,17 @@ int iio_write_channel_raw(struct iio_channel *chan, int val); > */ > int iio_read_max_channel_raw(struct iio_channel *chan, int *val); > > +/** > + * iio_read_min_channel_raw() - read minimum available raw value from a given > + * channel, i.e. the minimum possible value. > + * @chan: The channel being queried. > + * @val: Value read back. > + * > + * Note raw reads from iio channels are in adc counts and hence > + * scale will need to be applied if standard units are required. Hmm. That comment is almost always true, but not quite. Not related to your patch but some cleanup of this documentation and pushing it down next to implementations should be done at some point. If anyone is really bored and wants to take this on that's fine. If not, another one for the todo list ;) > + */ > +int iio_read_min_channel_raw(struct iio_channel *chan, int *val); > + > /** > * iio_read_avail_channel_raw() - read available raw values from a given channel > * @chan: The channel being queried.
Hi Jonathan, On Sat, 22 Apr 2023 17:49:16 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 21 Apr 2023 14:41:20 +0200 > Herve Codina <herve.codina@bootlin.com> wrote: > > > A helper, iio_read_max_channel_raw() exists to read the available > > maximum raw value of a channel but nothing similar exists to read the > > available minimum raw value. > > > > This new helper, iio_read_min_channel_raw(), fills the hole and can be > > used for reading the available minimum raw value of a channel. > > It is fully based on the existing iio_read_max_channel_raw(). > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > Hi Herve, > > All the comments on this are really comments on the existing code. > If you don't mind fixing the first one about checking the error code whilst > you are here that would be great. Don't worry about the docs comment. > There are lots of instances of that and the point is rather subtle and probably > post dates this code being written. In a few cases raw doesn't mean ADC counts > but rather something slightly modified... Long story for why! A next iteration is already planned for this series. I will fix the 'error checking before switch()' on the iio_channel_read_min() I introduced and add a new patch (doing the same) on the existing iio_channel_read_max(). > > Jonathan > > > --- > > drivers/iio/inkern.c | 67 ++++++++++++++++++++++++++++++++++++ > > include/linux/iio/consumer.h | 11 ++++++ > > 2 files changed, 78 insertions(+) > > > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c > > index 872fd5c24147..914fc69c718a 100644 > > --- a/drivers/iio/inkern.c > > +++ b/drivers/iio/inkern.c > > @@ -912,6 +912,73 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val) > > } > > EXPORT_SYMBOL_GPL(iio_read_max_channel_raw); > > > > +static int iio_channel_read_min(struct iio_channel *chan, > > + int *val, int *val2, int *type, > > + enum iio_chan_info_enum info) > > +{ > > + int unused; > > + const int *vals; > > + int length; > > + int ret; > > + > > + if (!val2) > > + val2 = &unused; > > + > > + ret = iio_channel_read_avail(chan, &vals, type, &length, info); > Obviously this is copied from *_read_max() but look at it here... > > We should check for an error first with > if (ret < 0) > return ret; > then the switch. > > Currently a different positive ret would result in that value > being returned which would be odd. Not a problem today, but if we add other > iio_avail_type enum entries in future and don't keep up with all the > utility functions then a mess may result. > > If you agree with change and wouldn't mind adding another patch to this series > tidying that up for the _max case that would be great! Otherwise I'll get to > fixing that at some point but not anytime soon. I will do in the next iteration. > > > + switch (ret) { > > + case IIO_AVAIL_RANGE: > > + switch (*type) { > > + case IIO_VAL_INT: > > + *val = vals[0]; > > + break; > > + default: > > + *val = vals[0]; > > + *val2 = vals[1]; > > + } > > + return 0; > > + > > + case IIO_AVAIL_LIST: > > + if (length <= 0) > > + return -EINVAL; > > + switch (*type) { > > + case IIO_VAL_INT: > > + *val = vals[--length]; > > + while (length) { > > + if (vals[--length] < *val) > > + *val = vals[length]; > > + } > > + break; > > + default: > > + /* FIXME: learn about min for other iio values */ > > + return -EINVAL; > > + } > > + return 0; > > + > > + default: > > + return ret; > > + } > > +} > > + > > +int iio_read_min_channel_raw(struct iio_channel *chan, int *val) > > +{ > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev); > > + int ret; > > + int type; > > + > > + mutex_lock(&iio_dev_opaque->info_exist_lock); > > + if (!chan->indio_dev->info) { > > + ret = -ENODEV; > > + goto err_unlock; > > + } > > + > > + ret = iio_channel_read_min(chan, val, NULL, &type, IIO_CHAN_INFO_RAW); > > +err_unlock: > > + mutex_unlock(&iio_dev_opaque->info_exist_lock); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(iio_read_min_channel_raw); > > + > > int iio_get_channel_type(struct iio_channel *chan, enum iio_chan_type *type) > > { > > struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev); > > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h > > index 6802596b017c..956120d8b5a3 100644 > > --- a/include/linux/iio/consumer.h > > +++ b/include/linux/iio/consumer.h > > @@ -297,6 +297,17 @@ int iio_write_channel_raw(struct iio_channel *chan, int val); > > */ > > int iio_read_max_channel_raw(struct iio_channel *chan, int *val); > > > > +/** > > + * iio_read_min_channel_raw() - read minimum available raw value from a given > > + * channel, i.e. the minimum possible value. > > + * @chan: The channel being queried. > > + * @val: Value read back. > > + * > > + * Note raw reads from iio channels are in adc counts and hence > > + * scale will need to be applied if standard units are required. > > Hmm. That comment is almost always true, but not quite. Not related to > your patch but some cleanup of this documentation and pushing it down next > to implementations should be done at some point. If anyone is really > bored and wants to take this on that's fine. If not, another one for the > todo list ;) If you are ok, I can change every where in consumer.h the following: * Note raw reads from iio channels are in adc counts and hence * scale will need to be applied if standard units required. by * Note raw reads from iio channels are not in standards units and * hence scale will need to be applied if standard units required. Also the same for raw writes: * Note raw writes to iio channels are in dac counts and hence * scale will need to be applied if standard units required. by * Note raw writes to iio channels are not in standards units and * hence scale will need to be applied if standard units required. > > > + */ > > +int iio_read_min_channel_raw(struct iio_channel *chan, int *val); > > + > > /** > > * iio_read_avail_channel_raw() - read available raw values from a given channel > > * @chan: The channel being queried. > Thanks for the review, Hervé
> > > diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h > > > index 6802596b017c..956120d8b5a3 100644 > > > --- a/include/linux/iio/consumer.h > > > +++ b/include/linux/iio/consumer.h > > > @@ -297,6 +297,17 @@ int iio_write_channel_raw(struct iio_channel *chan, int val); > > > */ > > > int iio_read_max_channel_raw(struct iio_channel *chan, int *val); > > > > > > +/** > > > + * iio_read_min_channel_raw() - read minimum available raw value from a given > > > + * channel, i.e. the minimum possible value. > > > + * @chan: The channel being queried. > > > + * @val: Value read back. > > > + * > > > + * Note raw reads from iio channels are in adc counts and hence > > > + * scale will need to be applied if standard units are required. > > > > Hmm. That comment is almost always true, but not quite. Not related to > > your patch but some cleanup of this documentation and pushing it down next > > to implementations should be done at some point. If anyone is really > > bored and wants to take this on that's fine. If not, another one for the > > todo list ;) > > If you are ok, I can change every where in consumer.h the following: > * Note raw reads from iio channels are in adc counts and hence > * scale will need to be applied if standard units required. > by > * Note raw reads from iio channels are not in standards units and > * hence scale will need to be applied if standard units required. If going to the effort, we should include offset and make it clear how they are applied. * Note, if standard units are required, raw reads from iio channels * need the offset (default 0) and scale (default 1) to be applied * as (raw + offset) * scale. > > Also the same for raw writes: > * Note raw writes to iio channels are in dac counts and hence > * scale will need to be applied if standard units required. > by > * Note raw writes to iio channels are not in standards units and > * hence scale will need to be applied if standard units required. This one is more interesting because you kind of need to apply the opposite logic. Perhaps text such as. * Note that for raw writes to iio channels, if the value provided is * in standard units, the affect of the scale and offset must be removed * as (value / scale) - offset. My slight concern is that we'll spend longer arguing about these comments than we spend on the rest of the patch set. Might be worth delaying fixing the others for a separate series after this one. Jonathan > > > > > > + */ > > > +int iio_read_min_channel_raw(struct iio_channel *chan, int *val); > > > + > > > /** > > > * iio_read_avail_channel_raw() - read available raw values from a given channel > > > * @chan: The channel being queried. > > > > Thanks for the review, > Hervé >
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c index 872fd5c24147..914fc69c718a 100644 --- a/drivers/iio/inkern.c +++ b/drivers/iio/inkern.c @@ -912,6 +912,73 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val) } EXPORT_SYMBOL_GPL(iio_read_max_channel_raw); +static int iio_channel_read_min(struct iio_channel *chan, + int *val, int *val2, int *type, + enum iio_chan_info_enum info) +{ + int unused; + const int *vals; + int length; + int ret; + + if (!val2) + val2 = &unused; + + ret = iio_channel_read_avail(chan, &vals, type, &length, info); + switch (ret) { + case IIO_AVAIL_RANGE: + switch (*type) { + case IIO_VAL_INT: + *val = vals[0]; + break; + default: + *val = vals[0]; + *val2 = vals[1]; + } + return 0; + + case IIO_AVAIL_LIST: + if (length <= 0) + return -EINVAL; + switch (*type) { + case IIO_VAL_INT: + *val = vals[--length]; + while (length) { + if (vals[--length] < *val) + *val = vals[length]; + } + break; + default: + /* FIXME: learn about min for other iio values */ + return -EINVAL; + } + return 0; + + default: + return ret; + } +} + +int iio_read_min_channel_raw(struct iio_channel *chan, int *val) +{ + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev); + int ret; + int type; + + mutex_lock(&iio_dev_opaque->info_exist_lock); + if (!chan->indio_dev->info) { + ret = -ENODEV; + goto err_unlock; + } + + ret = iio_channel_read_min(chan, val, NULL, &type, IIO_CHAN_INFO_RAW); +err_unlock: + mutex_unlock(&iio_dev_opaque->info_exist_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(iio_read_min_channel_raw); + int iio_get_channel_type(struct iio_channel *chan, enum iio_chan_type *type) { struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev); diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h index 6802596b017c..956120d8b5a3 100644 --- a/include/linux/iio/consumer.h +++ b/include/linux/iio/consumer.h @@ -297,6 +297,17 @@ int iio_write_channel_raw(struct iio_channel *chan, int val); */ int iio_read_max_channel_raw(struct iio_channel *chan, int *val); +/** + * iio_read_min_channel_raw() - read minimum available raw value from a given + * channel, i.e. the minimum possible value. + * @chan: The channel being queried. + * @val: Value read back. + * + * Note raw reads from iio channels are in adc counts and hence + * scale will need to be applied if standard units are required. + */ +int iio_read_min_channel_raw(struct iio_channel *chan, int *val); + /** * iio_read_avail_channel_raw() - read available raw values from a given channel * @chan: The channel being queried.
A helper, iio_read_max_channel_raw() exists to read the available maximum raw value of a channel but nothing similar exists to read the available minimum raw value. This new helper, iio_read_min_channel_raw(), fills the hole and can be used for reading the available minimum raw value of a channel. It is fully based on the existing iio_read_max_channel_raw(). Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- drivers/iio/inkern.c | 67 ++++++++++++++++++++++++++++++++++++ include/linux/iio/consumer.h | 11 ++++++ 2 files changed, 78 insertions(+)