[PATCH 0/1] iio/scmi: Add reading "raw" attribute.

[PATCH 0/1] iio/scmi: Add reading "raw" attribute.

[Andriy Tryshnivskyy]

This patch implements reading "raw" attribute.

The patch is based on v5.14.

Any comments are very welcome.

Thanks,
Andriy.

Andriy Tryshnivskyy (1):
  iio/scmi: Add reading "raw" attribute.

 drivers/iio/common/scmi_sensors/scmi_iio.c | 38 +++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)


base-commit: 7d2a07b769330c34b4deabeed939325c77a7ec2f
-- 
2.17.1

[PATCH 1/1] iio/scmi: Add reading "raw" attribute.

[Andriy Tryshnivskyy]

Add IIO_CHAN_INFO_RAW to the mask and implement corresponding
reading "raw" attribute in scmi_iio_read_raw.

Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
---
 drivers/iio/common/scmi_sensors/scmi_iio.c | 38 +++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 7cf2bf282cef..b88780a25796 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -286,6 +286,9 @@ static int scmi_iio_read_raw(struct iio_dev *iio_dev,
 	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
 	s8 scale;
 	int ret;
+	int err;
+	u32 sensor_config;
+	struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
@@ -300,6 +303,38 @@ static int scmi_iio_read_raw(struct iio_dev *iio_dev,
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		ret = scmi_iio_get_odr_val(iio_dev, val, val2);
 		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_RAW:
+		sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
+					   SCMI_SENS_CFG_SENSOR_ENABLE);
+		err = sensor->handle->sensor_ops->config_set(
+			sensor->handle, sensor->sensor_info->id, sensor_config);
+		if (err)
+			dev_err(&iio_dev->dev,
+				"Error in enabling sensor %s err %d",
+				sensor->sensor_info->name, err);
+
+		err = sensor->handle->sensor_ops->reading_get_timestamped(
+			sensor->handle, sensor->sensor_info->id,
+			sensor->sensor_info->num_axis,
+			(struct scmi_sensor_reading *)&readings);
+		if (err) {
+			dev_err(&iio_dev->dev,
+				"Error in reading raw attribute for sensor %s err %d",
+				sensor->sensor_info->name, err);
+			return err;
+		}
+
+		sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
+					   SCMI_SENS_CFG_SENSOR_DISABLE);
+		err = sensor->handle->sensor_ops->config_set(
+			sensor->handle, sensor->sensor_info->id, sensor_config);
+		if (err)
+			dev_err(&iio_dev->dev,
+				"Error in enabling sensor %s err %d",
+				sensor->sensor_info->name, err);
+		/* Use 32-bit value, since practically there is no need in 64 bits */
+		*val = (u32)readings[ch->scan_index].value;
+		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
@@ -381,7 +416,8 @@ static void scmi_iio_set_data_channel(struct iio_chan_spec *iio_chan,
 	iio_chan->type = type;
 	iio_chan->modified = 1;
 	iio_chan->channel2 = mod;
-	iio_chan->info_mask_separate = BIT(IIO_CHAN_INFO_SCALE);
+	iio_chan->info_mask_separate =
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_RAW);
 	iio_chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ);
 	iio_chan->info_mask_shared_by_type_available =
 		BIT(IIO_CHAN_INFO_SAMP_FREQ);
-- 
2.17.1

Re: [PATCH 1/1] iio/scmi: Add reading "raw" attribute.

[Jonathan Cameron]

On Wed, 22 Sep 2021 09:52:35 +0300
Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com> wrote:

> Add IIO_CHAN_INFO_RAW to the mask and implement corresponding
> reading "raw" attribute in scmi_iio_read_raw.
> 
> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>

Hi Andriy,

A few comments inline.  I don't haven't looked closely at whether the scmi
side of things is correct though so will rely on Jyoti for that.

Thanks,

Jonathan

> ---
>  drivers/iio/common/scmi_sensors/scmi_iio.c | 38 +++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
> index 7cf2bf282cef..b88780a25796 100644
> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -286,6 +286,9 @@ static int scmi_iio_read_raw(struct iio_dev *iio_dev,
>  	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
>  	s8 scale;
>  	int ret;
> +	int err;
> +	u32 sensor_config;
> +	struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -300,6 +303,38 @@ static int scmi_iio_read_raw(struct iio_dev *iio_dev,
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		ret = scmi_iio_get_odr_val(iio_dev, val, val2);
>  		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_RAW:
> +		sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
> +					   SCMI_SENS_CFG_SENSOR_ENABLE);
> +		err = sensor->handle->sensor_ops->config_set(
> +			sensor->handle, sensor->sensor_info->id, sensor_config);
> +		if (err)
> +			dev_err(&iio_dev->dev,
> +				"Error in enabling sensor %s err %d",
> +				sensor->sensor_info->name, err);

Don't try to carry on if you got an error. Unless something very odd has gone on
that should mean the sensor isn't not enabled so nothing else will work.
If this returns and error just return it here.

> +
> +		err = sensor->handle->sensor_ops->reading_get_timestamped(
> +			sensor->handle, sensor->sensor_info->id,
> +			sensor->sensor_info->num_axis,
> +			(struct scmi_sensor_reading *)&readings);

Why do we need the cast?   Also, this should probably just be readings
unless I'm missing something rather than the address of readings.
That will pass the address of the first element which is likely to be what you want.

> +		if (err) {
> +			dev_err(&iio_dev->dev,
> +				"Error in reading raw attribute for sensor %s err %d",
> +				sensor->sensor_info->name, err);
> +			return err;
> +		}
> +
> +		sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
> +					   SCMI_SENS_CFG_SENSOR_DISABLE);
> +		err = sensor->handle->sensor_ops->config_set(
> +			sensor->handle, sensor->sensor_info->id, sensor_config);
> +		if (err)
> +			dev_err(&iio_dev->dev,
> +				"Error in enabling sensor %s err %d",
> +				sensor->sensor_info->name, err);

As above, this indicates something has gone wrong and we should tell userspace that
in preference to trying to get one last value out.

> +		/* Use 32-bit value, since practically there is no need in 64 bits */
> +		*val = (u32)readings[ch->scan_index].value;

We should check it fits and if doesn't return an error rather than pretending all was fine.

> +		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -381,7 +416,8 @@ static void scmi_iio_set_data_channel(struct iio_chan_spec *iio_chan,
>  	iio_chan->type = type;
>  	iio_chan->modified = 1;
>  	iio_chan->channel2 = mod;
> -	iio_chan->info_mask_separate = BIT(IIO_CHAN_INFO_SCALE);
> +	iio_chan->info_mask_separate =
> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_RAW);
>  	iio_chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ);
>  	iio_chan->info_mask_shared_by_type_available =
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ);

Re: [PATCH 1/1] iio/scmi: Add reading "raw" attribute.

[Jonathan Cameron]

> >> +             /* Use 32-bit value, since practically there is no need in 64 bits */
> >> +             *val = (u32)readings[ch->scan_index].value;  
> > We should check it fits and if doesn't return an error rather than pretending all was fine.
> >  
> What kind of error to return in the case it does not fit - ENOEXEC (Exec format error) ?

That one is a bit obscure.

I'd go with either -ERANGE I think as we are reflecting that the clamp to 32 bit
is not representable.

Re: [PATCH 1/1] iio/scmi: Add reading "raw" attribute.

[Andriy Tryshnivskyy]

>>>> +             /* Use 32-bit value, since practically there is no need in 64 bits */
>>>> +             *val = (u32)readings[ch->scan_index].value;
>>> We should check it fits and if doesn't return an error rather than pretending all was fine.
>>>
>> What kind of error to return in the case it does not fit - ENOEXEC (Exec format error) ?
> That one is a bit obscure.
>
> I'd go with either -ERANGE I think as we are reflecting that the clamp to 32 bit
> is not representable.

Thanks for hint. Later I've found -ERANGE also and used it in my next patch versions.