Re: [PATCH 2/4] iio: inkern: Add a helper to query an available minimum raw value

[Herve Codina <herve.codina@bootlin.com>]

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é

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com