Add scmi_iio_get_raw() to read "raw" attribute.
Signed-off-by: Andriy Tryshnivskyy <[email protected]>
---
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
On Fri, 8 Oct 2021 21:28:26 +0300
Andriy Tryshnivskyy <[email protected]> wrote:
> Add scmi_iio_get_raw() to read "raw" attribute.
>
> Signed-off-by: Andriy Tryshnivskyy <[email protected]>
> ---
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,
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 <[email protected]> wrote:
>
>> Add scmi_iio_get_raw() to read "raw" attribute.
>>
>> Signed-off-by: Andriy Tryshnivskyy <[email protected]>
>> ---
> 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,
>