| Message ID | 20210701010034.303088-4-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
On Wed, 30 Jun 2021 21:00:27 -0400 Liam Beguin <liambeguin@gmail.com> wrote: > From: Liam Beguin <lvb@xiphos.com> > > iio_convert_raw_to_processed_unlocked() assumes the offset is an > integer. Make a best effort to get a valid offset value for fractional > cases without breaking implicit truncations. > > Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value") > Signed-off-by: Liam Beguin <lvb@xiphos.com> Looks good, but a few really minor comments / questions inline. Thanks, Jonathan > --- > drivers/iio/inkern.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c > index b69027690ed5..e1712c1099c5 100644 > --- a/drivers/iio/inkern.c > +++ b/drivers/iio/inkern.c > @@ -578,13 +578,39 @@ EXPORT_SYMBOL_GPL(iio_read_channel_average_raw); > static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan, > int raw, int *processed, unsigned int scale) > { > - int scale_type, scale_val, scale_val2, offset; > + int scale_type, scale_val, scale_val2; > + int offset_type, offset_val, offset_val2; > s64 raw64 = raw; > - int ret; > + int tmp; > > - ret = iio_channel_read(chan, &offset, NULL, IIO_CHAN_INFO_OFFSET); > - if (ret >= 0) > - raw64 += offset; > + offset_type = iio_channel_read(chan, &offset_val, &offset_val2, > + IIO_CHAN_INFO_OFFSET); > + if (offset_type >= 0) { > + switch (offset_type) { > + case IIO_VAL_INT: > + break; > + case IIO_VAL_INT_PLUS_MICRO: > + fallthrough; I'm fairly sure you don't need to mark fallthroughs in the case where there is nothing in the case statement at all. That case is assumed to be deliberate by the various static checkers. I am seeing a few examples as you have it here in kernel, but it certainly isn't particularly common so I'm assuming those where the result of people falsely thinking it was necessary or the outcomes of code changes in the surrounding code. > + case IIO_VAL_INT_PLUS_NANO: > + /* > + * Both IIO_VAL_INT_PLUS_MICRO and IIO_VAL_INT_PLUS_NANO > + * implicitely truncate the offset to it's integer form. > + */ > + break; > + case IIO_VAL_FRACTIONAL: > + tmp = offset_val / offset_val2; > + offset_val = tmp; What benefit do we get from the local variable? offset_val /= offset_val2; would be alternative. > + break; > + case IIO_VAL_FRACTIONAL_LOG2: > + tmp = offset_val / (1 << offset_val2); > + offset_val = tmp; > + break; > + default: > + return -EINVAL; > + } > + > + raw64 += offset_val; > + } > > scale_type = iio_channel_read(chan, &scale_val, &scale_val2, > IIO_CHAN_INFO_SCALE);
On Sun Jul 4, 2021 at 12:26 PM EDT, Jonathan Cameron wrote: > On Wed, 30 Jun 2021 21:00:27 -0400 > Liam Beguin <liambeguin@gmail.com> wrote: > > > From: Liam Beguin <lvb@xiphos.com> > > > > iio_convert_raw_to_processed_unlocked() assumes the offset is an > > integer. Make a best effort to get a valid offset value for fractional > > cases without breaking implicit truncations. > > > > Fixes: 48e44ce0f881 ("iio:inkern: Add function to read the processed value") > > Signed-off-by: Liam Beguin <lvb@xiphos.com> Hi Jonathan, Thanks for taking the time to review this again. > Looks good, but a few really minor comments / questions inline. > > Thanks, > > Jonathan > > > --- > > drivers/iio/inkern.c | 36 +++++++++++++++++++++++++++++++----- > > 1 file changed, 31 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c > > index b69027690ed5..e1712c1099c5 100644 > > --- a/drivers/iio/inkern.c > > +++ b/drivers/iio/inkern.c > > @@ -578,13 +578,39 @@ EXPORT_SYMBOL_GPL(iio_read_channel_average_raw); > > static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan, > > int raw, int *processed, unsigned int scale) > > { > > - int scale_type, scale_val, scale_val2, offset; > > + int scale_type, scale_val, scale_val2; > > + int offset_type, offset_val, offset_val2; > > s64 raw64 = raw; > > - int ret; > > + int tmp; > > > > - ret = iio_channel_read(chan, &offset, NULL, IIO_CHAN_INFO_OFFSET); > > - if (ret >= 0) > > - raw64 += offset; > > + offset_type = iio_channel_read(chan, &offset_val, &offset_val2, > > + IIO_CHAN_INFO_OFFSET); > > + if (offset_type >= 0) { > > + switch (offset_type) { > > + case IIO_VAL_INT: > > + break; > > + case IIO_VAL_INT_PLUS_MICRO: > > + fallthrough; > > I'm fairly sure you don't need to mark fallthroughs in the case where > there is nothing in the case statement at all. That case is assumed > to be deliberate by the various static checkers. I am seeing a few > examples as you have it here in kernel, but it certainly isn't > particularly common > so I'm assuming those where the result of people falsely thinking it was > necessary > or the outcomes of code changes in the surrounding code. > I thought it was always required with `-Wimplicit-fallthrough`. Building without it gives no warnings, and after looking into it a little, I found a bugzilla thread[1] that confirms what you're saying. Thanks for pointing that out. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=7652 > > + case IIO_VAL_INT_PLUS_NANO: > > + /* > > + * Both IIO_VAL_INT_PLUS_MICRO and IIO_VAL_INT_PLUS_NANO > > + * implicitely truncate the offset to it's integer form. > > + */ > > + break; > > + case IIO_VAL_FRACTIONAL: > > + tmp = offset_val / offset_val2; > > + offset_val = tmp; > > What benefit do we get from the local variable? > offset_val /= offset_val2; would be alternative. > Apologies for that, will fix! Thanks, Liam > > + break; > > + case IIO_VAL_FRACTIONAL_LOG2: > > + tmp = offset_val / (1 << offset_val2); > > + offset_val = tmp; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + raw64 += offset_val; > > + } > > > > scale_type = iio_channel_read(chan, &scale_val, &scale_val2, > > IIO_CHAN_INFO_SCALE);
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c index b69027690ed5..e1712c1099c5 100644 --- a/drivers/iio/inkern.c +++ b/drivers/iio/inkern.c @@ -578,13 +578,39 @@ EXPORT_SYMBOL_GPL(iio_read_channel_average_raw); static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan, int raw, int *processed, unsigned int scale) { - int scale_type, scale_val, scale_val2, offset; + int scale_type, scale_val, scale_val2; + int offset_type, offset_val, offset_val2; s64 raw64 = raw; - int ret; + int tmp; - ret = iio_channel_read(chan, &offset, NULL, IIO_CHAN_INFO_OFFSET); - if (ret >= 0) - raw64 += offset; + offset_type = iio_channel_read(chan, &offset_val, &offset_val2, + IIO_CHAN_INFO_OFFSET); + if (offset_type >= 0) { + switch (offset_type) { + case IIO_VAL_INT: + break; + case IIO_VAL_INT_PLUS_MICRO: + fallthrough; + case IIO_VAL_INT_PLUS_NANO: + /* + * Both IIO_VAL_INT_PLUS_MICRO and IIO_VAL_INT_PLUS_NANO + * implicitely truncate the offset to it's integer form. + */ + break; + case IIO_VAL_FRACTIONAL: + tmp = offset_val / offset_val2; + offset_val = tmp; + break; + case IIO_VAL_FRACTIONAL_LOG2: + tmp = offset_val / (1 << offset_val2); + offset_val = tmp; + break; + default: + return -EINVAL; + } + + raw64 += offset_val; + } scale_type = iio_channel_read(chan, &scale_val, &scale_val2, IIO_CHAN_INFO_SCALE);