Messages in this thread | | | Date | Wed, 20 Oct 2021 21:57:00 +0300 | Subject | Re: [PATCH v6 1/1] iio/scmi: Add reading "raw" attribute. | From | Andriy Tryshnivskyy <> |
| |
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 >
| |