Messages in this thread | | | Date | Fri, 8 Oct 2021 14:32:27 +0300 | From | Andriy Tryshnivskyy <> | Subject | Re: [PATCH v3 0/1] iio/scmi: Add reading "raw" attribute. |
| |
Hi Jyoti and Vasyl,
Thanks for your review. I will provide new patch version soon.
Thanks, Andriy
On 06.10.21 03:16, Jyoti Bhayana 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. > > > 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@opensynergy.com> 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;
| |