On Sun Jul 4, 2021 at 12:26 PM EDT, Jonathan Cameron wrote:

On Wed, 30 Jun 2021 21:00:27 -0400
Liam Beguin [off-list ref] wrote:

quoted

From: Liam Beguin <redacted>

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 <redacted>

Hi Jonathan,

Thanks for taking the time to review this again.

Looks good, but a few really minor comments / questions inline.

Thanks,

Jonathan

quoted

---
 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

quoted

+		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

quoted

+			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);