Hi Vasyl, Regarding below question, yes reading raw attribute should be blocked if buffer is enabled for that sensor. > 1. Should we block reading raw attribute and IIO buffer enabled, for for > SCMI sensor it can coexist? PLease see https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c#L667 as well. It has case IIO_CHAN_INFO_RAW: ret = iio_device_claim_direct_mode(indio_dev); if (ret) return ret; mutex_lock(&st->lock); ret = inv_mpu6050_read_channel_data(indio_dev, chan, val); mutex_unlock(&st->lock); iio_device_release_direct_mode(indio_dev); return ret; Regarding the question below, the answer is yes. > 2. Should we wrap reading raw attribute implementation in iio_dev->mlock > mutex? Thanks, Jyoti On Tue, Oct 5, 2021 at 5:52 AM Vasyl Vavrychuk <vasyl.vavrychuk@xxxxxxxxxxxxxxx> wrote: > > 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;
