Messages in this thread | | | Subject | Re: [PATCH v2 4/6] firmware: arm_scmi: add SCMIv3.0 Sensors timestamped reads | From | Peter Hilber <> | Date | Tue, 10 Nov 2020 17:01:26 +0100 |
| |
On 26.10.20 21:10, Cristian Marussi wrote: > Add new .reading_get_timestamped() method to sensor_ops to support SCMIv3.0 > timestamped reads. > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > drivers/firmware/arm_scmi/sensors.c | 134 ++++++++++++++++++++++++++-- > include/linux/scmi_protocol.h | 22 +++++ > 2 files changed, 151 insertions(+), 5 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c > index 5a18f8c84bef..bdb0ed0df683 100644 > --- a/drivers/firmware/arm_scmi/sensors.c > +++ b/drivers/firmware/arm_scmi/sensors.c > @@ -156,6 +156,27 @@ struct scmi_msg_sensor_reading_get { > #define SENSOR_READ_ASYNC BIT(0) > }; > > +struct scmi_resp_sensor_reading_get { > + __le64 readings; > +}; > + > +struct scmi_resp_sensor_reading_complete { > + __le32 id; > + __le64 readings; > +};
In my understanding the id field is not present in the spec. The implementation seems to have introduced it already before this patch.
> + > +struct scmi_sensor_reading_le { > + __le32 sensor_value_low; > + __le32 sensor_value_high; > + __le32 timestamp_low; > + __le32 timestamp_high; > +}; > + > +struct scmi_resp_sensor_reading_complete_v3 { > + __le32 id; > + struct scmi_sensor_reading_le readings[]; > +};
As above, IMHO the id field is not present in the spec.
> + > struct scmi_sensor_trip_notify_payld { > __le32 agent_id; > __le32 sensor_id; > @@ -576,6 +597,21 @@ scmi_sensor_trip_point_config(const struct scmi_handle *handle, u32 sensor_id, > return ret; > } > > +/** > + * scmi_sensor_reading_get - Read scalar sensor value > + * @handle: Platform handle > + * @sensor_id: Sensor ID > + * @value: The 64bit value sensor reading > + * > + * This function returns a single 64 bit reading value representing the sensor > + * value; if the platform SCMI Protocol implementation and the sensor support > + * multiple axis and timestamped-reads, this just returns the first axis while > + * dropping the timestamp value. > + * Use instead the @scmi_sensor_reading_get_timestamped to retrieve the array of > + * timestamped multi-axis values. > + * > + * Return: 0 on Success > + */ > static int scmi_sensor_reading_get(const struct scmi_handle *handle, > u32 sensor_id, u64 *value) > { > @@ -593,18 +629,105 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle,
How about changing the scmi_xfer_get_init() rx_size to 0 (in the immediately preceding, not shown lines)? An SCMI platform might not expect to just have room for the first reading, excluding the timestamp.
> > sensor = t->tx.buf; > sensor->id = cpu_to_le32(sensor_id); > + if (s->async) { > + sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC); > + ret = scmi_do_xfer_with_response(handle, t); > + if (!ret) { > + struct scmi_resp_sensor_reading_complete *resp; > + > + resp = t->rx.buf; > + if (le32_to_cpu(resp->id) == sensor_id) > + *value = le64_to_cpu(resp->readings);
Maybe this le64_to_cpu() and the one a few lines below should be replaced by get_unaligned_le64()?
> + else > + ret = -EPROTO; > + } > + } else { > + sensor->flags = cpu_to_le32(0); > + ret = scmi_do_xfer(handle, t); > + if (!ret) { > + struct scmi_resp_sensor_reading_get *resp; > + > + resp = t->rx.buf; > + *value = le64_to_cpu(resp->readings); > + } > + } > > + scmi_xfer_put(handle, t); > + return ret; > +} > +
[...] > + > /** > * scmi_range_attrs - specifies a sensor or axis values' range > * @min_range: The minimum value which can be represented by the sensor/axis. > @@ -387,6 +401,11 @@ enum scmi_sensor_class { > * @info_get: get the information of the specified sensor > * @trip_point_config: selects and configures a trip-point of interest > * @reading_get: gets the current value of the sensor > + * @reading_get_timestamped: gets the current value and timestamp, when > + * available, of the sensor. (as of v2.1 spec)
Nitpicking: v2.1 -> v3.0
> + * Supports multi-axis sensors for sensors which > + * supports it and if the @reading array size of > + * @count entry equals the sensor num_axis > */ > struct scmi_sensor_ops { > int (*count_get)(const struct scmi_handle *handle); > @@ -396,6 +415,9 @@ struct scmi_sensor_ops { > u32 sensor_id, u8 trip_id, u64 trip_value); > int (*reading_get)(const struct scmi_handle *handle, u32 sensor_id, > u64 *value); > + int (*reading_get_timestamped)(const struct scmi_handle *handle, > + u32 sensor_id, u8 count, > + struct scmi_sensor_reading *readings); > }; > > /** >
Best regards,
Peter
| |