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

[Vasyl Vavrychuk <vasyl.vavrychuk@xxxxxxxxxxxxxxx>]

Hi, Jyoti,

> In the code below, why is the logic of enabling and disabling the
> sensor in this function?  Generally the function to read the sensor
> value is just used for the code to read the sensor values ? and not
> enable/disable the sensor

But to read sensor value we have to enable it first. Other way to enable 
sensor we found is, for example:

  echo 1 > /sys/bus/iio/devices/.../scan_elements/in_anglvel_x_en

But, this command is related to IIO buffers use.

Other sensors drivers enable/disable sensor in read raw too, for 
example, drivers/iio/accel/kxcjk-1013.c has:

  case IIO_CHAN_INFO_RAW:
          mutex_lock(&data->mutex);
          if (iio_buffer_enabled(indio_dev))
                  ret = -EBUSY;
          else {
                  ret = kxcjk1013_set_power_state(data, true);
                  ... reading ...
                  ret = kxcjk1013_set_power_state(data, false);
          }
          mutex_unlock(&data->mutex);

But, after looking on this code I have some questions:

1. Should we block reading raw attribute and IIO buffer enabled, for for 
SCMI sensor it can coexist?
2. Should we wrap reading raw attribute implementation in iio_dev->mlock 
mutex?

> >     case IIO_CHAN_INFO_RAW:
> > +               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 enabling sensor %s err %d",
> > +                               sensor->sensor_info->name, err);
> > +                       return err;
> > +               }
> > +               /* Check if raw value fits 32 bits */
> > +               if (readings[ch->scan_index].value < INT_MIN ||
> > +                   readings[ch->scan_index].value > INT_MAX)
> > +                       return -ERANGE;
> > +               /* Use 32-bit value, since practically there is no need in 64 bits */
> > +               *val = (int)readings[ch->scan_index].value;
> >
> > +               return IIO_VAL_INT;