[PATCH 0/4] iio: Use scan_type shift and realbits when processing raw data

[PATCH 0/4] iio: Use scan_type shift and realbits when processing raw data

[Gwendal Grignou]

Using scan_type has source of truth, use shift and realbits instead of
constants when processing reading sensor registers to produce raw sysfs
entries.
The same shit and realbits are already used by the libiio user-space
library to present channel information from device buffer.

Fix only a handful of drivers, where channel scan_type was accessible
in the function handling the raw data request.

In mpl3115, use a 16 bit big endian buffer when reading temperature
channel to improve readability.

Gwendal Grignou (4):
  iio: Use scan_type shift and realbits when processing raw data
  iio: ti-ads1015: Remove shift variable ads1015_read_raw
  iio: xilinx-xadc-core: Use local variable in xadc_read_raw
  iio: mpl3115: Use scan_type.shift and realbit in mpl3115_read_raw

 drivers/iio/accel/bma220_spi.c     |  3 ++-
 drivers/iio/accel/kxcjk-1013.c     |  3 ++-
 drivers/iio/accel/mma7455_core.c   |  3 ++-
 drivers/iio/accel/sca3000.c        |  5 +++--
 drivers/iio/accel/stk8312.c        |  2 +-
 drivers/iio/accel/stk8ba50.c       |  3 ++-
 drivers/iio/adc/ad7266.c           |  3 ++-
 drivers/iio/adc/at91-sama5d2_adc.c |  3 ++-
 drivers/iio/adc/ti-adc12138.c      |  3 ++-
 drivers/iio/adc/ti-ads1015.c       |  8 +++-----
 drivers/iio/adc/xilinx-xadc-core.c |  2 +-
 drivers/iio/magnetometer/mag3110.c |  6 ++++--
 drivers/iio/pressure/mpl3115.c     | 16 +++++++++++-----
 13 files changed, 37 insertions(+), 23 deletions(-)

-- 
2.33.1.1089.g2158813163f-goog

[PATCH 1/4] iio: Use scan_type shift and realbits when processing raw data

[Gwendal Grignou]

When user space application read iio buffer though libiio, data is
converted (see iio_channel_convert()) using the _type sysfs parameter.
In particular, scan_type.shift and scan_type.realbits are used to shift
and tell on how many bits signed elements are encoded on.

When reading elements directly using the raw sysfs attributes, the same
rules for shifting and signing should apply.

Use channel definition as root of trust and replace constant with
them for the simple cases.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/iio/accel/bma220_spi.c     | 3 ++-
 drivers/iio/accel/kxcjk-1013.c     | 3 ++-
 drivers/iio/accel/mma7455_core.c   | 3 ++-
 drivers/iio/accel/sca3000.c        | 5 +++--
 drivers/iio/accel/stk8312.c        | 2 +-
 drivers/iio/accel/stk8ba50.c       | 3 ++-
 drivers/iio/adc/ad7266.c           | 3 ++-
 drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
 drivers/iio/adc/ti-adc12138.c      | 3 ++-
 drivers/iio/magnetometer/mag3110.c | 6 ++++--
 10 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index bc4c626e454d3..812d6749b24a7 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -125,7 +125,8 @@ static int bma220_read_raw(struct iio_dev *indio_dev,
 		ret = bma220_read_reg(data->spi_device, chan->address);
 		if (ret < 0)
 			return -EINVAL;
-		*val = sign_extend32(ret >> BMA220_DATA_SHIFT, 5);
+		*val = sign_extend32(ret >> chan->scan_type.shift,
+				     chan->scan_type.realbits - 1);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		ret = bma220_read_reg(data->spi_device, BMA220_REG_RANGE);
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index a51fdd3c9b5b5..88cf0c276893a 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -927,7 +927,8 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
 				mutex_unlock(&data->mutex);
 				return ret;
 			}
-			*val = sign_extend32(ret >> 4, 11);
+			*val = sign_extend32(ret >> chan->scan_type.shift,
+					     chan->scan_type.realbits - 1);
 			ret = kxcjk1013_set_power_state(data, false);
 		}
 		mutex_unlock(&data->mutex);
diff --git a/drivers/iio/accel/mma7455_core.c b/drivers/iio/accel/mma7455_core.c
index 777c6c384b09e..e6739ba74edfa 100644
--- a/drivers/iio/accel/mma7455_core.c
+++ b/drivers/iio/accel/mma7455_core.c
@@ -134,7 +134,8 @@ static int mma7455_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
-		*val = sign_extend32(le16_to_cpu(data), 9);
+		*val = sign_extend32(le16_to_cpu(data),
+				     chan->scan_type.realbits - 1);
 
 		return IIO_VAL_INT;
 
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index c6b75308148aa..938eb6bda73b3 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -730,8 +730,9 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
 				mutex_unlock(&st->lock);
 				return ret;
 			}
-			*val = (be16_to_cpup((__be16 *)st->rx) >> 3) & 0x1FFF;
-			*val = sign_extend32(*val, 12);
+			*val = (be16_to_cpup((__be16 *)st->rx) >>
+					chan->scan_type.shift) & 0x1FFF;
+			*val = sign_extend32(*val, chan->scan_type.realbits - 1);
 		} else {
 			/* get the temperature when available */
 			ret = sca3000_read_data_short(st,
diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 43c621d0f11e4..de0cdf8c1f94c 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -355,7 +355,7 @@ static int stk8312_read_raw(struct iio_dev *indio_dev,
 			mutex_unlock(&data->lock);
 			return ret;
 		}
-		*val = sign_extend32(ret, 7);
+		*val = sign_extend32(ret, chan->scan_type.realbits - 1);
 		ret = stk8312_set_mode(data,
 				       data->mode & (~STK8312_MODE_ACTIVE));
 		mutex_unlock(&data->lock);
diff --git a/drivers/iio/accel/stk8ba50.c b/drivers/iio/accel/stk8ba50.c
index e137a34b5c9a9..517c57ed9e949 100644
--- a/drivers/iio/accel/stk8ba50.c
+++ b/drivers/iio/accel/stk8ba50.c
@@ -227,7 +227,8 @@ static int stk8ba50_read_raw(struct iio_dev *indio_dev,
 			mutex_unlock(&data->lock);
 			return -EINVAL;
 		}
-		*val = sign_extend32(ret >> STK8BA50_DATA_SHIFT, 9);
+		*val = sign_extend32(ret >> chan->scan_type.shift,
+				     chan->scan_type.realbits - 1);
 		stk8ba50_set_power(data, STK8BA50_MODE_SUSPEND);
 		mutex_unlock(&data->lock);
 		return IIO_VAL_INT;
diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index a8ec3efd659ed..1d345d66742d8 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -159,7 +159,8 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
 
 		*val = (*val >> 2) & 0xfff;
 		if (chan->scan_type.sign == 's')
-			*val = sign_extend32(*val, 11);
+			*val = sign_extend32(*val,
+					     chan->scan_type.realbits - 1);
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 4c922ef634f8e..92a57cf10fba4 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1586,7 +1586,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 		*val = st->conversion_value;
 		ret = at91_adc_adjust_val_osr(st, val);
 		if (chan->scan_type.sign == 's')
-			*val = sign_extend32(*val, 11);
+			*val = sign_extend32(*val,
+					     chan->scan_type.realbits - 1);
 		st->conversion_done = false;
 	}
 
diff --git a/drivers/iio/adc/ti-adc12138.c b/drivers/iio/adc/ti-adc12138.c
index fcd5d39dd03ea..5b5d452105393 100644
--- a/drivers/iio/adc/ti-adc12138.c
+++ b/drivers/iio/adc/ti-adc12138.c
@@ -239,7 +239,8 @@ static int adc12138_read_raw(struct iio_dev *iio,
 		if (ret)
 			return ret;
 
-		*value = sign_extend32(be16_to_cpu(data) >> 3, 12);
+		*value = sign_extend32(be16_to_cpu(data) >> channel->scan_type.shift,
+				       channel->scan_type.realbits - 1);
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
diff --git a/drivers/iio/magnetometer/mag3110.c b/drivers/iio/magnetometer/mag3110.c
index c96415a1aeadd..17c62d806218d 100644
--- a/drivers/iio/magnetometer/mag3110.c
+++ b/drivers/iio/magnetometer/mag3110.c
@@ -291,7 +291,8 @@ static int mag3110_read_raw(struct iio_dev *indio_dev,
 			if (ret < 0)
 				goto release;
 			*val = sign_extend32(
-				be16_to_cpu(buffer[chan->scan_index]), 15);
+				be16_to_cpu(buffer[chan->scan_index]),
+					    chan->scan_type.realbits - 1);
 			ret = IIO_VAL_INT;
 			break;
 		case IIO_TEMP: /* in 1 C / LSB */
@@ -306,7 +307,8 @@ static int mag3110_read_raw(struct iio_dev *indio_dev,
 			mutex_unlock(&data->lock);
 			if (ret < 0)
 				goto release;
-			*val = sign_extend32(ret, 7);
+			*val = sign_extend32(ret,
+					     chan->scan_type.realbits - 1);
 			ret = IIO_VAL_INT;
 			break;
 		default:
-- 
2.33.1.1089.g2158813163f-goog

[PATCH 2/4] iio: ti-ads1015: Remove shift variable ads1015_read_raw

[Gwendal Grignou]

By using scan_type.realbits when processing raw data,
we use scan_type.shit only once, thus we don't need to define a local
variable for it anymore.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/iio/adc/ti-ads1015.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index b0352e91ac165..b92d4cd1b8238 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -464,9 +464,7 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
 
 	mutex_lock(&data->lock);
 	switch (mask) {
-	case IIO_CHAN_INFO_RAW: {
-		int shift = chan->scan_type.shift;
-
+	case IIO_CHAN_INFO_RAW:
 		ret = iio_device_claim_direct_mode(indio_dev);
 		if (ret)
 			break;
@@ -487,7 +485,8 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
 			goto release_direct;
 		}
 
-		*val = sign_extend32(*val >> shift, 15 - shift);
+		*val = sign_extend32(*val >> chan->scan_type.shift,
+				     chan->scan_type.realbits - 1);
 
 		ret = ads1015_set_power_state(data, false);
 		if (ret < 0)
@@ -497,7 +496,6 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
 release_direct:
 		iio_device_release_direct_mode(indio_dev);
 		break;
-	}
 	case IIO_CHAN_INFO_SCALE:
 		idx = data->channel_data[chan->address].pga;
 		*val = ads1015_fullscale_range[idx];
-- 
2.33.1.1089.g2158813163f-goog

[PATCH 3/4] iio: xilinx-xadc-core: Use local variable in xadc_read_raw

[Gwendal Grignou]

Minor cleanup: bit is already defined as chan->scan_type.realbits,
use bit when needed.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/iio/adc/xilinx-xadc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 83bea5ef765da..05050709e4f8a 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -943,7 +943,7 @@ static int xadc_read_raw(struct iio_dev *indio_dev,
 				*val = 1000;
 				break;
 			}
-			*val2 = chan->scan_type.realbits;
+			*val2 = bits;
 			return IIO_VAL_FRACTIONAL_LOG2;
 		case IIO_TEMP:
 			/* Temp in C = (val * 503.975) / 2**bits - 273.15 */
-- 
2.33.1.1089.g2158813163f-goog

[PATCH 4/4] iio: mpl3115: Use scan_type.shift and realbit in mpl3115_read_raw

[Gwendal Grignou]

When processing raw data using channel scan_type.shift as source of
trust to shift data appropriately.
When processing the temperature channel, use a 16bit big endian variable
as buffer to increase conversion readability.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/iio/pressure/mpl3115.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 1eb9e7b29e050..e95b9a5475b4e 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -74,7 +74,6 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
 			    int *val, int *val2, long mask)
 {
 	struct mpl3115_data *data = iio_priv(indio_dev);
-	__be32 tmp = 0;
 	int ret;
 
 	switch (mask) {
@@ -84,7 +83,9 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
 			return ret;
 
 		switch (chan->type) {
-		case IIO_PRESSURE: /* in 0.25 pascal / LSB */
+		case IIO_PRESSURE: { /* in 0.25 pascal / LSB */
+			__be32 tmp = 0;
+
 			mutex_lock(&data->lock);
 			ret = mpl3115_request(data);
 			if (ret < 0) {
@@ -96,10 +97,13 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
 			mutex_unlock(&data->lock);
 			if (ret < 0)
 				break;
-			*val = be32_to_cpu(tmp) >> 12;
+			*val = be32_to_cpu(tmp) >> chan->scan_type.shift;
 			ret = IIO_VAL_INT;
 			break;
-		case IIO_TEMP: /* in 0.0625 celsius / LSB */
+		}
+		case IIO_TEMP: { /* in 0.0625 celsius / LSB */
+			__be16 tmp;
+
 			mutex_lock(&data->lock);
 			ret = mpl3115_request(data);
 			if (ret < 0) {
@@ -111,9 +115,11 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
 			mutex_unlock(&data->lock);
 			if (ret < 0)
 				break;
-			*val = sign_extend32(be32_to_cpu(tmp) >> 20, 11);
+			*val = sign_extend32(be16_to_cpu(tmp) >> chan->scan_type.shift,
+					     chan->scan_type.realbits - 1);
 			ret = IIO_VAL_INT;
 			break;
+		}
 		default:
 			ret = -EINVAL;
 			break;
-- 
2.33.1.1089.g2158813163f-goog

Re: [PATCH 1/4] iio: Use scan_type shift and realbits when processing raw data

[Jonathan Cameron]

On Mon,  1 Nov 2021 00:18:19 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> When user space application read iio buffer though libiio, data is
> converted (see iio_channel_convert()) using the _type sysfs parameter.
> In particular, scan_type.shift and scan_type.realbits are used to shift
> and tell on how many bits signed elements are encoded on.
> 
> When reading elements directly using the raw sysfs attributes, the same
> rules for shifting and signing should apply.
> 
> Use channel definition as root of trust and replace constant with
> them for the simple cases.
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>

Hmm. I'd have been tempted to do this as a long series, at least
partly so it would let me pick up the ones I'm happy with + also
we may create some history that needs backporting at some stage
and that's a mess if we have code touching lots of drivers in one patch.

Ludovic, Eugen,  This change raised a question about the current
code in at91-sama5d2_adc.c

> ---
>  drivers/iio/accel/bma220_spi.c     | 3 ++-
>  drivers/iio/accel/kxcjk-1013.c     | 3 ++-
>  drivers/iio/accel/mma7455_core.c   | 3 ++-
>  drivers/iio/accel/sca3000.c        | 5 +++--
>  drivers/iio/accel/stk8312.c        | 2 +-
>  drivers/iio/accel/stk8ba50.c       | 3 ++-
>  drivers/iio/adc/ad7266.c           | 3 ++-
>  drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
>  drivers/iio/adc/ti-adc12138.c      | 3 ++-
>  drivers/iio/magnetometer/mag3110.c | 6 ++++--
>  10 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> index bc4c626e454d3..812d6749b24a7 100644
> --- a/drivers/iio/accel/bma220_spi.c
> +++ b/drivers/iio/accel/bma220_spi.c
> @@ -125,7 +125,8 @@ static int bma220_read_raw(struct iio_dev *indio_dev,
>  		ret = bma220_read_reg(data->spi_device, chan->address);
>  		if (ret < 0)
>  			return -EINVAL;
> -		*val = sign_extend32(ret >> BMA220_DATA_SHIFT, 5);
> +		*val = sign_extend32(ret >> chan->scan_type.shift,
> +				     chan->scan_type.realbits - 1);

The BMA220_DATA_SHIFT define is now only used as the value for .shift, so
could you move the value inline to there and get rid of the define.

That will be match what is done for all the other parts of scan_type.
 
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		ret = bma220_read_reg(data->spi_device, BMA220_REG_RANGE);


> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index c6b75308148aa..938eb6bda73b3 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -730,8 +730,9 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
>  				mutex_unlock(&st->lock);
>  				return ret;
>  			}
> -			*val = (be16_to_cpup((__be16 *)st->rx) >> 3) & 0x1FFF;
> -			*val = sign_extend32(*val, 12);
> +			*val = (be16_to_cpup((__be16 *)st->rx) >>
> +					chan->scan_type.shift) & 0x1FFF;

While here, GENMASK(12, 0) for the mask might be a nice to have..

> +			*val = sign_extend32(*val, chan->scan_type.realbits - 1);
>  		} else {
>  			/* get the temperature when available */
>  			ret = sca3000_read_data_short(st,
The code following this is exciting as well... and would benefit from
being rewritten as be16_to_cpup() with a shift and mask but that's a job for
a different patch, or you could add the scan_type to the temperature channel and
add it to this series using that... It's unsigned unlike the above, so
it probably doesn't make sense to have a single path.




> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 4c922ef634f8e..92a57cf10fba4 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1586,7 +1586,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  		*val = st->conversion_value;
>  		ret = at91_adc_adjust_val_osr(st, val);
>  		if (chan->scan_type.sign == 's')
> -			*val = sign_extend32(*val, 11);
> +			*val = sign_extend32(*val,
> +					     chan->scan_type.realbits - 1);
>  		st->conversion_done = false;
Hmm. This is exciting.  I'm not sure if the current code is correct.
There is a comment that says it's a voltage channel if we are in this path
a few lines earlier, yet the only signed voltage channel is from the
macro AT91_SAMA5D2_CHAN_DIFF() which sets realbits = 14, not 12.


>  	}
>  

The other changes all looked good to me.

Jonathan

Re: [PATCH 2/4] iio: ti-ads1015: Remove shift variable ads1015_read_raw

[Jonathan Cameron]

On Mon,  1 Nov 2021 00:18:20 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> By using scan_type.realbits when processing raw data,
> we use scan_type.shit only once, thus we don't need to define a local

Possibly Freudian typo..

> variable for it anymore.
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Otherwise looks fine to me and works for both
realbits = 16, shift = 0
realbits = 12, shift = 4

Jonathan

> ---
>  drivers/iio/adc/ti-ads1015.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index b0352e91ac165..b92d4cd1b8238 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -464,9 +464,7 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&data->lock);
>  	switch (mask) {
> -	case IIO_CHAN_INFO_RAW: {
> -		int shift = chan->scan_type.shift;
> -
> +	case IIO_CHAN_INFO_RAW:
>  		ret = iio_device_claim_direct_mode(indio_dev);
>  		if (ret)
>  			break;
> @@ -487,7 +485,8 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>  			goto release_direct;
>  		}
>  
> -		*val = sign_extend32(*val >> shift, 15 - shift);
> +		*val = sign_extend32(*val >> chan->scan_type.shift,
> +				     chan->scan_type.realbits - 1);
>  
>  		ret = ads1015_set_power_state(data, false);
>  		if (ret < 0)
> @@ -497,7 +496,6 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
>  release_direct:
>  		iio_device_release_direct_mode(indio_dev);
>  		break;
> -	}
>  	case IIO_CHAN_INFO_SCALE:
>  		idx = data->channel_data[chan->address].pga;
>  		*val = ads1015_fullscale_range[idx];

Re: [PATCH 3/4] iio: xilinx-xadc-core: Use local variable in xadc_read_raw

[Jonathan Cameron]

On Mon,  1 Nov 2021 00:18:21 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> Minor cleanup: bit is already defined as chan->scan_type.realbits,
> use bit when needed.
> 
bits

> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Otherwise looks good to me.

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 83bea5ef765da..05050709e4f8a 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -943,7 +943,7 @@ static int xadc_read_raw(struct iio_dev *indio_dev,
>  				*val = 1000;
>  				break;
>  			}
> -			*val2 = chan->scan_type.realbits;
> +			*val2 = bits;
>  			return IIO_VAL_FRACTIONAL_LOG2;
>  		case IIO_TEMP:
>  			/* Temp in C = (val * 503.975) / 2**bits - 273.15 */

Re: [PATCH 4/4] iio: mpl3115: Use scan_type.shift and realbit in mpl3115_read_raw

[Jonathan Cameron]

On Mon,  1 Nov 2021 00:18:22 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> When processing raw data using channel scan_type.shift as source of
> trust to shift data appropriately.
> When processing the temperature channel, use a 16bit big endian variable
> as buffer to increase conversion readability.
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>

This one looks fine to me, will pick up when rest are read if no
one else comments,

Jonathan

> ---
>  drivers/iio/pressure/mpl3115.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 1eb9e7b29e050..e95b9a5475b4e 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -74,7 +74,6 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	struct mpl3115_data *data = iio_priv(indio_dev);
> -	__be32 tmp = 0;
>  	int ret;
>  
>  	switch (mask) {
> @@ -84,7 +83,9 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  
>  		switch (chan->type) {
> -		case IIO_PRESSURE: /* in 0.25 pascal / LSB */
> +		case IIO_PRESSURE: { /* in 0.25 pascal / LSB */
> +			__be32 tmp = 0;
> +
>  			mutex_lock(&data->lock);
>  			ret = mpl3115_request(data);
>  			if (ret < 0) {
> @@ -96,10 +97,13 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
>  			mutex_unlock(&data->lock);
>  			if (ret < 0)
>  				break;
> -			*val = be32_to_cpu(tmp) >> 12;
> +			*val = be32_to_cpu(tmp) >> chan->scan_type.shift;
>  			ret = IIO_VAL_INT;
>  			break;
> -		case IIO_TEMP: /* in 0.0625 celsius / LSB */
> +		}
> +		case IIO_TEMP: { /* in 0.0625 celsius / LSB */
> +			__be16 tmp;
> +
>  			mutex_lock(&data->lock);
>  			ret = mpl3115_request(data);
>  			if (ret < 0) {
> @@ -111,9 +115,11 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
>  			mutex_unlock(&data->lock);
>  			if (ret < 0)
>  				break;
> -			*val = sign_extend32(be32_to_cpu(tmp) >> 20, 11);
> +			*val = sign_extend32(be16_to_cpu(tmp) >> chan->scan_type.shift,
> +					     chan->scan_type.realbits - 1);
>  			ret = IIO_VAL_INT;
>  			break;
> +		}
>  		default:
>  			ret = -EINVAL;
>  			break;

Re: [PATCH 1/4] iio: Use scan_type shift and realbits when processing raw data

[Gwendal Grignou]

On Wed, Nov 3, 2021 at 11:37 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon,  1 Nov 2021 00:18:19 -0700
> Gwendal Grignou <gwendal@chromium.org> wrote:
>
> > When user space application read iio buffer though libiio, data is
> > converted (see iio_channel_convert()) using the _type sysfs parameter.
> > In particular, scan_type.shift and scan_type.realbits are used to shift
> > and tell on how many bits signed elements are encoded on.
> >
> > When reading elements directly using the raw sysfs attributes, the same
> > rules for shifting and signing should apply.
> >
> > Use channel definition as root of trust and replace constant with
> > them for the simple cases.
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>
> Hmm. I'd have been tempted to do this as a long series, at least
> partly so it would let me pick up the ones I'm happy with + also
> we may create some history that needs backporting at some stage
> and that's a mess if we have code touching lots of drivers in one patch.
I will split this patch, one per driver so that you can pick only the
safe changes only.
There are many more drivers that would benefit from using scan_type,
but they require more restructuring.

>
> Ludovic, Eugen,  This change raised a question about the current
> code in at91-sama5d2_adc.c
>
> > ---
> >  drivers/iio/accel/bma220_spi.c     | 3 ++-
> >  drivers/iio/accel/kxcjk-1013.c     | 3 ++-
> >  drivers/iio/accel/mma7455_core.c   | 3 ++-
> >  drivers/iio/accel/sca3000.c        | 5 +++--
> >  drivers/iio/accel/stk8312.c        | 2 +-
> >  drivers/iio/accel/stk8ba50.c       | 3 ++-
> >  drivers/iio/adc/ad7266.c           | 3 ++-
> >  drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
> >  drivers/iio/adc/ti-adc12138.c      | 3 ++-
> >  drivers/iio/magnetometer/mag3110.c | 6 ++++--
> >  10 files changed, 22 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> > index bc4c626e454d3..812d6749b24a7 100644
> > --- a/drivers/iio/accel/bma220_spi.c
> > +++ b/drivers/iio/accel/bma220_spi.c
> > @@ -125,7 +125,8 @@ static int bma220_read_raw(struct iio_dev *indio_dev,
> >               ret = bma220_read_reg(data->spi_device, chan->address);
> >               if (ret < 0)
> >                       return -EINVAL;
> > -             *val = sign_extend32(ret >> BMA220_DATA_SHIFT, 5);
> > +             *val = sign_extend32(ret >> chan->scan_type.shift,
> > +                                  chan->scan_type.realbits - 1);
>
> The BMA220_DATA_SHIFT define is now only used as the value for .shift, so
> could you move the value inline to there and get rid of the define.
>
> That will be match what is done for all the other parts of scan_type.
Done.
>
> >               return IIO_VAL_INT;
> >       case IIO_CHAN_INFO_SCALE:
> >               ret = bma220_read_reg(data->spi_device, BMA220_REG_RANGE);
>
>
> > diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> > index c6b75308148aa..938eb6bda73b3 100644
> > --- a/drivers/iio/accel/sca3000.c
> > +++ b/drivers/iio/accel/sca3000.c
> > @@ -730,8 +730,9 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
> >                               mutex_unlock(&st->lock);
> >                               return ret;
> >                       }
> > -                     *val = (be16_to_cpup((__be16 *)st->rx) >> 3) & 0x1FFF;
> > -                     *val = sign_extend32(*val, 12);
> > +                     *val = (be16_to_cpup((__be16 *)st->rx) >>
> > +                                     chan->scan_type.shift) & 0x1FFF;
>
> While here, GENMASK(12, 0) for the mask might be a nice to have..
Actually, it may not be needed at all: we push a 16bit field 3 bits to
the left, so 13 bits remain anyway.
>
> > +                     *val = sign_extend32(*val, chan->scan_type.realbits - 1);
> >               } else {
> >                       /* get the temperature when available */
> >                       ret = sca3000_read_data_short(st,
> The code following this is exciting as well... and would benefit from
> being rewritten as be16_to_cpup() with a shift and mask but that's a job for
> a different patch, or you could add the scan_type to the temperature channel and
> add it to this series using that... It's unsigned unlike the above, so
> it probably doesn't make sense to have a single path.
Done
>
>
>
>
> > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> > index 4c922ef634f8e..92a57cf10fba4 100644
> > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > @@ -1586,7 +1586,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >               *val = st->conversion_value;
> >               ret = at91_adc_adjust_val_osr(st, val);
> >               if (chan->scan_type.sign == 's')
> > -                     *val = sign_extend32(*val, 11);
> > +                     *val = sign_extend32(*val,
> > +                                          chan->scan_type.realbits - 1);
> >               st->conversion_done = false;
> Hmm. This is exciting.  I'm not sure if the current code is correct.
> There is a comment that says it's a voltage channel if we are in this path
> a few lines earlier, yet the only signed voltage channel is from the
> macro AT91_SAMA5D2_CHAN_DIFF() which sets realbits = 14, not 12.
From https://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf,
the ADC is natively 12 bits, but can be configured to oversample to
reach 14 bits resolution.
|realbit| may actually be a function of
IIO_CHAN_INFO_OVERSAMPLING_RATIO [aka "oversampling_ratio"] value.
>
>
> >       }
> >
>
> The other changes all looked good to me.
Thanks,
Gwendal.
>
> Jonathan
>

Re: [PATCH 1/4] iio: Use scan_type shift and realbits when processing raw data

[Eugen.Hristev]

On 11/4/21 10:24 AM, Gwendal Grignou wrote:
> On Wed, Nov 3, 2021 at 11:37 AM Jonathan Cameron <jic23@kernel.org> wrote:
>>
>> On Mon,  1 Nov 2021 00:18:19 -0700
>> Gwendal Grignou <gwendal@chromium.org> wrote:
>>
>>> When user space application read iio buffer though libiio, data is
>>> converted (see iio_channel_convert()) using the _type sysfs parameter.
>>> In particular, scan_type.shift and scan_type.realbits are used to shift
>>> and tell on how many bits signed elements are encoded on.
>>>
>>> When reading elements directly using the raw sysfs attributes, the same
>>> rules for shifting and signing should apply.
>>>
>>> Use channel definition as root of trust and replace constant with
>>> them for the simple cases.
>>>
>>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>>
>> Hmm. I'd have been tempted to do this as a long series, at least
>> partly so it would let me pick up the ones I'm happy with + also
>> we may create some history that needs backporting at some stage
>> and that's a mess if we have code touching lots of drivers in one patch.
> I will split this patch, one per driver so that you can pick only the
> safe changes only.
> There are many more drivers that would benefit from using scan_type,
> but they require more restructuring.
> 
>>
>> Ludovic, Eugen,  This change raised a question about the current
>> code in at91-sama5d2_adc.c
>>
>>> ---
>>>   drivers/iio/accel/bma220_spi.c     | 3 ++-
>>>   drivers/iio/accel/kxcjk-1013.c     | 3 ++-
>>>   drivers/iio/accel/mma7455_core.c   | 3 ++-
>>>   drivers/iio/accel/sca3000.c        | 5 +++--
>>>   drivers/iio/accel/stk8312.c        | 2 +-
>>>   drivers/iio/accel/stk8ba50.c       | 3 ++-
>>>   drivers/iio/adc/ad7266.c           | 3 ++-
>>>   drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
>>>   drivers/iio/adc/ti-adc12138.c      | 3 ++-
>>>   drivers/iio/magnetometer/mag3110.c | 6 ++++--
>>>   10 files changed, 22 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
>>> index bc4c626e454d3..812d6749b24a7 100644
>>> --- a/drivers/iio/accel/bma220_spi.c
>>> +++ b/drivers/iio/accel/bma220_spi.c
>>> @@ -125,7 +125,8 @@ static int bma220_read_raw(struct iio_dev *indio_dev,
>>>                ret = bma220_read_reg(data->spi_device, chan->address);
>>>                if (ret < 0)
>>>                        return -EINVAL;
>>> -             *val = sign_extend32(ret >> BMA220_DATA_SHIFT, 5);
>>> +             *val = sign_extend32(ret >> chan->scan_type.shift,
>>> +                                  chan->scan_type.realbits - 1);
>>
>> The BMA220_DATA_SHIFT define is now only used as the value for .shift, so
>> could you move the value inline to there and get rid of the define.
>>
>> That will be match what is done for all the other parts of scan_type.
> Done.
>>
>>>                return IIO_VAL_INT;
>>>        case IIO_CHAN_INFO_SCALE:
>>>                ret = bma220_read_reg(data->spi_device, BMA220_REG_RANGE);
>>
>>
>>> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
>>> index c6b75308148aa..938eb6bda73b3 100644
>>> --- a/drivers/iio/accel/sca3000.c
>>> +++ b/drivers/iio/accel/sca3000.c
>>> @@ -730,8 +730,9 @@ static int sca3000_read_raw(struct iio_dev *indio_dev,
>>>                                mutex_unlock(&st->lock);
>>>                                return ret;
>>>                        }
>>> -                     *val = (be16_to_cpup((__be16 *)st->rx) >> 3) & 0x1FFF;
>>> -                     *val = sign_extend32(*val, 12);
>>> +                     *val = (be16_to_cpup((__be16 *)st->rx) >>
>>> +                                     chan->scan_type.shift) & 0x1FFF;
>>
>> While here, GENMASK(12, 0) for the mask might be a nice to have..
> Actually, it may not be needed at all: we push a 16bit field 3 bits to
> the left, so 13 bits remain anyway.
>>
>>> +                     *val = sign_extend32(*val, chan->scan_type.realbits - 1);
>>>                } else {
>>>                        /* get the temperature when available */
>>>                        ret = sca3000_read_data_short(st,
>> The code following this is exciting as well... and would benefit from
>> being rewritten as be16_to_cpup() with a shift and mask but that's a job for
>> a different patch, or you could add the scan_type to the temperature channel and
>> add it to this series using that... It's unsigned unlike the above, so
>> it probably doesn't make sense to have a single path.
> Done
>>
>>
>>
>>
>>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>>> index 4c922ef634f8e..92a57cf10fba4 100644
>>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>>> @@ -1586,7 +1586,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>>                *val = st->conversion_value;
>>>                ret = at91_adc_adjust_val_osr(st, val);
>>>                if (chan->scan_type.sign == 's')
>>> -                     *val = sign_extend32(*val, 11);
>>> +                     *val = sign_extend32(*val,
>>> +                                          chan->scan_type.realbits - 1);
>>>                st->conversion_done = false;
>> Hmm. This is exciting.  I'm not sure if the current code is correct.
>> There is a comment that says it's a voltage channel if we are in this path
>> a few lines earlier, yet the only signed voltage channel is from the
>> macro AT91_SAMA5D2_CHAN_DIFF() which sets realbits = 14, not 12.
>  From https://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf,
> the ADC is natively 12 bits, but can be configured to oversample to
> reach 14 bits resolution.
> |realbit| may actually be a function of
> IIO_CHAN_INFO_OVERSAMPLING_RATIO [aka "oversampling_ratio"] value.
>>

Hi,

Ever since the oversampling was implemented, the realbits was changed to 14.
This means that if oversampling is enabled, the data will have 14 bits 
of precision.
If oversampling is disabled, it will have 12 bits of useful data, but it 
will be shifted to the left by 2 bits, so in the end the result will be 
14 bits, but with LSB 2 bits always zero. ( this is what 
at91_adc_adjust_val_osr does )

So I think that maybe a fix for this commit should be done first :
6794e23fa3fe ("iio: adc: at91-sama5d2_adc: add support for oversampling 
resolution")
which does not change the '11' value to '13'
and then you can apply your commit that starts using the 'realbits' field.

So in fact, another bit was used to obtain the sign.. I wonder if 
someone uses these differential channels since I haven't heard of any 
problem reported. And during my tests it looked to be fine, but maybe it 
was a bit coincidence.

If I get my hands on a signal generator I will try to make a 
differential GND to a sinus waveform and plot the results to see if it 
works as expected

Nice catch and thanks,
Eugen


>>
>>>        }
>>>
>>
>> The other changes all looked good to me.
> Thanks,
> Gwendal.
>>
>> Jonathan
>>