| Message ID | 20220711193714.50314-1-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC] iio: afe: rescale: Add support for converting scale avail table | expand |
On Mon, Jul 11, 2022 at 9:46 PM Paul Cercueil <paul@crapouillou.net> wrote: > > When the IIO channel has a scale_available attribute, we want the values > contained to be properly converted the same way the scale value is. Beside that not very readable `foo <<= x == y;` line, I think this will look much better if we first convert the rescale driver to use struct s32_fract (or whatever is suitable).
Hi Andy, Le mar., juil. 12 2022 at 10:24:02 +0200, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit : > On Mon, Jul 11, 2022 at 9:46 PM Paul Cercueil <paul@crapouillou.net> > wrote: >> >> When the IIO channel has a scale_available attribute, we want the >> values >> contained to be properly converted the same way the scale value is. > > Beside that not very readable `foo <<= x == y;` line, I think this > will look much better if we first convert the rescale driver to use > struct s32_fract (or whatever is suitable). More like a s64_fract since IIO uses two ints + a type to represent how the value should be computed from the ints. Is something like that already implemented somewhere in the kernel? -Paul
On Tue, Jul 12, 2022 at 12:13 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le mar., juil. 12 2022 at 10:24:02 +0200, Andy Shevchenko > <andy.shevchenko@gmail.com> a écrit : > > On Mon, Jul 11, 2022 at 9:46 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > >> > >> When the IIO channel has a scale_available attribute, we want the > >> values > >> contained to be properly converted the same way the scale value is. > > > > Beside that not very readable `foo <<= x == y;` line, I think this > > will look much better if we first convert the rescale driver to use > > struct s32_fract (or whatever is suitable). > > More like a s64_fract since IIO uses two ints + a type to represent how > the value should be computed from the ints. So, it's s32_fract :-) > Is something like that already implemented somewhere in the kernel? math.h and a few drivers are already using it.
On Mon, 11 Jul 2022 20:37:14 +0100 Paul Cercueil <paul@crapouillou.net> wrote: > When the IIO channel has a scale_available attribute, we want the values > contained to be properly converted the same way the scale value is. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/iio/afe/iio-rescale.c | 75 +++++++++++++++++++++++++++++++++ > include/linux/iio/afe/rescale.h | 2 + > 2 files changed, 77 insertions(+) > > > Hi Jonathan, > > I'm trying to add support for converting the scale_available attribute > in the iio-rescale driver. > > The code below works fine… as long as all the possible scales returned > by the underlying IIO device driver are translated to the exact same > type. The problem then is that rescale_process_scale() can return many > different types, while rescale_read_avail() only supports returning one > type. > > I don't really know what would be the way forward. Should the > .read_avail callback support returning multiple types? Should > rescale_process_scale() have an option to force all the types to be > converted to a specific one? Ah. I don't have a particularly strong view either way. Didn't have the complexities of the rescale driver in mind when we read_avail() was added (pesky hindsight ;) If you do go with read_avail() being able to return types then a reasonably clean way to do it might be to add a new IIO_AVAIL_LIST_WITH_TYPE and have *vals be a 3xN array with the type as the final integer. That should only impact the core code in a few places and means no changes to existing drivers. Jonathan > > Thoughts welcome. > > Cheers, > -Paul > > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 6949d2151025..8b00ff3de733 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -232,6 +232,19 @@ static int rescale_read_avail(struct iio_dev *indio_dev, > *type = IIO_VAL_INT; > return iio_read_avail_channel_raw(rescale->source, > vals, length); > + case IIO_CHAN_INFO_SCALE: > + if (rescale->chan_processed) { > + return iio_read_avail_channel_attribute(rescale->source, > + vals, type, > + length, > + IIO_CHAN_INFO_SCALE); > + } else if (rescale->scale_len) { > + *type = rescale->scale_type; > + *length = rescale->scale_len; > + *vals = rescale->scale_data; > + return IIO_AVAIL_LIST; > + } > + fallthrough; > default: > return -EINVAL; > } > @@ -266,11 +279,63 @@ static ssize_t rescale_write_ext_info(struct iio_dev *indio_dev, > buf, len); > } > > +static int rescale_init_scale_avail(struct device *dev, struct rescale *rescale) > +{ > + int ret, type, length, *data; > + const int *scale_raw; > + unsigned int i; > + > + ret = iio_read_avail_channel_attribute(rescale->source, &scale_raw, > + &type, &length, > + IIO_CHAN_INFO_SCALE); > + if (ret < 0) > + return ret; > + > + /* TODO: Support IIO_AVAIL_RANGE */ > + if (ret != IIO_AVAIL_LIST) > + return -ENOTSUPP; > + > + length <<= type == IIO_VAL_INT; > + > + data = devm_kzalloc(dev, sizeof(*data) * length, GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + if (type == IIO_VAL_INT) { > + /* Convert from integer to fractional form to ease processing */ > + for (i = 0; i < length / 2; i++) { > + data[i * 2] = scale_raw[i]; > + data[i * 2 + 1] = 1; > + } > + > + type = IIO_VAL_FRACTIONAL; > + } else { > + /* Copy raw scale info into our own buffer */ > + memcpy(data, scale_raw, sizeof(*scale_raw) * length); > + } > + > + for (i = 0; i < length; i += 2) { > + ret = rescale_process_scale(rescale, type, > + &data[i], &data[i + 1]); > + if (ret < 0) > + return ret; > + > + type = ret; > + } > + > + rescale->scale_type = type; > + rescale->scale_len = length; > + rescale->scale_data = data; > + > + return 0; > +} > + > static int rescale_configure_channel(struct device *dev, > struct rescale *rescale) > { > struct iio_chan_spec *chan = &rescale->chan; > struct iio_chan_spec const *schan = rescale->source->channel; > + int ret; > > chan->indexed = 1; > chan->output = schan->output; > @@ -303,6 +368,16 @@ static int rescale_configure_channel(struct device *dev, > !rescale->chan_processed) > chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW); > > + if (iio_channel_has_available(schan, IIO_CHAN_INFO_SCALE)) { > + chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SCALE); > + > + if (!rescale->chan_processed) { > + ret = rescale_init_scale_avail(dev, rescale); > + if (ret) > + return ret; > + } > + } > + > return 0; > } > > diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h > index 6eecb435488f..8618955695df 100644 > --- a/include/linux/iio/afe/rescale.h > +++ b/include/linux/iio/afe/rescale.h > @@ -26,6 +26,8 @@ struct rescale { > s32 numerator; > s32 denominator; > s32 offset; > + int scale_type, scale_len; > + int *scale_data; > }; > > int rescale_process_scale(struct rescale *rescale, int scale_type,
Hi Jonathan, Le mer., juil. 13 2022 at 17:34:01 +0100, Jonathan Cameron <jic23@kernel.org> a écrit : > On Mon, 11 Jul 2022 20:37:14 +0100 > Paul Cercueil <paul@crapouillou.net> wrote: > >> When the IIO channel has a scale_available attribute, we want the >> values >> contained to be properly converted the same way the scale value is. >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> drivers/iio/afe/iio-rescale.c | 75 >> +++++++++++++++++++++++++++++++++ >> include/linux/iio/afe/rescale.h | 2 + >> 2 files changed, 77 insertions(+) >> >> >> Hi Jonathan, >> >> I'm trying to add support for converting the scale_available >> attribute >> in the iio-rescale driver. >> >> The code below works fine… as long as all the possible scales >> returned >> by the underlying IIO device driver are translated to the exact >> same >> type. The problem then is that rescale_process_scale() can return >> many >> different types, while rescale_read_avail() only supports >> returning one >> type. >> >> I don't really know what would be the way forward. Should the >> .read_avail callback support returning multiple types? Should >> rescale_process_scale() have an option to force all the types to be >> converted to a specific one? > > Ah. I don't have a particularly strong view either way. Didn't have > the complexities of the rescale driver in mind when we read_avail() > was > added (pesky hindsight ;) > > If you do go with read_avail() being able to return types then a > reasonably > clean way to do it might be to add a new > IIO_AVAIL_LIST_WITH_TYPE > and have *vals be a 3xN array with the type as the final integer. > That should only impact the core code in a few places and means no > changes > to existing drivers. That's a smart idea. I will try that. Thanks! -Paul >> >> diff --git a/drivers/iio/afe/iio-rescale.c >> b/drivers/iio/afe/iio-rescale.c >> index 6949d2151025..8b00ff3de733 100644 >> --- a/drivers/iio/afe/iio-rescale.c >> +++ b/drivers/iio/afe/iio-rescale.c >> @@ -232,6 +232,19 @@ static int rescale_read_avail(struct iio_dev >> *indio_dev, >> *type = IIO_VAL_INT; >> return iio_read_avail_channel_raw(rescale->source, >> vals, length); >> + case IIO_CHAN_INFO_SCALE: >> + if (rescale->chan_processed) { >> + return iio_read_avail_channel_attribute(rescale->source, >> + vals, type, >> + length, >> + IIO_CHAN_INFO_SCALE); >> + } else if (rescale->scale_len) { >> + *type = rescale->scale_type; >> + *length = rescale->scale_len; >> + *vals = rescale->scale_data; >> + return IIO_AVAIL_LIST; >> + } >> + fallthrough; >> default: >> return -EINVAL; >> } >> @@ -266,11 +279,63 @@ static ssize_t rescale_write_ext_info(struct >> iio_dev *indio_dev, >> buf, len); >> } >> >> +static int rescale_init_scale_avail(struct device *dev, struct >> rescale *rescale) >> +{ >> + int ret, type, length, *data; >> + const int *scale_raw; >> + unsigned int i; >> + >> + ret = iio_read_avail_channel_attribute(rescale->source, >> &scale_raw, >> + &type, &length, >> + IIO_CHAN_INFO_SCALE); >> + if (ret < 0) >> + return ret; >> + >> + /* TODO: Support IIO_AVAIL_RANGE */ >> + if (ret != IIO_AVAIL_LIST) >> + return -ENOTSUPP; >> + >> + length <<= type == IIO_VAL_INT; >> + >> + data = devm_kzalloc(dev, sizeof(*data) * length, GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + if (type == IIO_VAL_INT) { >> + /* Convert from integer to fractional form to ease processing */ >> + for (i = 0; i < length / 2; i++) { >> + data[i * 2] = scale_raw[i]; >> + data[i * 2 + 1] = 1; >> + } >> + >> + type = IIO_VAL_FRACTIONAL; >> + } else { >> + /* Copy raw scale info into our own buffer */ >> + memcpy(data, scale_raw, sizeof(*scale_raw) * length); >> + } >> + >> + for (i = 0; i < length; i += 2) { >> + ret = rescale_process_scale(rescale, type, >> + &data[i], &data[i + 1]); >> + if (ret < 0) >> + return ret; >> + >> + type = ret; >> + } >> + >> + rescale->scale_type = type; >> + rescale->scale_len = length; >> + rescale->scale_data = data; >> + >> + return 0; >> +} >> + >> static int rescale_configure_channel(struct device *dev, >> struct rescale *rescale) >> { >> struct iio_chan_spec *chan = &rescale->chan; >> struct iio_chan_spec const *schan = rescale->source->channel; >> + int ret; >> >> chan->indexed = 1; >> chan->output = schan->output; >> @@ -303,6 +368,16 @@ static int rescale_configure_channel(struct >> device *dev, >> !rescale->chan_processed) >> chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW); >> >> + if (iio_channel_has_available(schan, IIO_CHAN_INFO_SCALE)) { >> + chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SCALE); >> + >> + if (!rescale->chan_processed) { >> + ret = rescale_init_scale_avail(dev, rescale); >> + if (ret) >> + return ret; >> + } >> + } >> + >> return 0; >> } >> >> diff --git a/include/linux/iio/afe/rescale.h >> b/include/linux/iio/afe/rescale.h >> index 6eecb435488f..8618955695df 100644 >> --- a/include/linux/iio/afe/rescale.h >> +++ b/include/linux/iio/afe/rescale.h >> @@ -26,6 +26,8 @@ struct rescale { >> s32 numerator; >> s32 denominator; >> s32 offset; >> + int scale_type, scale_len; >> + int *scale_data; >> }; >> >> int rescale_process_scale(struct rescale *rescale, int scale_type, >
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index 6949d2151025..8b00ff3de733 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -232,6 +232,19 @@ static int rescale_read_avail(struct iio_dev *indio_dev, *type = IIO_VAL_INT; return iio_read_avail_channel_raw(rescale->source, vals, length); + case IIO_CHAN_INFO_SCALE: + if (rescale->chan_processed) { + return iio_read_avail_channel_attribute(rescale->source, + vals, type, + length, + IIO_CHAN_INFO_SCALE); + } else if (rescale->scale_len) { + *type = rescale->scale_type; + *length = rescale->scale_len; + *vals = rescale->scale_data; + return IIO_AVAIL_LIST; + } + fallthrough; default: return -EINVAL; } @@ -266,11 +279,63 @@ static ssize_t rescale_write_ext_info(struct iio_dev *indio_dev, buf, len); } +static int rescale_init_scale_avail(struct device *dev, struct rescale *rescale) +{ + int ret, type, length, *data; + const int *scale_raw; + unsigned int i; + + ret = iio_read_avail_channel_attribute(rescale->source, &scale_raw, + &type, &length, + IIO_CHAN_INFO_SCALE); + if (ret < 0) + return ret; + + /* TODO: Support IIO_AVAIL_RANGE */ + if (ret != IIO_AVAIL_LIST) + return -ENOTSUPP; + + length <<= type == IIO_VAL_INT; + + data = devm_kzalloc(dev, sizeof(*data) * length, GFP_KERNEL); + if (!data) + return -ENOMEM; + + if (type == IIO_VAL_INT) { + /* Convert from integer to fractional form to ease processing */ + for (i = 0; i < length / 2; i++) { + data[i * 2] = scale_raw[i]; + data[i * 2 + 1] = 1; + } + + type = IIO_VAL_FRACTIONAL; + } else { + /* Copy raw scale info into our own buffer */ + memcpy(data, scale_raw, sizeof(*scale_raw) * length); + } + + for (i = 0; i < length; i += 2) { + ret = rescale_process_scale(rescale, type, + &data[i], &data[i + 1]); + if (ret < 0) + return ret; + + type = ret; + } + + rescale->scale_type = type; + rescale->scale_len = length; + rescale->scale_data = data; + + return 0; +} + static int rescale_configure_channel(struct device *dev, struct rescale *rescale) { struct iio_chan_spec *chan = &rescale->chan; struct iio_chan_spec const *schan = rescale->source->channel; + int ret; chan->indexed = 1; chan->output = schan->output; @@ -303,6 +368,16 @@ static int rescale_configure_channel(struct device *dev, !rescale->chan_processed) chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW); + if (iio_channel_has_available(schan, IIO_CHAN_INFO_SCALE)) { + chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_SCALE); + + if (!rescale->chan_processed) { + ret = rescale_init_scale_avail(dev, rescale); + if (ret) + return ret; + } + } + return 0; } diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h index 6eecb435488f..8618955695df 100644 --- a/include/linux/iio/afe/rescale.h +++ b/include/linux/iio/afe/rescale.h @@ -26,6 +26,8 @@ struct rescale { s32 numerator; s32 denominator; s32 offset; + int scale_type, scale_len; + int *scale_data; }; int rescale_process_scale(struct rescale *rescale, int scale_type,
When the IIO channel has a scale_available attribute, we want the values contained to be properly converted the same way the scale value is. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- drivers/iio/afe/iio-rescale.c | 75 +++++++++++++++++++++++++++++++++ include/linux/iio/afe/rescale.h | 2 + 2 files changed, 77 insertions(+) Hi Jonathan, I'm trying to add support for converting the scale_available attribute in the iio-rescale driver. The code below works fine… as long as all the possible scales returned by the underlying IIO device driver are translated to the exact same type. The problem then is that rescale_process_scale() can return many different types, while rescale_read_avail() only supports returning one type. I don't really know what would be the way forward. Should the .read_avail callback support returning multiple types? Should rescale_process_scale() have an option to force all the types to be converted to a specific one? Thoughts welcome. Cheers, -Paul