| Message ID | 20220721191526.374152-4-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: afe/rescale improvements | expand |
Hi! 2022-07-21 at 21:15, Paul Cercueil 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. > > Since rescale_process_scale() may change the encoding type, we must > convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/iio/afe/iio-rescale.c | 85 +++++++++++++++++++++++++++++++++ > include/linux/iio/afe/rescale.h | 2 + > 2 files changed, 87 insertions(+) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 6949d2151025..5c9970b93384 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -232,6 +232,18 @@ 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) { I think it is wrong to simply feed the info-scale to the source channel if it happens to be processed. It still needs the inverse rescale. But see below. > + return iio_read_avail_channel_attribute(rescale->source, > + vals, type, > + length, > + IIO_CHAN_INFO_SCALE); > + } else if (rescale->scale_len) { > + *length = rescale->scale_len; > + *vals = rescale->scale_data; > + return IIO_AVAIL_LIST_WITH_TYPE; > + } > + fallthrough; > default: > return -EINVAL; > } > @@ -266,11 +278,74 @@ 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; > + size_t out_len; > + > + ret = iio_read_avail_channel_attribute(rescale->source, &scale_raw, > + &type, &length, > + IIO_CHAN_INFO_SCALE); > + if (ret < 0) > + return ret; > + > + switch (ret) { > + case IIO_AVAIL_LIST_WITH_TYPE: > + out_len = length; > + break; > + case IIO_AVAIL_LIST: > + if (type == IIO_VAL_INT) > + out_len = length * 3 / 1; > + else > + out_len = length * 3 / 2; > + break; > + default: > + /* TODO: Support IIO_AVAIL_RANGE */ > + return -EOPNOTSUPP; > + } > + > + data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + if (ret == IIO_AVAIL_LIST_WITH_TYPE) { > + memcpy(data, scale_raw, sizeof(*scale_raw) * length); > + } else if (type == IIO_VAL_INT) { > + for (i = 0; i < length; i++) { > + data[i * 3 + 0] = scale_raw[i]; > + data[i * 3 + 2] = IIO_VAL_INT; > + } > + } else { > + for (i = 0; i < length / 2; i++) { > + data[i * 3 + 0] = scale_raw[i * 2]; > + data[i * 3 + 1] = scale_raw[i * 2 + 1]; > + data[i * 3 + 2] = type; > + } > + } > + > + for (i = 0; i < out_len; i += 3) { > + ret = rescale_process_scale(rescale, data[i + 2], > + &data[i], &data[i + 1]); > + if (ret < 0) > + return ret; > + > + data[i + 2] = ret; > + } > + > + rescale->scale_len = out_len; > + 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 +378,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; > + } > + } > + Does a (sane) processed channel really have a scale? That seems a bit fringe. Wouldn't it be better to conditionally publish availability of info-scale so that it isn't visible for processed channels and dodge that rabbit-hole? In either case, the above commented implementation of info-scale for rescaled processed channels is wrong (I think...). Cheers, Peter > return 0; > } > > diff --git a/include/linux/iio/afe/rescale.h b/include/linux/iio/afe/rescale.h > index 6eecb435488f..74de2962f864 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_len; > + int *scale_data; > }; > > int rescale_process_scale(struct rescale *rescale, int scale_type,
Hi Peter, Le ven., juil. 22 2022 at 00:16:31 +0200, Peter Rosin <peda@axentia.se> a écrit : > Hi! > > 2022-07-21 at 21:15, Paul Cercueil 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. >> >> Since rescale_process_scale() may change the encoding type, we must >> convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE. >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> drivers/iio/afe/iio-rescale.c | 85 >> +++++++++++++++++++++++++++++++++ >> include/linux/iio/afe/rescale.h | 2 + >> 2 files changed, 87 insertions(+) >> >> diff --git a/drivers/iio/afe/iio-rescale.c >> b/drivers/iio/afe/iio-rescale.c >> index 6949d2151025..5c9970b93384 100644 >> --- a/drivers/iio/afe/iio-rescale.c >> +++ b/drivers/iio/afe/iio-rescale.c >> @@ -232,6 +232,18 @@ 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) { > > I think it is wrong to simply feed the info-scale to the source > channel if it > happens to be processed. It still needs the inverse rescale. But see > below. Yes, when I started working on that patchset, processed channels weren't a thing, and I don't think I understood what they are about. > >> + return iio_read_avail_channel_attribute(rescale->source, >> + vals, type, >> + length, >> + IIO_CHAN_INFO_SCALE); >> + } else if (rescale->scale_len) { >> + *length = rescale->scale_len; >> + *vals = rescale->scale_data; >> + return IIO_AVAIL_LIST_WITH_TYPE; >> + } >> + fallthrough; >> default: >> return -EINVAL; >> } >> @@ -266,11 +278,74 @@ 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; >> + size_t out_len; >> + >> + ret = iio_read_avail_channel_attribute(rescale->source, >> &scale_raw, >> + &type, &length, >> + IIO_CHAN_INFO_SCALE); >> + if (ret < 0) >> + return ret; >> + >> + switch (ret) { >> + case IIO_AVAIL_LIST_WITH_TYPE: >> + out_len = length; >> + break; >> + case IIO_AVAIL_LIST: >> + if (type == IIO_VAL_INT) >> + out_len = length * 3 / 1; >> + else >> + out_len = length * 3 / 2; >> + break; >> + default: >> + /* TODO: Support IIO_AVAIL_RANGE */ >> + return -EOPNOTSUPP; >> + } >> + >> + data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + if (ret == IIO_AVAIL_LIST_WITH_TYPE) { >> + memcpy(data, scale_raw, sizeof(*scale_raw) * length); >> + } else if (type == IIO_VAL_INT) { >> + for (i = 0; i < length; i++) { >> + data[i * 3 + 0] = scale_raw[i]; >> + data[i * 3 + 2] = IIO_VAL_INT; >> + } >> + } else { >> + for (i = 0; i < length / 2; i++) { >> + data[i * 3 + 0] = scale_raw[i * 2]; >> + data[i * 3 + 1] = scale_raw[i * 2 + 1]; >> + data[i * 3 + 2] = type; >> + } >> + } >> + >> + for (i = 0; i < out_len; i += 3) { >> + ret = rescale_process_scale(rescale, data[i + 2], >> + &data[i], &data[i + 1]); >> + if (ret < 0) >> + return ret; >> + >> + data[i + 2] = ret; >> + } >> + >> + rescale->scale_len = out_len; >> + 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 +378,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; >> + } >> + } >> + > > Does a (sane) processed channel really have a scale? That seems a bit > fringe. > Wouldn't it be better to conditionally publish availability of > info-scale so > that it isn't visible for processed channels and dodge that > rabbit-hole? In > either case, the above commented implementation of info-scale for > rescaled > processed channels is wrong (I think...). I could set the IIO_CHAN_INFO_SCALE only for non-processed channels, since this is what I can test with. Cheers, -Paul >> return 0; >> } >> >> diff --git a/include/linux/iio/afe/rescale.h >> b/include/linux/iio/afe/rescale.h >> index 6eecb435488f..74de2962f864 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_len; >> + int *scale_data; >> }; >> >> int rescale_process_scale(struct rescale *rescale, int scale_type,
On Fri, 22 Jul 2022 09:52:34 +0100 Paul Cercueil <paul@crapouillou.net> wrote: > Hi Peter, > > Le ven., juil. 22 2022 at 00:16:31 +0200, Peter Rosin <peda@axentia.se> > a écrit : > > Hi! > > > > 2022-07-21 at 21:15, Paul Cercueil 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. > >> > >> Since rescale_process_scale() may change the encoding type, we must > >> convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE. > >> > >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> > >> --- > >> drivers/iio/afe/iio-rescale.c | 85 > >> +++++++++++++++++++++++++++++++++ > >> include/linux/iio/afe/rescale.h | 2 + > >> 2 files changed, 87 insertions(+) > >> > >> diff --git a/drivers/iio/afe/iio-rescale.c > >> b/drivers/iio/afe/iio-rescale.c > >> index 6949d2151025..5c9970b93384 100644 > >> --- a/drivers/iio/afe/iio-rescale.c > >> +++ b/drivers/iio/afe/iio-rescale.c > >> @@ -232,6 +232,18 @@ 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) { > > > > I think it is wrong to simply feed the info-scale to the source > > channel if it > > happens to be processed. It still needs the inverse rescale. But see > > below. > > Yes, when I started working on that patchset, processed channels > weren't a thing, and I don't think I understood what they are about. > > > > >> + return iio_read_avail_channel_attribute(rescale->source, > >> + vals, type, > >> + length, > >> + IIO_CHAN_INFO_SCALE); > >> + } else if (rescale->scale_len) { > >> + *length = rescale->scale_len; > >> + *vals = rescale->scale_data; > >> + return IIO_AVAIL_LIST_WITH_TYPE; > >> + } > >> + fallthrough; > >> default: > >> return -EINVAL; > >> } > >> @@ -266,11 +278,74 @@ 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; > >> + size_t out_len; > >> + > >> + ret = iio_read_avail_channel_attribute(rescale->source, > >> &scale_raw, > >> + &type, &length, > >> + IIO_CHAN_INFO_SCALE); > >> + if (ret < 0) > >> + return ret; > >> + > >> + switch (ret) { > >> + case IIO_AVAIL_LIST_WITH_TYPE: > >> + out_len = length; > >> + break; > >> + case IIO_AVAIL_LIST: > >> + if (type == IIO_VAL_INT) > >> + out_len = length * 3 / 1; > >> + else > >> + out_len = length * 3 / 2; > >> + break; > >> + default: > >> + /* TODO: Support IIO_AVAIL_RANGE */ > >> + return -EOPNOTSUPP; > >> + } > >> + > >> + data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL); > >> + if (!data) > >> + return -ENOMEM; > >> + > >> + if (ret == IIO_AVAIL_LIST_WITH_TYPE) { > >> + memcpy(data, scale_raw, sizeof(*scale_raw) * length); > >> + } else if (type == IIO_VAL_INT) { > >> + for (i = 0; i < length; i++) { > >> + data[i * 3 + 0] = scale_raw[i]; > >> + data[i * 3 + 2] = IIO_VAL_INT; > >> + } > >> + } else { > >> + for (i = 0; i < length / 2; i++) { > >> + data[i * 3 + 0] = scale_raw[i * 2]; > >> + data[i * 3 + 1] = scale_raw[i * 2 + 1]; > >> + data[i * 3 + 2] = type; > >> + } > >> + } > >> + > >> + for (i = 0; i < out_len; i += 3) { > >> + ret = rescale_process_scale(rescale, data[i + 2], > >> + &data[i], &data[i + 1]); > >> + if (ret < 0) > >> + return ret; > >> + > >> + data[i + 2] = ret; > >> + } > >> + > >> + rescale->scale_len = out_len; > >> + 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 +378,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; > >> + } > >> + } > >> + > > > > Does a (sane) processed channel really have a scale? That seems a bit > > fringe. > > Wouldn't it be better to conditionally publish availability of > > info-scale so > > that it isn't visible for processed channels and dodge that > > rabbit-hole? In > > either case, the above commented implementation of info-scale for > > rescaled > > processed channels is wrong (I think...). > > I could set the IIO_CHAN_INFO_SCALE only for non-processed channels, > since this is what I can test with. Indeed both nonsensical for PROCESSED channels to have a scale and in the rare corner case where it happens you shouldn't apply it anyway. I'm struggling to think of when it might happen due to maintaining backwards compatibility and similar (the reason we have channels with both PROCESSED and RAW) but in those cases I don't think we'd have SCALE because they tend to be devices with nasty non linear transfer functions where SCALE isn't appropriate. Jonathan > > Cheers, > -Paul > > >> return 0; > >> } > >> > >> diff --git a/include/linux/iio/afe/rescale.h > >> b/include/linux/iio/afe/rescale.h > >> index 6eecb435488f..74de2962f864 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_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..5c9970b93384 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -232,6 +232,18 @@ 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) { + *length = rescale->scale_len; + *vals = rescale->scale_data; + return IIO_AVAIL_LIST_WITH_TYPE; + } + fallthrough; default: return -EINVAL; } @@ -266,11 +278,74 @@ 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; + size_t out_len; + + ret = iio_read_avail_channel_attribute(rescale->source, &scale_raw, + &type, &length, + IIO_CHAN_INFO_SCALE); + if (ret < 0) + return ret; + + switch (ret) { + case IIO_AVAIL_LIST_WITH_TYPE: + out_len = length; + break; + case IIO_AVAIL_LIST: + if (type == IIO_VAL_INT) + out_len = length * 3 / 1; + else + out_len = length * 3 / 2; + break; + default: + /* TODO: Support IIO_AVAIL_RANGE */ + return -EOPNOTSUPP; + } + + data = devm_kzalloc(dev, sizeof(*data) * out_len, GFP_KERNEL); + if (!data) + return -ENOMEM; + + if (ret == IIO_AVAIL_LIST_WITH_TYPE) { + memcpy(data, scale_raw, sizeof(*scale_raw) * length); + } else if (type == IIO_VAL_INT) { + for (i = 0; i < length; i++) { + data[i * 3 + 0] = scale_raw[i]; + data[i * 3 + 2] = IIO_VAL_INT; + } + } else { + for (i = 0; i < length / 2; i++) { + data[i * 3 + 0] = scale_raw[i * 2]; + data[i * 3 + 1] = scale_raw[i * 2 + 1]; + data[i * 3 + 2] = type; + } + } + + for (i = 0; i < out_len; i += 3) { + ret = rescale_process_scale(rescale, data[i + 2], + &data[i], &data[i + 1]); + if (ret < 0) + return ret; + + data[i + 2] = ret; + } + + rescale->scale_len = out_len; + 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 +378,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..74de2962f864 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_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. Since rescale_process_scale() may change the encoding type, we must convert the IIO_AVAIL_LIST to a IIO_AVAIL_LIST_WITH_TYPE. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- drivers/iio/afe/iio-rescale.c | 85 +++++++++++++++++++++++++++++++++ include/linux/iio/afe/rescale.h | 2 + 2 files changed, 87 insertions(+)