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.maru...@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
