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

[Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>]

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.