* [PATCH v6 0/1] iio/scmi: Add reading "raw" attribute.
@ 2021-10-19 7:59 Andriy Tryshnivskyy
2021-10-19 7:59 ` [PATCH v6 1/1] " Andriy Tryshnivskyy
0 siblings, 1 reply; 9+ messages in thread
From: Andriy Tryshnivskyy @ 2021-10-19 7:59 UTC (permalink / raw)
To: jbhayana, jic23
Cc: lars, linux-iio, linux-kernel, Vasyl.Vavrychuk, andriy.tryshnivskyy
iio/scmi: Add reading "raw" attribute.
Add IIO_CHAN_INFO_RAW to the mask and implement corresponding
reading "raw" attribute in scmi_iio_read_raw.
Introduce new type IIO_VAL_INT_64 to read 64-bit value
for "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 | 57 +++++++++++++++++++++-
drivers/iio/industrialio-core.c | 3 ++
include/linux/iio/types.h | 1 +
3 files changed, 60 insertions(+), 1 deletion(-)
base-commit: 7d2a07b769330c34b4deabeed939325c77a7ec2f
--
2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 1/1] iio/scmi: Add reading "raw" attribute.
2021-10-19 7:59 [PATCH v6 0/1] iio/scmi: Add reading "raw" attribute Andriy Tryshnivskyy
@ 2021-10-19 7:59 ` Andriy Tryshnivskyy
2021-10-20 17:51 ` Jonathan Cameron
[not found] ` <CAHp75VeZ5=T8N50PoZQVsWjf5ZWjZ4kwHAxUp=pyuK6ioUrQog@mail.gmail.com>
0 siblings, 2 replies; 9+ messages in thread
From: Andriy Tryshnivskyy @ 2021-10-19 7:59 UTC (permalink / raw)
To: jbhayana, jic23
Cc: lars, linux-iio, linux-kernel, Vasyl.Vavrychuk, andriy.tryshnivskyy
Add IIO_CHAN_INFO_RAW to the mask and implement corresponding
reading "raw" attribute in scmi_iio_read_raw.
Introduce new type IIO_VAL_INT_64 to read 64-bit value
for "raw" attribute.
Changes comparing v5 -> v6:
* revert v5 changes since with scmi_iio_read_raw() the channel
can't be used by in kernel users (iio-hwmon)
* returned to v3 with direct mode
* introduce new type IIO_VAL_INT_64 to read 64-bit value
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 32-bit
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
Changes comparing v2 -> v3:
* adaptation for changes in structure scmi_iio_priv (no member
named 'handle')
Changes comparing v0 -> v2:
* added an error return when the error happened during config_set
* removed redundant cast for "readings"
* added check if raw value fits 32 bits
Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
---
drivers/iio/common/scmi_sensors/scmi_iio.c | 57 +++++++++++++++++++++-
drivers/iio/industrialio-core.c | 3 ++
include/linux/iio/types.h | 1 +
3 files changed, 60 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..2c1aec0fd5ff 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -279,6 +279,52 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2)
return 0;
}
+static int scmi_iio_read_channel_data(struct iio_dev *iio_dev,
+ struct iio_chan_spec const *ch, int *val, int *val2)
+{
+ struct scmi_iio_priv *sensor = iio_priv(iio_dev);
+ u32 sensor_config;
+ struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
+ int err;
+
+ 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);
+ return err;
+ }
+
+ 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);
+ return err;
+ }
+
+ 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);
+ return err;
+ }
+
+ *val = (u32)readings[ch->scan_index].value;
+ *val2 = (u32)(readings[ch->scan_index].value >> 32);
+
+ return IIO_VAL_INT_64;
+}
+
static int scmi_iio_read_raw(struct iio_dev *iio_dev,
struct iio_chan_spec const *ch, int *val,
int *val2, long mask)
@@ -300,6 +346,14 @@ 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:
+ ret = iio_device_claim_direct_mode(iio_dev);
+ if (ret)
+ return ret;
+
+ ret = scmi_iio_read_channel_data(iio_dev, ch, val, val2);
+ iio_device_release_direct_mode(iio_dev);
+ return ret;
default:
return -EINVAL;
}
@@ -381,7 +435,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);
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 6d2175eb7af2..49e42d04ea16 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -702,6 +702,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
}
case IIO_VAL_CHAR:
return sysfs_emit_at(buf, offset, "%c", (char)vals[0]);
+ case IIO_VAL_INT_64:
+ tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
+ return sysfs_emit_at(buf, offset, "%lld", tmp2);
default:
return 0;
}
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 84b3f8175cc6..e148fe11a3dc 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -24,6 +24,7 @@ enum iio_event_info {
#define IIO_VAL_INT_PLUS_NANO 3
#define IIO_VAL_INT_PLUS_MICRO_DB 4
#define IIO_VAL_INT_MULTIPLE 5
+#define IIO_VAL_INT_64 6
#define IIO_VAL_FRACTIONAL 10
#define IIO_VAL_FRACTIONAL_LOG2 11
#define IIO_VAL_CHAR 12
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/1] iio/scmi: Add reading "raw" attribute.
2021-10-19 7:59 ` [PATCH v6 1/1] " Andriy Tryshnivskyy
@ 2021-10-20 17:51 ` Jonathan Cameron
2021-10-20 18:57 ` Andriy Tryshnivskyy
[not found] ` <CAHp75VeZ5=T8N50PoZQVsWjf5ZWjZ4kwHAxUp=pyuK6ioUrQog@mail.gmail.com>
1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2021-10-20 17:51 UTC (permalink / raw)
To: Andriy Tryshnivskyy
Cc: jbhayana, lars, linux-iio, linux-kernel, Vasyl.Vavrychuk
On Tue, 19 Oct 2021 10:59:49 +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.
> Introduce new type IIO_VAL_INT_64 to read 64-bit value
> for "raw" attribute.
>
Change log needs to be below the --- otherwise we'll store it forever
in git. A linked tag (which will be generated when I apply)
is sufficient for this sort of historical info.
> Changes comparing v5 -> v6:
> * revert v5 changes since with scmi_iio_read_raw() the channel
> can't be used by in kernel users (iio-hwmon)
> * returned to v3 with direct mode
> * introduce new type IIO_VAL_INT_64 to read 64-bit value
>
> 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 32-bit
> 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
>
> Changes comparing v2 -> v3:
> * adaptation for changes in structure scmi_iio_priv (no member
> named 'handle')
>
> Changes comparing v0 -> v2:
> * added an error return when the error happened during config_set
> * removed redundant cast for "readings"
> * added check if raw value fits 32 bits
>
> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
> ---
> drivers/iio/common/scmi_sensors/scmi_iio.c | 57 +++++++++++++++++++++-
> drivers/iio/industrialio-core.c | 3 ++
> include/linux/iio/types.h | 1 +
Two patches needed. One to introduce the new core functionality then
a second to use it in the driver.
Actual code looks good to me though I think I'd like a comment next to
the #define as not obvious which way around the two parts will go.
There are some other places we will probably need to ultimately handle this
to allow for in kernel consumers but those can come when someone needs them.
Will need an ack from Jyoti on this one though as driver author.
Thanks,
Jonathan
> 3 files changed, 60 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..2c1aec0fd5ff 100644
> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -279,6 +279,52 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2)
> return 0;
> }
>
> +static int scmi_iio_read_channel_data(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *ch, int *val, int *val2)
> +{
> + struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> + u32 sensor_config;
> + struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
> + int err;
> +
> + 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);
> + return err;
> + }
> +
> + 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);
> + return err;
> + }
> +
> + 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);
> + return err;
> + }
> +
> + *val = (u32)readings[ch->scan_index].value;
> + *val2 = (u32)(readings[ch->scan_index].value >> 32)
> +
> + return IIO_VAL_INT_64;
> +}
> +
> static int scmi_iio_read_raw(struct iio_dev *iio_dev,
> struct iio_chan_spec const *ch, int *val,
> int *val2, long mask)
> @@ -300,6 +346,14 @@ 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:
> + ret = iio_device_claim_direct_mode(iio_dev);
> + if (ret)
> + return ret;
> +
> + ret = scmi_iio_read_channel_data(iio_dev, ch, val, val2);
> + iio_device_release_direct_mode(iio_dev);
> + return ret;
> default:
> return -EINVAL;
> }
> @@ -381,7 +435,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);
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 6d2175eb7af2..49e42d04ea16 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -702,6 +702,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
> }
> case IIO_VAL_CHAR:
> return sysfs_emit_at(buf, offset, "%c", (char)vals[0]);
> + case IIO_VAL_INT_64:
> + tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
> + return sysfs_emit_at(buf, offset, "%lld", tmp2);
> default:
> return 0;
> }
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 84b3f8175cc6..e148fe11a3dc 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -24,6 +24,7 @@ enum iio_event_info {
> #define IIO_VAL_INT_PLUS_NANO 3
> #define IIO_VAL_INT_PLUS_MICRO_DB 4
> #define IIO_VAL_INT_MULTIPLE 5
> +#define IIO_VAL_INT_64 6
Possibly worth a descriptive comment. The other
types tend to make it easy to assume the role
of val and that of val2, in this case, val being
the lower 32 bits isn't obvious...
> #define IIO_VAL_FRACTIONAL 10
> #define IIO_VAL_FRACTIONAL_LOG2 11
> #define IIO_VAL_CHAR 12
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/1] iio/scmi: Add reading "raw" attribute.
2021-10-20 17:51 ` Jonathan Cameron
@ 2021-10-20 18:57 ` Andriy Tryshnivskyy
2021-10-21 0:40 ` Jyoti Bhayana
2021-10-21 9:36 ` Jonathan Cameron
0 siblings, 2 replies; 9+ messages in thread
From: Andriy Tryshnivskyy @ 2021-10-20 18:57 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: jbhayana, lars, linux-iio, linux-kernel, Vasyl.Vavrychuk
On 20.10.21 20: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 Tue, 19 Oct 2021 10:59:49 +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.
>> Introduce new type IIO_VAL_INT_64 to read 64-bit value
>> for "raw" attribute.
>>
> Change log needs to be below the --- otherwise we'll store it forever
> in git. A linked tag (which will be generated when I apply)
> is sufficient for this sort of historical info.
>
Sorry, this is my first patch, I was not aware of that.
Thanks for the explanation.
Quick question: since next version will include 2 patches,
I guess a change log should be moved back to the cover letter, right?
>> Changes comparing v5 -> v6:
>> * revert v5 changes since with scmi_iio_read_raw() the channel
>> can't be used by in kernel users (iio-hwmon)
>> * returned to v3 with direct mode
>> * introduce new type IIO_VAL_INT_64 to read 64-bit value
>>
>> 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 32-bit
>> 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
>>
>> Changes comparing v2 -> v3:
>> * adaptation for changes in structure scmi_iio_priv (no member
>> named 'handle')
>>
>> Changes comparing v0 -> v2:
>> * added an error return when the error happened during config_set
>> * removed redundant cast for "readings"
>> * added check if raw value fits 32 bits
>>
>> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
>> ---
>> drivers/iio/common/scmi_sensors/scmi_iio.c | 57 +++++++++++++++++++++-
>> drivers/iio/industrialio-core.c | 3 ++
>> include/linux/iio/types.h | 1 +
> Two patches needed. One to introduce the new core functionality then
> a second to use it in the driver.
>
> Actual code looks good to me though I think I'd like a comment next to
> the #define as not obvious which way around the two parts will go.
>
> There are some other places we will probably need to ultimately handle this
> to allow for in kernel consumers but those can come when someone needs them.
>
> Will need an ack from Jyoti on this one though as driver author.
> Thanks,
>
> Jonathan
Sure, will split the current patch into two patches.
>> 3 files changed, 60 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..2c1aec0fd5ff 100644
>> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
>> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
>> @@ -279,6 +279,52 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2)
>> return 0;
>> }
>>
>> +static int scmi_iio_read_channel_data(struct iio_dev *iio_dev,
>> + struct iio_chan_spec const *ch, int *val, int *val2)
>> +{
>> + struct scmi_iio_priv *sensor = iio_priv(iio_dev);
>> + u32 sensor_config;
>> + struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
>> + int err;
>> +
>> + 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);
>> + return err;
>> + }
>> +
>> + 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);
>> + return err;
>> + }
>> +
>> + 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);
>> + return err;
>> + }
>> +
>> + *val = (u32)readings[ch->scan_index].value;
>> + *val2 = (u32)(readings[ch->scan_index].value >> 32)
>> +
>> + return IIO_VAL_INT_64;
>> +}
>> +
>> static int scmi_iio_read_raw(struct iio_dev *iio_dev,
>> struct iio_chan_spec const *ch, int *val,
>> int *val2, long mask)
>> @@ -300,6 +346,14 @@ 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:
>> + ret = iio_device_claim_direct_mode(iio_dev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = scmi_iio_read_channel_data(iio_dev, ch, val, val2);
>> + iio_device_release_direct_mode(iio_dev);
>> + return ret;
>> default:
>> return -EINVAL;
>> }
>> @@ -381,7 +435,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);
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index 6d2175eb7af2..49e42d04ea16 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -702,6 +702,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
>> }
>> case IIO_VAL_CHAR:
>> return sysfs_emit_at(buf, offset, "%c", (char)vals[0]);
>> + case IIO_VAL_INT_64:
>> + tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
>> + return sysfs_emit_at(buf, offset, "%lld", tmp2);
>> default:
>> return 0;
>> }
>> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
>> index 84b3f8175cc6..e148fe11a3dc 100644
>> --- a/include/linux/iio/types.h
>> +++ b/include/linux/iio/types.h
>> @@ -24,6 +24,7 @@ enum iio_event_info {
>> #define IIO_VAL_INT_PLUS_NANO 3
>> #define IIO_VAL_INT_PLUS_MICRO_DB 4
>> #define IIO_VAL_INT_MULTIPLE 5
>> +#define IIO_VAL_INT_64 6
> Possibly worth a descriptive comment. The other
> types tend to make it easy to assume the role
> of val and that of val2, in this case, val being
> the lower 32 bits isn't obvious...
I will add a comment here.
Thank you for your review!
Best regard,
Andriy.
>> #define IIO_VAL_FRACTIONAL 10
>> #define IIO_VAL_FRACTIONAL_LOG2 11
>> #define IIO_VAL_CHAR 12
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/1] iio/scmi: Add reading "raw" attribute.
2021-10-20 18:57 ` Andriy Tryshnivskyy
@ 2021-10-21 0:40 ` Jyoti Bhayana
2021-10-24 7:35 ` Andriy Tryshnivskyy
2021-10-21 9:36 ` Jonathan Cameron
1 sibling, 1 reply; 9+ messages in thread
From: Jyoti Bhayana @ 2021-10-21 0:40 UTC (permalink / raw)
To: Andriy Tryshnivskyy
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel,
Vasyl Vavrychuk
Hi Andriy,
Technically, the changes look good to me.
Thanks,
Jyoti
On Wed, Oct 20, 2021 at 11:57 AM Andriy Tryshnivskyy
<andriy.tryshnivskyy@opensynergy.com> wrote:
>
>
> On 20.10.21 20: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 Tue, 19 Oct 2021 10:59:49 +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.
> >> Introduce new type IIO_VAL_INT_64 to read 64-bit value
> >> for "raw" attribute.
> >>
> > Change log needs to be below the --- otherwise we'll store it forever
> > in git. A linked tag (which will be generated when I apply)
> > is sufficient for this sort of historical info.
> >
> Sorry, this is my first patch, I was not aware of that.
> Thanks for the explanation.
> Quick question: since next version will include 2 patches,
> I guess a change log should be moved back to the cover letter, right?
>
>
> >> Changes comparing v5 -> v6:
> >> * revert v5 changes since with scmi_iio_read_raw() the channel
> >> can't be used by in kernel users (iio-hwmon)
> >> * returned to v3 with direct mode
> >> * introduce new type IIO_VAL_INT_64 to read 64-bit value
> >>
> >> 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 32-bit
> >> 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
> >>
> >> Changes comparing v2 -> v3:
> >> * adaptation for changes in structure scmi_iio_priv (no member
> >> named 'handle')
> >>
> >> Changes comparing v0 -> v2:
> >> * added an error return when the error happened during config_set
> >> * removed redundant cast for "readings"
> >> * added check if raw value fits 32 bits
> >>
> >> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
> >> ---
> >> drivers/iio/common/scmi_sensors/scmi_iio.c | 57 +++++++++++++++++++++-
> >> drivers/iio/industrialio-core.c | 3 ++
> >> include/linux/iio/types.h | 1 +
> > Two patches needed. One to introduce the new core functionality then
> > a second to use it in the driver.
> >
> > Actual code looks good to me though I think I'd like a comment next to
> > the #define as not obvious which way around the two parts will go.
> >
> > There are some other places we will probably need to ultimately handle this
> > to allow for in kernel consumers but those can come when someone needs them.
> >
> > Will need an ack from Jyoti on this one though as driver author.
> > Thanks,
> >
> > Jonathan
>
> Sure, will split the current patch into two patches.
>
>
> >> 3 files changed, 60 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..2c1aec0fd5ff 100644
> >> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> >> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> >> @@ -279,6 +279,52 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2)
> >> return 0;
> >> }
> >>
> >> +static int scmi_iio_read_channel_data(struct iio_dev *iio_dev,
> >> + struct iio_chan_spec const *ch, int *val, int *val2)
> >> +{
> >> + struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> >> + u32 sensor_config;
> >> + struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
> >> + int err;
> >> +
> >> + 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);
> >> + return err;
> >> + }
> >> +
> >> + 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);
> >> + return err;
> >> + }
> >> +
> >> + 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);
> >> + return err;
> >> + }
> >> +
> >> + *val = (u32)readings[ch->scan_index].value;
> >> + *val2 = (u32)(readings[ch->scan_index].value >> 32)
> >> +
> >> + return IIO_VAL_INT_64;
> >> +}
> >> +
> >> static int scmi_iio_read_raw(struct iio_dev *iio_dev,
> >> struct iio_chan_spec const *ch, int *val,
> >> int *val2, long mask)
> >> @@ -300,6 +346,14 @@ 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:
> >> + ret = iio_device_claim_direct_mode(iio_dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = scmi_iio_read_channel_data(iio_dev, ch, val, val2);
> >> + iio_device_release_direct_mode(iio_dev);
> >> + return ret;
> >> default:
> >> return -EINVAL;
> >> }
> >> @@ -381,7 +435,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);
> >> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> >> index 6d2175eb7af2..49e42d04ea16 100644
> >> --- a/drivers/iio/industrialio-core.c
> >> +++ b/drivers/iio/industrialio-core.c
> >> @@ -702,6 +702,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
> >> }
> >> case IIO_VAL_CHAR:
> >> return sysfs_emit_at(buf, offset, "%c", (char)vals[0]);
> >> + case IIO_VAL_INT_64:
> >> + tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
> >> + return sysfs_emit_at(buf, offset, "%lld", tmp2);
> >> default:
> >> return 0;
> >> }
> >> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> >> index 84b3f8175cc6..e148fe11a3dc 100644
> >> --- a/include/linux/iio/types.h
> >> +++ b/include/linux/iio/types.h
> >> @@ -24,6 +24,7 @@ enum iio_event_info {
> >> #define IIO_VAL_INT_PLUS_NANO 3
> >> #define IIO_VAL_INT_PLUS_MICRO_DB 4
> >> #define IIO_VAL_INT_MULTIPLE 5
> >> +#define IIO_VAL_INT_64 6
> > Possibly worth a descriptive comment. The other
> > types tend to make it easy to assume the role
> > of val and that of val2, in this case, val being
> > the lower 32 bits isn't obvious...
>
> I will add a comment here.
>
> Thank you for your review!
>
> Best regard,
> Andriy.
>
>
> >> #define IIO_VAL_FRACTIONAL 10
> >> #define IIO_VAL_FRACTIONAL_LOG2 11
> >> #define IIO_VAL_CHAR 12
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/1] iio/scmi: Add reading "raw" attribute.
2021-10-20 18:57 ` Andriy Tryshnivskyy
2021-10-21 0:40 ` Jyoti Bhayana
@ 2021-10-21 9:36 ` Jonathan Cameron
2021-10-24 7:23 ` Andriy Tryshnivskyy
1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2021-10-21 9:36 UTC (permalink / raw)
To: Andriy Tryshnivskyy
Cc: jbhayana, lars, linux-iio, linux-kernel, Vasyl.Vavrychuk
On Wed, 20 Oct 2021 21:57:00 +0300
Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com> wrote:
> On 20.10.21 20: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 Tue, 19 Oct 2021 10:59:49 +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.
> >> Introduce new type IIO_VAL_INT_64 to read 64-bit value
> >> for "raw" attribute.
> >>
> > Change log needs to be below the --- otherwise we'll store it forever
> > in git. A linked tag (which will be generated when I apply)
> > is sufficient for this sort of historical info.
> >
> Sorry, this is my first patch, I was not aware of that.
> Thanks for the explanation.
> Quick question: since next version will include 2 patches,
> I guess a change log should be moved back to the cover letter, right?
It's a trade off for which there are no firm rules.
Sometimes changes are well isolated in individual patches, in which case
the best bet is to put the change logs in each patch, sometimes they are
more global things that affect the whole series in which case the change
log is best in the cover letter.
However, for a given series pick one or other style (don't mix!) as
otherwise it would get really confusing. Mostly no one really minds
where the log is as long as we can find it easily.
Jonathan
>
>
> >> Changes comparing v5 -> v6:
> >> * revert v5 changes since with scmi_iio_read_raw() the channel
> >> can't be used by in kernel users (iio-hwmon)
> >> * returned to v3 with direct mode
> >> * introduce new type IIO_VAL_INT_64 to read 64-bit value
> >>
> >> 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 32-bit
> >> 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
> >>
> >> Changes comparing v2 -> v3:
> >> * adaptation for changes in structure scmi_iio_priv (no member
> >> named 'handle')
> >>
> >> Changes comparing v0 -> v2:
> >> * added an error return when the error happened during config_set
> >> * removed redundant cast for "readings"
> >> * added check if raw value fits 32 bits
> >>
> >> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
> >> ---
> >> drivers/iio/common/scmi_sensors/scmi_iio.c | 57 +++++++++++++++++++++-
> >> drivers/iio/industrialio-core.c | 3 ++
> >> include/linux/iio/types.h | 1 +
> > Two patches needed. One to introduce the new core functionality then
> > a second to use it in the driver.
> >
> > Actual code looks good to me though I think I'd like a comment next to
> > the #define as not obvious which way around the two parts will go.
> >
> > There are some other places we will probably need to ultimately handle this
> > to allow for in kernel consumers but those can come when someone needs them.
> >
> > Will need an ack from Jyoti on this one though as driver author.
> > Thanks,
> >
> > Jonathan
>
> Sure, will split the current patch into two patches.
>
>
> >> 3 files changed, 60 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..2c1aec0fd5ff 100644
> >> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> >> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> >> @@ -279,6 +279,52 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2)
> >> return 0;
> >> }
> >>
> >> +static int scmi_iio_read_channel_data(struct iio_dev *iio_dev,
> >> + struct iio_chan_spec const *ch, int *val, int *val2)
> >> +{
> >> + struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> >> + u32 sensor_config;
> >> + struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
> >> + int err;
> >> +
> >> + 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);
> >> + return err;
> >> + }
> >> +
> >> + 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);
> >> + return err;
> >> + }
> >> +
> >> + 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);
> >> + return err;
> >> + }
> >> +
> >> + *val = (u32)readings[ch->scan_index].value;
> >> + *val2 = (u32)(readings[ch->scan_index].value >> 32)
> >> +
> >> + return IIO_VAL_INT_64;
> >> +}
> >> +
> >> static int scmi_iio_read_raw(struct iio_dev *iio_dev,
> >> struct iio_chan_spec const *ch, int *val,
> >> int *val2, long mask)
> >> @@ -300,6 +346,14 @@ 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:
> >> + ret = iio_device_claim_direct_mode(iio_dev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = scmi_iio_read_channel_data(iio_dev, ch, val, val2);
> >> + iio_device_release_direct_mode(iio_dev);
> >> + return ret;
> >> default:
> >> return -EINVAL;
> >> }
> >> @@ -381,7 +435,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);
> >> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> >> index 6d2175eb7af2..49e42d04ea16 100644
> >> --- a/drivers/iio/industrialio-core.c
> >> +++ b/drivers/iio/industrialio-core.c
> >> @@ -702,6 +702,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
> >> }
> >> case IIO_VAL_CHAR:
> >> return sysfs_emit_at(buf, offset, "%c", (char)vals[0]);
> >> + case IIO_VAL_INT_64:
> >> + tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
> >> + return sysfs_emit_at(buf, offset, "%lld", tmp2);
> >> default:
> >> return 0;
> >> }
> >> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> >> index 84b3f8175cc6..e148fe11a3dc 100644
> >> --- a/include/linux/iio/types.h
> >> +++ b/include/linux/iio/types.h
> >> @@ -24,6 +24,7 @@ enum iio_event_info {
> >> #define IIO_VAL_INT_PLUS_NANO 3
> >> #define IIO_VAL_INT_PLUS_MICRO_DB 4
> >> #define IIO_VAL_INT_MULTIPLE 5
> >> +#define IIO_VAL_INT_64 6
> > Possibly worth a descriptive comment. The other
> > types tend to make it easy to assume the role
> > of val and that of val2, in this case, val being
> > the lower 32 bits isn't obvious...
>
> I will add a comment here.
>
> Thank you for your review!
>
> Best regard,
> Andriy.
>
>
> >> #define IIO_VAL_FRACTIONAL 10
> >> #define IIO_VAL_FRACTIONAL_LOG2 11
> >> #define IIO_VAL_CHAR 12
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/1] iio/scmi: Add reading "raw" attribute.
2021-10-21 9:36 ` Jonathan Cameron
@ 2021-10-24 7:23 ` Andriy Tryshnivskyy
0 siblings, 0 replies; 9+ messages in thread
From: Andriy Tryshnivskyy @ 2021-10-24 7:23 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: jbhayana, lars, linux-iio, linux-kernel, Vasyl.Vavrychuk
>>> On Tue, 19 Oct 2021 10:59:49 +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.
>>>> Introduce new type IIO_VAL_INT_64 to read 64-bit value
>>>> for "raw" attribute.
>>>>
>>> Change log needs to be below the --- otherwise we'll store it forever
>>> in git. A linked tag (which will be generated when I apply)
>>> is sufficient for this sort of historical info.
>>>
>> Sorry, this is my first patch, I was not aware of that.
>> Thanks for the explanation.
>> Quick question: since next version will include 2 patches,
>> I guess a change log should be moved back to the cover letter, right?
> It's a trade off for which there are no firm rules.
> Sometimes changes are well isolated in individual patches, in which case
> the best bet is to put the change logs in each patch, sometimes they are
> more global things that affect the whole series in which case the change
> log is best in the cover letter.
>
> However, for a given series pick one or other style (don't mix!) as
> otherwise it would get really confusing. Mostly no one really minds
> where the log is as long as we can find it easily.
>
> Jonathan
thank you for the clarification!
best regards,
Andriy.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 1/1] iio/scmi: Add reading "raw" attribute.
2021-10-21 0:40 ` Jyoti Bhayana
@ 2021-10-24 7:35 ` Andriy Tryshnivskyy
0 siblings, 0 replies; 9+ messages in thread
From: Andriy Tryshnivskyy @ 2021-10-24 7:35 UTC (permalink / raw)
To: Jyoti Bhayana
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, linux-kernel,
Vasyl Vavrychuk
Hi Jyoti,
Thank you for your review.
As was discussed with Jonathan, I will prepare new version with 2 patches and additional comment.
Thanks,
Andriy.
On 21.10.21 03:40, Jyoti Bhayana 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.
>
>
> Hi Andriy,
>
> Technically, the changes look good to me.
>
> Thanks,
> Jyoti
>
> On Wed, Oct 20, 2021 at 11:57 AM Andriy Tryshnivskyy
> <andriy.tryshnivskyy@opensynergy.com> wrote:
>>
>> On 20.10.21 20: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 Tue, 19 Oct 2021 10:59:49 +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.
>>>> Introduce new type IIO_VAL_INT_64 to read 64-bit value
>>>> for "raw" attribute.
>>>>
>>> Change log needs to be below the --- otherwise we'll store it forever
>>> in git. A linked tag (which will be generated when I apply)
>>> is sufficient for this sort of historical info.
>>>
>> Sorry, this is my first patch, I was not aware of that.
>> Thanks for the explanation.
>> Quick question: since next version will include 2 patches,
>> I guess a change log should be moved back to the cover letter, right?
>>
>>
>>>> Changes comparing v5 -> v6:
>>>> * revert v5 changes since with scmi_iio_read_raw() the channel
>>>> can't be used by in kernel users (iio-hwmon)
>>>> * returned to v3 with direct mode
>>>> * introduce new type IIO_VAL_INT_64 to read 64-bit value
>>>>
>>>> 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 32-bit
>>>> 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
>>>>
>>>> Changes comparing v2 -> v3:
>>>> * adaptation for changes in structure scmi_iio_priv (no member
>>>> named 'handle')
>>>>
>>>> Changes comparing v0 -> v2:
>>>> * added an error return when the error happened during config_set
>>>> * removed redundant cast for "readings"
>>>> * added check if raw value fits 32 bits
>>>>
>>>> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>
>>>> ---
>>>> drivers/iio/common/scmi_sensors/scmi_iio.c | 57 +++++++++++++++++++++-
>>>> drivers/iio/industrialio-core.c | 3 ++
>>>> include/linux/iio/types.h | 1 +
>>> Two patches needed. One to introduce the new core functionality then
>>> a second to use it in the driver.
>>>
>>> Actual code looks good to me though I think I'd like a comment next to
>>> the #define as not obvious which way around the two parts will go.
>>>
>>> There are some other places we will probably need to ultimately handle this
>>> to allow for in kernel consumers but those can come when someone needs them.
>>>
>>> Will need an ack from Jyoti on this one though as driver author.
>>> Thanks,
>>>
>>> Jonathan
>> Sure, will split the current patch into two patches.
>>
>>
>>>> 3 files changed, 60 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..2c1aec0fd5ff 100644
>>>> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
>>>> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
>>>> @@ -279,6 +279,52 @@ static int scmi_iio_get_odr_val(struct iio_dev *iio_dev, int *val, int *val2)
>>>> return 0;
>>>> }
>>>>
>>>> +static int scmi_iio_read_channel_data(struct iio_dev *iio_dev,
>>>> + struct iio_chan_spec const *ch, int *val, int *val2)
>>>> +{
>>>> + struct scmi_iio_priv *sensor = iio_priv(iio_dev);
>>>> + u32 sensor_config;
>>>> + struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
>>>> + int err;
>>>> +
>>>> + 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);
>>>> + return err;
>>>> + }
>>>> +
>>>> + 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);
>>>> + return err;
>>>> + }
>>>> +
>>>> + 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);
>>>> + return err;
>>>> + }
>>>> +
>>>> + *val = (u32)readings[ch->scan_index].value;
>>>> + *val2 = (u32)(readings[ch->scan_index].value >> 32)
>>>> +
>>>> + return IIO_VAL_INT_64;
>>>> +}
>>>> +
>>>> static int scmi_iio_read_raw(struct iio_dev *iio_dev,
>>>> struct iio_chan_spec const *ch, int *val,
>>>> int *val2, long mask)
>>>> @@ -300,6 +346,14 @@ 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:
>>>> + ret = iio_device_claim_direct_mode(iio_dev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = scmi_iio_read_channel_data(iio_dev, ch, val, val2);
>>>> + iio_device_release_direct_mode(iio_dev);
>>>> + return ret;
>>>> default:
>>>> return -EINVAL;
>>>> }
>>>> @@ -381,7 +435,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);
>>>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>>>> index 6d2175eb7af2..49e42d04ea16 100644
>>>> --- a/drivers/iio/industrialio-core.c
>>>> +++ b/drivers/iio/industrialio-core.c
>>>> @@ -702,6 +702,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
>>>> }
>>>> case IIO_VAL_CHAR:
>>>> return sysfs_emit_at(buf, offset, "%c", (char)vals[0]);
>>>> + case IIO_VAL_INT_64:
>>>> + tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
>>>> + return sysfs_emit_at(buf, offset, "%lld", tmp2);
>>>> default:
>>>> return 0;
>>>> }
>>>> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
>>>> index 84b3f8175cc6..e148fe11a3dc 100644
>>>> --- a/include/linux/iio/types.h
>>>> +++ b/include/linux/iio/types.h
>>>> @@ -24,6 +24,7 @@ enum iio_event_info {
>>>> #define IIO_VAL_INT_PLUS_NANO 3
>>>> #define IIO_VAL_INT_PLUS_MICRO_DB 4
>>>> #define IIO_VAL_INT_MULTIPLE 5
>>>> +#define IIO_VAL_INT_64 6
>>> Possibly worth a descriptive comment. The other
>>> types tend to make it easy to assume the role
>>> of val and that of val2, in this case, val being
>>> the lower 32 bits isn't obvious...
>> I will add a comment here.
>>
>> Thank you for your review!
>>
>> Best regard,
>> Andriy.
>>
>>
>>>> #define IIO_VAL_FRACTIONAL 10
>>>> #define IIO_VAL_FRACTIONAL_LOG2 11
>>>> #define IIO_VAL_CHAR 12
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v6 0/1] iio/scmi: Add reading "raw" attribute.
[not found] ` <CAHp75VeZ5=T8N50PoZQVsWjf5ZWjZ4kwHAxUp=pyuK6ioUrQog@mail.gmail.com>
@ 2021-10-24 8:49 ` Andriy Tryshnivskyy
0 siblings, 0 replies; 9+ messages in thread
From: Andriy Tryshnivskyy @ 2021-10-24 8:49 UTC (permalink / raw)
To: Andy Shevchenko
Cc: jbhayana, jic23, lars, linux-iio, linux-kernel, Vasyl.Vavrychuk
On 24.10.21 11:33, Andy Shevchenko 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 Tuesday, October 19, 2021, 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.
> Introduce new type IIO_VAL_INT_64 to read 64-bit value
> for "raw" attribute.
>
> Changes comparing v5 -> v6:
> * revert v5 changes since with scmi_iio_read_raw() the channel
> can't be used by in kernel users (iio-hwmon)
> * returned to v3 with direct mode
> * introduce new type IIO_VAL_INT_64 to read 64-bit value
>
> 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
> 32-bit
> 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
>
> Changes comparing v2 -> v3:
> * adaptation for changes in structure scmi_iio_priv (no member
> named 'handle')
>
> Changes comparing v0 -> v2:
> * added an error return when the error happened during config_set
> * removed redundant cast for "readings"
> * added check if raw value fits 32 bits
>
> Signed-off-by: Andriy Tryshnivskyy
> <andriy.tryshnivskyy@opensynergy.com
> <mailto:andriy.tryshnivskyy@opensynergy.com>>
> ---
> drivers/iio/common/scmi_sensors/scmi_iio.c | 57
> +++++++++++++++++++++-
> drivers/iio/industrialio-core.c | 3 ++
> include/linux/iio/types.h | 1 +
>
>
> Usually we separate changes for core and for individual drivers for
> the sake of bisecting and, if requested, reverting.
Noted. Thanks!
> 3 files changed, 60 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..2c1aec0fd5ff 100644
> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c
> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
> @@ -279,6 +279,52 @@ static int scmi_iio_get_odr_val(struct
> iio_dev *iio_dev, int *val, int *val2)
> return 0;
> }
>
> +static int scmi_iio_read_channel_data(struct iio_dev *iio_dev,
> + struct iio_chan_spec const *ch, int
> *val, int *val2)
> +{
> + struct scmi_iio_priv *sensor = iio_priv(iio_dev);
> + u32 sensor_config;
> + struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS];
> + int err;
> +
> + 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);
> + return err;
> + }
> +
> + 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);
> + return err;
> + }
> +
> + 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);
> + return err;
> + }
>
>
> + *val = (u32)readings[ch->scan_index].value;
> + *val2 = (u32)(readings[ch->scan_index].value >> 32);
>
>
>
> We have upper_32_bits() and similar for low part.
I will use upper_32_bits() and lower_32_bits() then.
Thank you for review!
>
> +
> + return IIO_VAL_INT_64;
> +}
> +
> static int scmi_iio_read_raw(struct iio_dev *iio_dev,
> struct iio_chan_spec const *ch, int *val,
> int *val2, long mask)
> @@ -300,6 +346,14 @@ 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:
> + ret = iio_device_claim_direct_mode(iio_dev);
> + if (ret)
> + return ret;
> +
> + ret = scmi_iio_read_channel_data(iio_dev, ch, val,
> val2);
> + iio_device_release_direct_mode(iio_dev);
> + return ret;
> default:
> return -EINVAL;
> }
> @@ -381,7 +435,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);
> diff --git a/drivers/iio/industrialio-core.c
> b/drivers/iio/industrialio-core.c
> index 6d2175eb7af2..49e42d04ea16 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -702,6 +702,9 @@ static ssize_t __iio_format_value(char *buf,
> size_t offset, unsigned int type,
> }
> case IIO_VAL_CHAR:
> return sysfs_emit_at(buf, offset, "%c",
> (char)vals[0]);
> + case IIO_VAL_INT_64:
> + tmp2 = (s64)((((u64)vals[1]) << 32) | (u32)vals[0]);
> + return sysfs_emit_at(buf, offset, "%lld", tmp2);
> default:
> return 0;
> }
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 84b3f8175cc6..e148fe11a3dc 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -24,6 +24,7 @@ enum iio_event_info {
> #define IIO_VAL_INT_PLUS_NANO 3
> #define IIO_VAL_INT_PLUS_MICRO_DB 4
> #define IIO_VAL_INT_MULTIPLE 5
> +#define IIO_VAL_INT_64 6
> #define IIO_VAL_FRACTIONAL 10
> #define IIO_VAL_FRACTIONAL_LOG2 11
> #define IIO_VAL_CHAR 12
> --
> 2.17.1
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks,
Andriy.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-10-24 8:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 7:59 [PATCH v6 0/1] iio/scmi: Add reading "raw" attribute Andriy Tryshnivskyy
2021-10-19 7:59 ` [PATCH v6 1/1] " Andriy Tryshnivskyy
2021-10-20 17:51 ` Jonathan Cameron
2021-10-20 18:57 ` Andriy Tryshnivskyy
2021-10-21 0:40 ` Jyoti Bhayana
2021-10-24 7:35 ` Andriy Tryshnivskyy
2021-10-21 9:36 ` Jonathan Cameron
2021-10-24 7:23 ` Andriy Tryshnivskyy
[not found] ` <CAHp75VeZ5=T8N50PoZQVsWjf5ZWjZ4kwHAxUp=pyuK6ioUrQog@mail.gmail.com>
2021-10-24 8:49 ` [PATCH v6 0/1] " Andriy Tryshnivskyy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).