Hi Jonathan,
On Sat, 22 Apr 2023 17:49:16 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Fri, 21 Apr 2023 14:41:20 +0200
> Herve Codina <herve.codina@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> 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é
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
