| Message ID | 20230523151223.109551-6-herve.codina@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for IIO devices in ASoC | expand |
Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti: > 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(). ... > +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; It's a single place, where this is used, can you move it there? > + ret = iio_channel_read_avail(chan, &vals, type, &length, info); > + if (ret < 0) > + return ret; > + > + 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) { while (length--) { will do the job and at the same time... > + if (vals[--length] < *val) > + *val = vals[length]; ...this construction becomes less confusing (easier to parse). > + } > + break; > + default: > + /* FIXME: learn about min for other iio values */ I believe in a final version this comment won't be here. > + return -EINVAL; > + } > + return 0; > + > + default: > + return -EINVAL; > + } > +}
On Sat, 3 Jun 2023 17:04:37 +0300 andy.shevchenko@gmail.com wrote: > Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti: > > 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(). > > ... > > > +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; > > It's a single place, where this is used, can you move it there? I will do that in the next iteration. Also, I will do the same modification in iio_channel_read_max() as it has exactly the same code. > > > + ret = iio_channel_read_avail(chan, &vals, type, &length, info); > > + if (ret < 0) > > + return ret; > > + > > + 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) { > > while (length--) { > > will do the job and at the same time... > > > > + if (vals[--length] < *val) > > + *val = vals[length]; > > ...this construction becomes less confusing (easier to parse). Indeed, I will change in the next iteration. > > > + } > > + break; > > + default: > > + /* FIXME: learn about min for other iio values */ > > I believe in a final version this comment won't be here. We have the same FIXME comment in the iio_channel_read_max() function I copied to create this iio_channel_read_min() and, to be honest, I don't really know how to handle these other cases. In this series, I would prefer to keep this FIXME. > > > + return -EINVAL; > > + } > > + return 0; > > + > > + default: > > + return -EINVAL; > > + } > > +} > Thanks for the review, Hervé
On Mon, Jun 5, 2023 at 10:46 AM Herve Codina <herve.codina@bootlin.com> wrote: > On Sat, 3 Jun 2023 17:04:37 +0300 > andy.shevchenko@gmail.com wrote: > > Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti: ... > > > + case IIO_VAL_INT: > > > + *val = vals[--length]; > > > > > + while (length) { > > > > while (length--) { > > > > will do the job and at the same time... > > > > > + if (vals[--length] < *val) > > > + *val = vals[length]; > > > > ...this construction becomes less confusing (easier to parse). > > Indeed, I will change in the next iteration. And looking into above line, this whole construction I would prefer to have a macro in minmax.h like #define min_array(array, len) \ {( \ typeof(len) __len = (len); \ typeof(*(array)) __element = (array)[--__len]; \ while (__len--) \ __element = min(__element, (array)[__len]); \ __element; \ )} (it might need more work, but you got the idea) > > > + } > > > + break; ... > > > + default: > > > + /* FIXME: learn about min for other iio values */ > > > > I believe in a final version this comment won't be here. > > We have the same FIXME comment in the iio_channel_read_max() function I > copied to create this iio_channel_read_min() and, to be honest, I > don't really know how to handle these other cases. > > In this series, I would prefer to keep this FIXME. I see, Jonathan needs to be involved here then. > > > + return -EINVAL;
Hi Andy, On Mon, 5 Jun 2023 12:45:24 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jun 5, 2023 at 10:46 AM Herve Codina <herve.codina@bootlin.com> wrote: > > On Sat, 3 Jun 2023 17:04:37 +0300 > > andy.shevchenko@gmail.com wrote: > > > Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti: > > ... > > > > > + case IIO_VAL_INT: > > > > + *val = vals[--length]; > > > > > > > + while (length) { > > > > > > while (length--) { > > > > > > will do the job and at the same time... > > > > > > > + if (vals[--length] < *val) > > > > + *val = vals[length]; > > > > > > ...this construction becomes less confusing (easier to parse). > > > > Indeed, I will change in the next iteration. > > And looking into above line, this whole construction I would prefer to > have a macro in minmax.h like > > #define min_array(array, len) \ > {( \ > typeof(len) __len = (len); \ > typeof(*(array)) __element = (array)[--__len]; \ > while (__len--) \ > __element = min(__element, (array)[__len]); \ > __element; \ > )} > > (it might need more work, but you got the idea) I will also introduce max_array() and update both iio_channel_read_max() and iio_channel_read_min() to use these macros. Will be available in the next series iteration. Thanks, Hervé > > > > > + } > > > > + break; > > ... > > > > > + default: > > > > + /* FIXME: learn about min for other iio values */ > > > > > > I believe in a final version this comment won't be here. > > > > We have the same FIXME comment in the iio_channel_read_max() function I > > copied to create this iio_channel_read_min() and, to be honest, I > > don't really know how to handle these other cases. > > > > In this series, I would prefer to keep this FIXME. > > I see, Jonathan needs to be involved here then. > > > > > + return -EINVAL; >
On Mon, 5 Jun 2023 12:45:24 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jun 5, 2023 at 10:46 AM Herve Codina <herve.codina@bootlin.com> wrote: > > On Sat, 3 Jun 2023 17:04:37 +0300 > > andy.shevchenko@gmail.com wrote: > > > Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti: > > ... > > > > > + case IIO_VAL_INT: > > > > + *val = vals[--length]; > > > > > > > + while (length) { > > > > > > while (length--) { > > > > > > will do the job and at the same time... > > > > > > > + if (vals[--length] < *val) > > > > + *val = vals[length]; > > > > > > ...this construction becomes less confusing (easier to parse). > > > > Indeed, I will change in the next iteration. > > And looking into above line, this whole construction I would prefer to > have a macro in minmax.h like > > #define min_array(array, len) \ > {( \ > typeof(len) __len = (len); \ > typeof(*(array)) __element = (array)[--__len]; \ > while (__len--) \ > __element = min(__element, (array)[__len]); \ > __element; \ > )} > > (it might need more work, but you got the idea) > > > > > + } > > > > + break; > > ... > > > > > + default: > > > > + /* FIXME: learn about min for other iio values */ > > > > > > I believe in a final version this comment won't be here. > > > > We have the same FIXME comment in the iio_channel_read_max() function I > > copied to create this iio_channel_read_min() and, to be honest, I > > don't really know how to handle these other cases. > > > > In this series, I would prefer to keep this FIXME. > > I see, Jonathan needs to be involved here then. It's really more of a TODO when someone needs it than a FIXME. I'm not particularly keen to see a bunch of code supporting cases that no one uses, but it's useful to call out where the code would go if other cases were to be supported. Perhaps soften it to a note that doesn't have the work FIXME in it. Jonathan > > > > > + return -EINVAL; >
Hi Jonathan, Andy, On Mon, 5 Jun 2023 18:05:47 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Mon, 5 Jun 2023 12:45:24 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > ... > > > > > + default: > > > > > + /* FIXME: learn about min for other iio values */ > > > > > > > > I believe in a final version this comment won't be here. > > > > > > We have the same FIXME comment in the iio_channel_read_max() function I > > > copied to create this iio_channel_read_min() and, to be honest, I > > > don't really know how to handle these other cases. > > > > > > In this series, I would prefer to keep this FIXME. > > > > I see, Jonathan needs to be involved here then. > > It's really more of a TODO when someone needs it than a FIXME. > I'm not particularly keen to see a bunch of code supporting cases > that no one uses, but it's useful to call out where the code would > go if other cases were to be supported. > > Perhaps soften it to a note that doesn't have the work FIXME in it. > > Jonathan > Right, I will change to /* TODO: learn about max for other iio values */ in the next iteration (both iio_channel_read_max() and iio_channel_read_min()) Regards, Hervé > > > > > > > > + return -EINVAL; > > >
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c index f738db9a0c04..07b26ff8a334 100644 --- a/drivers/iio/inkern.c +++ b/drivers/iio/inkern.c @@ -915,6 +915,76 @@ 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); + if (ret < 0) + return ret; + + 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 -EINVAL; + } +} + +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 f536820b9cf2..e9910b41d48e 100644 --- a/include/linux/iio/consumer.h +++ b/include/linux/iio/consumer.h @@ -301,6 +301,18 @@ 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, 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. + */ +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 | 70 ++++++++++++++++++++++++++++++++++++ include/linux/iio/consumer.h | 12 +++++++ 2 files changed, 82 insertions(+)