Messages in this thread | | | Date | Mon, 18 Oct 2021 09:49:32 +0300 | Subject | Re: [PATCH v5 1/1] iio/scmi: Add reading "raw" attribute. | From | Andriy Tryshnivskyy <> |
| |
On 17.10.21 14:51, Jonathan Cameron 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 Fri, 8 Oct 2021 21:28:26 +0300 > Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com> wrote: > >> Add scmi_iio_get_raw() to read "raw" attribute. >> >> Signed-off-by: Andriy Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com> >> --- > For a single patch series, it is better to put a change log in the patch (here) > Whilst I can see why you would use this approach rather than the read_raw callback > there are significant disadvantages in doing so. The channel can't be used > by in kernel users such as iio-hwmon. That may cause you more trouble than > it is worth in the long run. > > Note that you could also define a new IIO_VAL type to still use the two > (possibly) 32 bit values and return a 64 bit value. That way, with appropriate > additions in the consumer drivers the channel could still be used. > > IIO_VAL_INT_64 perhaps with val as the lower 32 bits and val2 as the upper > with appropriate care around the sign.
Hi Jonathan,
In the past I also was thinking about introducing a new type to return 64 bits value - IIO_VAL_INT_LONG. However since it requires changes in
include/linux/iio/types.h drivers/iio/industrialio-core.c
I thought it will be harder to get approval for this approach. But now it seems there is no other way. So I will prepare a new patch version soon which introduces IIO_VAL_INT_LONG.
Thank you for your review!
> >> drivers/iio/common/scmi_sensors/scmi_iio.c | 61 ++++++++++++++++++++++ >> 1 file changed, 61 insertions(+) >> >> diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c >> index 7cf2bf282cef..691cbbd61e3a 100644 >> --- a/drivers/iio/common/scmi_sensors/scmi_iio.c >> +++ b/drivers/iio/common/scmi_sensors/scmi_iio.c >> @@ -311,6 +311,62 @@ static const struct iio_info scmi_iio_info = { >> .write_raw = scmi_iio_write_raw, >> }; >> >> +static ssize_t scmi_iio_get_raw(struct iio_dev *iio_dev, uintptr_t private, >> + const struct iio_chan_spec *chan, char *buf) >> +{ >> + struct scmi_iio_priv *sensor = iio_priv(iio_dev); >> + int err; >> + u32 sensor_config; >> + struct scmi_sensor_reading readings[SCMI_IIO_NUM_OF_AXIS]; >> + int len = 0; >> + >> + err = iio_device_claim_direct_mode(iio_dev); >> + if (err) { >> + dev_err(&iio_dev->dev, >> + "Error in claiming direct mode for sensor %s err %d", >> + sensor->sensor_info->name, err); > It's not an error, it just means the device is busy, so at most dev_info() > or just rely on userspace correctly interpreting EBUSY.
Agree. Will use dev_info(). Thank you!
>> + goto err_release; >> + } >> + >> + 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); >> + goto err_release; >> + } >> + >> + 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); >> + goto err_release; >> + } >> + >> + 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); >> + goto err_release; >> + } >> + >> + len = scnprintf(buf, PAGE_SIZE, "%lld\n", >> + readings[chan->scan_index].value); >> + >> +err_release: >> + iio_device_release_direct_mode(iio_dev); >> + >> + return len; >> +} >> + >> static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev, >> uintptr_t private, >> const struct iio_chan_spec *chan, >> @@ -355,6 +411,11 @@ static ssize_t scmi_iio_get_raw_available(struct iio_dev *iio_dev, >> } >> >> static const struct iio_chan_spec_ext_info scmi_iio_ext_info[] = { >> + { >> + .name = "raw", >> + .read = scmi_iio_get_raw, >> + .shared = IIO_SEPARATE, >> + }, >> { >> .name = "raw_available", >> .read = scmi_iio_get_raw_available, >
| |