Re: [PATCH v2 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions

[Peter Hilber]

Hi Cristian,

sorry, I mistakenly used the wrong sender ("Mailing Lists") for the
original comment mail. Please see below for my reply.

On 10.11.20 18:21, Cristian Marussi wrote:
> On Tue, Nov 10, 2020 at 05:00:05PM +0100, Mailing Lists wrote:
>> On 26.10.20 21:10, Cristian Marussi wrote:
>>> Add support for new SCMIv3.0 Sensors extensions related to new sensors'
>>> features, like multiple axis and update intervals, while keeping
>>> compatibility with SCMIv2.0 features.
>>> While at that, refactor and simplify all the internal helpers macros and
>>> move struct scmi_sensor_info to use only non-fixed-size typing.
>>>
>>> Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
>>> ---
>>> v1 --> v2
>>> - restrict segmented intervals descriptors to single triplet
>>> - add proper usage of scmi_reset_rx_to_maxsz
>>> ---
>>>  drivers/firmware/arm_scmi/sensors.c | 391 ++++++++++++++++++++++++++--
>>>  include/linux/scmi_protocol.h       | 219 +++++++++++++++-
>>>  2 files changed, 584 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
>>> index 6aaff478d032..5a18f8c84bef 100644
>>> --- a/drivers/firmware/arm_scmi/sensors.c
>>> +++ b/drivers/firmware/arm_scmi/sensors.c
>>> @@ -7,16 +7,21 @@
>>>
>>>  #define pr_fmt(fmt) "SCMI Notifications SENSOR - " fmt
>>>
>>> +#include <linux/bitfield.h>
>>>  #include <linux/scmi_protocol.h>
>>>
>>>  #include "common.h"
>>>  #include "notify.h"
>>>
>>> +#define SCMI_MAX_NUM_SENSOR_AXIS   64
>>
>> IMHO the related 6 bit wide fields, like SENSOR_DESCRIPTION_GET "Number
>> of axes", should determine the maximum value, so 64 -> 63.
>>
> 
> Yes, bits [21:16] 'Number of Axes' in sensor_attributes_high, but this
> #define was meant to represent the maximum number of sensor axis (64...ranging
> from 0 to 63) not the highest possible numbered (63).
> 

But in my understanding the actual maximum number of sensor axes is 63
due to the maximum value 63 of 'Number of Axes', 64 would overflow
already. The ids would range from 0 to 62.

That said, in my understanding there is no problem with retaining a
higher value ATM.

Best regards,

Peter