Re: [PATCH v2 4/6] firmware: arm_scmi: add SCMIv3.0 Sensors timestamped reads

[Peter Hilber]

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@xxxxxxx>
> ---
>  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