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

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

[Andriy Tryshnivskyy]

This patch implements reading "raw" attribute.

The patch is based on v5.14.

Changes comparing v4 -> v5:
* call iio_device_release_direct_mode() on error
* code cleanup, fix typo

Changes comparing v3 -> v4:
* do not use scmi_iio_get_raw() for reading raw attribute due to 32bit
  return value limitation (actually I reverted the previous v3)
* introduce scmi_iio_read_raw to scmi_iio_ext_info[] which can return 64 bit
  value
* enabling/disabling and reading raw attribute is done in direct mode

Any comments are very welcome.

Thanks,
Andriy.

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

 drivers/iio/common/scmi_sensors/scmi_iio.c | 61 ++++++++++++++++++++++
 1 file changed, 61 insertions(+)


base-commit: 7d2a07b769330c34b4deabeed939325c77a7ec2f
-- 
2.17.1

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

[Andriy Tryshnivskyy]

Add scmi_iio_get_raw() to read "raw" attribute.

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

diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index 7cf2bf282cef..691cbbd61e3a 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -311,6 +311,62 @@ static const struct iio_info scmi_iio_info = {
 	.write_raw = scmi_iio_write_raw,
 };
 
+static ssize_t scmi_iio_get_raw(struct iio_dev *iio_dev, uintptr_t private,
+				const struct iio_chan_spec *chan, char *buf)
+{
+	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+	int err;
+	u32 sensor_config;
+	struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
+	int len = 0;
+
+	err = iio_device_claim_direct_mode(iio_dev);
+	if (err) {
+		dev_err(&iio_dev->dev,
+			"Error in claiming direct mode for sensor %s err %d",
+			sensor->sensor_info->name, err);
+		goto err_release;
+	}
+
+	sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
+				   SCMI_SENS_CFG_SENSOR_ENABLE);
+	err = sensor->sensor_ops->config_set(
+		sensor->ph, 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);
+		goto err_release;
+	}
+
+	err = sensor->sensor_ops->reading_get_timestamped(
+		sensor->ph, sensor->sensor_info->id,
+		sensor->sensor_info->num_axis, readings);
+	if (err) {
+		dev_err(&iio_dev->dev,
+			"Error in reading raw attribute for sensor %s err %d",
+			sensor->sensor_info->name, err);
+		goto err_release;
+	}
+
+	sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
+				   SCMI_SENS_CFG_SENSOR_DISABLE);
+	err = sensor->sensor_ops->config_set(
+		sensor->ph, sensor->sensor_info->id, sensor_config);
+	if (err) {
+		dev_err(&iio_dev->dev, "Error in disabling sensor %s err %d",
+			sensor->sensor_info->name, err);
+		goto err_release;
+	}
+
+	len = scnprintf(buf, PAGE_SIZE, "%lld\n",
+			readings[chan->scan_index].value);
+
+err_release:
+	iio_device_release_direct_mode(iio_dev);
+
+	return len;
+}
+
 static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
 					  uintptr_t private,
 					  const struct iio_chan_spec *chan,
@@ -355,6 +411,11 @@ static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
 }
 
 static const struct iio_chan_spec_ext_info scmi_iio_ext_info[] = {
+	{
+		.name = "raw",
+		.read = scmi_iio_get_raw,
+		.shared = IIO_SEPARATE,
+	},
 	{
 		.name = "raw_available",
 		.read = scmi_iio_get_raw_available,
-- 
2.17.1

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

[Jonathan Cameron]

On Fri,  8 Oct 2021 21:28:26 +0300
Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com> wrote:

> Add scmi_iio_get_raw() to read "raw" attribute.
> 
> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
> ---
For a single patch series, it is better to put a change log in the patch (here)
Whilst I can see why you would use this approach rather than the read_raw callback
there are significant disadvantages in doing so.  The channel can't be used
by in kernel users such as iio-hwmon.   That may cause you more trouble than
it is worth in the long run.

Note that you could also define a new IIO_VAL type to still use the two
(possibly) 32 bit values and return a 64 bit value. That way, with appropriate
additions in the consumer drivers the channel could still be used.

IIO_VAL_INT_64 perhaps with val as the lower 32 bits and val2 as the upper
with appropriate care around the sign.


>  drivers/iio/common/scmi_sensors/scmi_iio.c | 61 ++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
> index 7cf2bf282cef..691cbbd61e3a 100644
> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -311,6 +311,62 @@ static const struct iio_info scmi_iio_info = {
>  	.write_raw = scmi_iio_write_raw,
>  };
>  
> +static ssize_t scmi_iio_get_raw(struct iio_dev *iio_dev, uintptr_t private,
> +				const struct iio_chan_spec *chan, char *buf)
> +{
> +	struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> +	int err;
> +	u32 sensor_config;
> +	struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
> +	int len = 0;
> +
> +	err = iio_device_claim_direct_mode(iio_dev);
> +	if (err) {
> +		dev_err(&iio_dev->dev,
> +			"Error in claiming direct mode for sensor %s err %d",
> +			sensor->sensor_info->name, err);

It's not an error, it just means the device is busy, so at most dev_info()
or just rely on userspace correctly interpreting EBUSY.

> +		goto err_release;
> +	}
> +
> +	sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
> +				   SCMI_SENS_CFG_SENSOR_ENABLE);
> +	err = sensor->sensor_ops->config_set(
> +		sensor->ph, 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);
> +		goto err_release;
> +	}
> +
> +	err = sensor->sensor_ops->reading_get_timestamped(
> +		sensor->ph, sensor->sensor_info->id,
> +		sensor->sensor_info->num_axis, readings);
> +	if (err) {
> +		dev_err(&iio_dev->dev,
> +			"Error in reading raw attribute for sensor %s err %d",
> +			sensor->sensor_info->name, err);
> +		goto err_release;
> +	}
> +
> +	sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
> +				   SCMI_SENS_CFG_SENSOR_DISABLE);
> +	err = sensor->sensor_ops->config_set(
> +		sensor->ph, sensor->sensor_info->id, sensor_config);
> +	if (err) {
> +		dev_err(&iio_dev->dev, "Error in disabling sensor %s err %d",
> +			sensor->sensor_info->name, err);
> +		goto err_release;
> +	}
> +
> +	len = scnprintf(buf, PAGE_SIZE, "%lld\n",
> +			readings[chan->scan_index].value);
> +
> +err_release:
> +	iio_device_release_direct_mode(iio_dev);
> +
> +	return len;
> +}
> +
>  static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
>  					  uintptr_t private,
>  					  const struct iio_chan_spec *chan,
> @@ -355,6 +411,11 @@ static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
>  }
>  
>  static const struct iio_chan_spec_ext_info scmi_iio_ext_info[] = {
> +	{
> +		.name = "raw",
> +		.read = scmi_iio_get_raw,
> +		.shared = IIO_SEPARATE,
> +	},
>  	{
>  		.name = "raw_available",
>  		.read = scmi_iio_get_raw_available,

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

[Andriy Tryshnivskyy]

On 17.10.21 14:51, Jonathan Cameron wrote:

> CAUTION: This email originated from outside of the organization.
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> On Fri,  8 Oct 2021 21:28:26 +0300
> Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com> wrote:
>
>> Add scmi_iio_get_raw() to read "raw" attribute.
>>
>> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
>> ---
> For a single patch series, it is better to put a change log in the patch (here)
> Whilst I can see why you would use this approach rather than the read_raw callback
> there are significant disadvantages in doing so.  The channel can't be used
> by in kernel users such as iio-hwmon.   That may cause you more trouble than
> it is worth in the long run.
>
> Note that you could also define a new IIO_VAL type to still use the two
> (possibly) 32 bit values and return a 64 bit value. That way, with appropriate
> additions in the consumer drivers the channel could still be used.
>
> IIO_VAL_INT_64 perhaps with val as the lower 32 bits and val2 as the upper
> with appropriate care around the sign.

Hi Jonathan,

In the past I also was thinking about introducing a new type to return 64 bits value - IIO_VAL_INT_LONG.
However since it requires changes in

     include/linux/iio/types.h
     drivers/iio/industrialio-core.c

I thought it will be harder to get approval for this approach.
But now it seems there is no other way. So I will prepare a new patch version soon which introduces IIO_VAL_INT_LONG.

Thank you for your review!

>
>>   drivers/iio/common/scmi_sensors/scmi_iio.c | 61 ++++++++++++++++++++++
>>   1 file changed, 61 insertions(+)
>>
>> diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
>> index 7cf2bf282cef..691cbbd61e3a 100644
>> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
>> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
>> @@ -311,6 +311,62 @@ static const struct iio_info scmi_iio_info = {
>>        .write_raw = scmi_iio_write_raw,
>>   };
>>
>> +static ssize_t scmi_iio_get_raw(struct iio_dev *iio_dev, uintptr_t private,
>> +                             const struct iio_chan_spec *chan, char *buf)
>> +{
>> +     struct scmi_iio_priv *sensor = iio_priv(iio_dev);
>> +     int err;
>> +     u32 sensor_config;
>> +     struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
>> +     int len = 0;
>> +
>> +     err = iio_device_claim_direct_mode(iio_dev);
>> +     if (err) {
>> +             dev_err(&iio_dev->dev,
>> +                     "Error in claiming direct mode for sensor %s err %d",
>> +                     sensor->sensor_info->name, err);
> It's not an error, it just means the device is busy, so at most dev_info()
> or just rely on userspace correctly interpreting EBUSY.

Agree. Will use dev_info().
Thank you!

>> +             goto err_release;
>> +     }
>> +
>> +     sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
>> +                                SCMI_SENS_CFG_SENSOR_ENABLE);
>> +     err = sensor->sensor_ops->config_set(
>> +             sensor->ph, 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);
>> +             goto err_release;
>> +     }
>> +
>> +     err = sensor->sensor_ops->reading_get_timestamped(
>> +             sensor->ph, sensor->sensor_info->id,
>> +             sensor->sensor_info->num_axis, readings);
>> +     if (err) {
>> +             dev_err(&iio_dev->dev,
>> +                     "Error in reading raw attribute for sensor %s err %d",
>> +                     sensor->sensor_info->name, err);
>> +             goto err_release;
>> +     }
>> +
>> +     sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
>> +                                SCMI_SENS_CFG_SENSOR_DISABLE);
>> +     err = sensor->sensor_ops->config_set(
>> +             sensor->ph, sensor->sensor_info->id, sensor_config);
>> +     if (err) {
>> +             dev_err(&iio_dev->dev, "Error in disabling sensor %s err %d",
>> +                     sensor->sensor_info->name, err);
>> +             goto err_release;
>> +     }
>> +
>> +     len = scnprintf(buf, PAGE_SIZE, "%lld\n",
>> +                     readings[chan->scan_index].value);
>> +
>> +err_release:
>> +     iio_device_release_direct_mode(iio_dev);
>> +
>> +     return len;
>> +}
>> +
>>   static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
>>                                          uintptr_t private,
>>                                          const struct iio_chan_spec *chan,
>> @@ -355,6 +411,11 @@ static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev,
>>   }
>>
>>   static const struct iio_chan_spec_ext_info scmi_iio_ext_info[] = {
>> +     {
>> +             .name = "raw",
>> +             .read = scmi_iio_get_raw,
>> +             .shared = IIO_SEPARATE,
>> +     },
>>        {
>>                .name = "raw_available",
>>                .read = scmi_iio_get_raw_available,
>