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

[Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>]

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;