[1/6] dt-bindings: thermal: tsens: Add nvmem cells for calibration data

Message ID 20230710103735.1375847-2-quic_ipkumar@quicinc.com
State New
Headers
Series Add IPQ5332 TSENS support |

Commit Message

Praveenkumar I July 10, 2023, 10:37 a.m. UTC
  Add TSENS V2 calibration nvmem cells for IPQ5332

Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
---
 .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)
  

Comments

Krzysztof Kozlowski July 10, 2023, 8:10 p.m. UTC | #1
On 10/07/2023 12:37, Praveenkumar I wrote:
> Add TSENS V2 calibration nvmem cells for IPQ5332
> 
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> ---
>  .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> index 27e9e16e6455..8b7863c3989e 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> @@ -91,7 +91,7 @@ properties:
>      maxItems: 2
>  
>    nvmem-cells:
> -    oneOf:
> +    anyOf:
>        - minItems: 1
>          maxItems: 2
>          description:
> @@ -106,9 +106,13 @@ properties:
>          description: |
>            Reference to nvmem cells for the calibration mode, two calibration
>            bases and two cells per each sensor, main and backup copies, plus use_backup cell
> +      - maxItems: 17
> +        description: |
> +          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
> +          bases and one cell per each sensor

I think this is already included in one of the previous entries.
Otherwise, are you sure that all new devices will have exactly 17 entries?

>  
>    nvmem-cell-names:
> -    oneOf:
> +    anyOf:
>        - minItems: 1
>          items:
>            - const: calib
> @@ -205,6 +209,24 @@ properties:
>            - const: s9_p2_backup
>            - const: s10_p1_backup
>            - const: s10_p2_backup
> +      - items:
> +          - const: mode
> +          - const: base0
> +          - const: base1
> +          - const: s0_offset
> +          - const: s3_offset
> +          - const: s4_offset
> +          - const: s5_offset
> +          - const: s6_offset
> +          - const: s7_offset
> +          - const: s8_offset
> +          - const: s9_offset
> +          - const: s10_offset
> +          - const: s11_offset
> +          - const: s12_offset
> +          - const: s13_offset
> +          - const: s14_offset
> +          - const: s15_offset

Don't introduce new naming style. Existing uses s[0-9]+, without offset
suffix. Why this should be different?

>  
>    "#qcom,sensors":
>      description:

Best regards,
Krzysztof
  
Praveenkumar I July 11, 2023, 9:39 a.m. UTC | #2
On 7/11/2023 1:40 AM, Krzysztof Kozlowski wrote:
> On 10/07/2023 12:37, Praveenkumar I wrote:
>> Add TSENS V2 calibration nvmem cells for IPQ5332
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> index 27e9e16e6455..8b7863c3989e 100644
>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> @@ -91,7 +91,7 @@ properties:
>>       maxItems: 2
>>   
>>     nvmem-cells:
>> -    oneOf:
>> +    anyOf:
>>         - minItems: 1
>>           maxItems: 2
>>           description:
>> @@ -106,9 +106,13 @@ properties:
>>           description: |
>>             Reference to nvmem cells for the calibration mode, two calibration
>>             bases and two cells per each sensor, main and backup copies, plus use_backup cell
>> +      - maxItems: 17
>> +        description: |
>> +          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
>> +          bases and one cell per each sensor
> I think this is already included in one of the previous entries.
> Otherwise, are you sure that all new devices will have exactly 17 entries?
Previous entries does not support TSENS version 2.X.X QFPROM. TSENS V2 
QFPROM has mode, base0, base1 and s[0-15]+_offset.
Ideally it should be like,
- minItems: 4
- maxItems: 19
But dt binding check fails in oneOf / anyOf condition. So added the 
IPQ5332 properties which is exactly 17.
>
>>   
>>     nvmem-cell-names:
>> -    oneOf:
>> +    anyOf:
>>         - minItems: 1
>>           items:
>>             - const: calib
>> @@ -205,6 +209,24 @@ properties:
>>             - const: s9_p2_backup
>>             - const: s10_p1_backup
>>             - const: s10_p2_backup
>> +      - items:
>> +          - const: mode
>> +          - const: base0
>> +          - const: base1
>> +          - const: s0_offset
>> +          - const: s3_offset
>> +          - const: s4_offset
>> +          - const: s5_offset
>> +          - const: s6_offset
>> +          - const: s7_offset
>> +          - const: s8_offset
>> +          - const: s9_offset
>> +          - const: s10_offset
>> +          - const: s11_offset
>> +          - const: s12_offset
>> +          - const: s13_offset
>> +          - const: s14_offset
>> +          - const: s15_offset
> Don't introduce new naming style. Existing uses s[0-9]+, without offset
> suffix. Why this should be different?
As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2 
QFPROM layout is different from the existing one.
I would like to add mode, base0, base1 and 16 patterns 
'^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf 
condintion.

--
Thanks,
Praveenkumar
>
>>   
>>     "#qcom,sensors":
>>       description:
> Best regards,
> Krzysztof
>
  
Krzysztof Kozlowski July 11, 2023, 9:52 a.m. UTC | #3
On 11/07/2023 11:39, Praveenkumar I wrote:
> 
> On 7/11/2023 1:40 AM, Krzysztof Kozlowski wrote:
>> On 10/07/2023 12:37, Praveenkumar I wrote:
>>> Add TSENS V2 calibration nvmem cells for IPQ5332
>>>
>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> ---
>>>   .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
>>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> index 27e9e16e6455..8b7863c3989e 100644
>>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> @@ -91,7 +91,7 @@ properties:
>>>       maxItems: 2
>>>   
>>>     nvmem-cells:
>>> -    oneOf:
>>> +    anyOf:
>>>         - minItems: 1
>>>           maxItems: 2
>>>           description:
>>> @@ -106,9 +106,13 @@ properties:
>>>           description: |
>>>             Reference to nvmem cells for the calibration mode, two calibration
>>>             bases and two cells per each sensor, main and backup copies, plus use_backup cell
>>> +      - maxItems: 17
>>> +        description: |
>>> +          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
>>> +          bases and one cell per each sensor
>> I think this is already included in one of the previous entries.
>> Otherwise, are you sure that all new devices will have exactly 17 entries?
> Previous entries does not support TSENS version 2.X.X QFPROM. TSENS V2 
> QFPROM has mode, base0, base1 and s[0-15]+_offset.
> Ideally it should be like,
> - minItems: 4
> - maxItems: 19

I see it covered:
minItems: 5
maxItems: 35

I think 17 is between 5 and 35.

> But dt binding check fails in oneOf / anyOf condition. So added the 
> IPQ5332 properties which is exactly 17.
>>
>>>   
>>>     nvmem-cell-names:
>>> -    oneOf:
>>> +    anyOf:
>>>         - minItems: 1
>>>           items:
>>>             - const: calib
>>> @@ -205,6 +209,24 @@ properties:
>>>             - const: s9_p2_backup
>>>             - const: s10_p1_backup
>>>             - const: s10_p2_backup
>>> +      - items:
>>> +          - const: mode
>>> +          - const: base0
>>> +          - const: base1
>>> +          - const: s0_offset
>>> +          - const: s3_offset
>>> +          - const: s4_offset
>>> +          - const: s5_offset
>>> +          - const: s6_offset
>>> +          - const: s7_offset
>>> +          - const: s8_offset
>>> +          - const: s9_offset
>>> +          - const: s10_offset
>>> +          - const: s11_offset
>>> +          - const: s12_offset
>>> +          - const: s13_offset
>>> +          - const: s14_offset
>>> +          - const: s15_offset
>> Don't introduce new naming style. Existing uses s[0-9]+, without offset
>> suffix. Why this should be different?
> As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2 
> QFPROM layout is different from the existing one.

I know, I did not write about p1/p2.

> I would like to add mode, base0, base1 and 16 patterns 
> '^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf 
> condintion.

This does not explain why you need different style - this "offset" suffix.

Best regards,
Krzysztof
  
Praveenkumar I July 11, 2023, 2:13 p.m. UTC | #4
On 7/11/2023 3:22 PM, Krzysztof Kozlowski wrote:
> On 11/07/2023 11:39, Praveenkumar I wrote:
>> On 7/11/2023 1:40 AM, Krzysztof Kozlowski wrote:
>>> On 10/07/2023 12:37, Praveenkumar I wrote:
>>>> Add TSENS V2 calibration nvmem cells for IPQ5332
>>>>
>>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>>> ---
>>>>    .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
>>>>    1 file changed, 24 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>>> index 27e9e16e6455..8b7863c3989e 100644
>>>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>>> @@ -91,7 +91,7 @@ properties:
>>>>        maxItems: 2
>>>>    
>>>>      nvmem-cells:
>>>> -    oneOf:
>>>> +    anyOf:
>>>>          - minItems: 1
>>>>            maxItems: 2
>>>>            description:
>>>> @@ -106,9 +106,13 @@ properties:
>>>>            description: |
>>>>              Reference to nvmem cells for the calibration mode, two calibration
>>>>              bases and two cells per each sensor, main and backup copies, plus use_backup cell
>>>> +      - maxItems: 17
>>>> +        description: |
>>>> +          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
>>>> +          bases and one cell per each sensor
>>> I think this is already included in one of the previous entries.
>>> Otherwise, are you sure that all new devices will have exactly 17 entries?
>> Previous entries does not support TSENS version 2.X.X QFPROM. TSENS V2
>> QFPROM has mode, base0, base1 and s[0-15]+_offset.
>> Ideally it should be like,
>> - minItems: 4
>> - maxItems: 19
> I see it covered:
> minItems: 5
> maxItems: 35
>
> I think 17 is between 5 and 35.
Okay, will remove the nvmem-cells entry.
>
>> But dt binding check fails in oneOf / anyOf condition. So added the
>> IPQ5332 properties which is exactly 17.
>>>>    
>>>>      nvmem-cell-names:
>>>> -    oneOf:
>>>> +    anyOf:
>>>>          - minItems: 1
>>>>            items:
>>>>              - const: calib
>>>> @@ -205,6 +209,24 @@ properties:
>>>>              - const: s9_p2_backup
>>>>              - const: s10_p1_backup
>>>>              - const: s10_p2_backup
>>>> +      - items:
>>>> +          - const: mode
>>>> +          - const: base0
>>>> +          - const: base1
>>>> +          - const: s0_offset
>>>> +          - const: s3_offset
>>>> +          - const: s4_offset
>>>> +          - const: s5_offset
>>>> +          - const: s6_offset
>>>> +          - const: s7_offset
>>>> +          - const: s8_offset
>>>> +          - const: s9_offset
>>>> +          - const: s10_offset
>>>> +          - const: s11_offset
>>>> +          - const: s12_offset
>>>> +          - const: s13_offset
>>>> +          - const: s14_offset
>>>> +          - const: s15_offset
>>> Don't introduce new naming style. Existing uses s[0-9]+, without offset
>>> suffix. Why this should be different?
>> As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2
>> QFPROM layout is different from the existing one.
> I know, I did not write about p1/p2.
>
>> I would like to add mode, base0, base1 and 16 patterns
>> '^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf
>> condintion.
> This does not explain why you need different style - this "offset" suffix.
In QFPROM, the BIT field names are s0_offset, s3_offset to s15_offset. 
s1_offset and s2_offset not present in IPQ5332.
Hence used the same "offset" suffix here. May I know in this case, which 
nvmem-cells-names you are suggesting to use?

--
Thanks,
Praveenkumar
>
> Best regards,
> Krzysztof
>
  
Krzysztof Kozlowski July 13, 2023, 11:38 a.m. UTC | #5
On 11/07/2023 16:13, Praveenkumar I wrote:
>>>>>              - const: calib
>>>>> @@ -205,6 +209,24 @@ properties:
>>>>>              - const: s9_p2_backup
>>>>>              - const: s10_p1_backup
>>>>>              - const: s10_p2_backup
>>>>> +      - items:
>>>>> +          - const: mode
>>>>> +          - const: base0
>>>>> +          - const: base1
>>>>> +          - const: s0_offset
>>>>> +          - const: s3_offset
>>>>> +          - const: s4_offset
>>>>> +          - const: s5_offset
>>>>> +          - const: s6_offset
>>>>> +          - const: s7_offset
>>>>> +          - const: s8_offset
>>>>> +          - const: s9_offset
>>>>> +          - const: s10_offset
>>>>> +          - const: s11_offset
>>>>> +          - const: s12_offset
>>>>> +          - const: s13_offset
>>>>> +          - const: s14_offset
>>>>> +          - const: s15_offset
>>>> Don't introduce new naming style. Existing uses s[0-9]+, without offset
>>>> suffix. Why this should be different?
>>> As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2
>>> QFPROM layout is different from the existing one.
>> I know, I did not write about p1/p2.
>>
>>> I would like to add mode, base0, base1 and 16 patterns
>>> '^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf
>>> condintion.
>> This does not explain why you need different style - this "offset" suffix.
> In QFPROM, the BIT field names are s0_offset, s3_offset to s15_offset. 
> s1_offset and s2_offset not present in IPQ5332.
> Hence used the same "offset" suffix here. May I know in this case, which 
> nvmem-cells-names you are suggesting to use?

"Existing uses s[0-9]+"

so s[0-9]+

Best regards,
Krzysztof
  

Patch

diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
index 27e9e16e6455..8b7863c3989e 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
@@ -91,7 +91,7 @@  properties:
     maxItems: 2
 
   nvmem-cells:
-    oneOf:
+    anyOf:
       - minItems: 1
         maxItems: 2
         description:
@@ -106,9 +106,13 @@  properties:
         description: |
           Reference to nvmem cells for the calibration mode, two calibration
           bases and two cells per each sensor, main and backup copies, plus use_backup cell
+      - maxItems: 17
+        description: |
+          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
+          bases and one cell per each sensor
 
   nvmem-cell-names:
-    oneOf:
+    anyOf:
       - minItems: 1
         items:
           - const: calib
@@ -205,6 +209,24 @@  properties:
           - const: s9_p2_backup
           - const: s10_p1_backup
           - const: s10_p2_backup
+      - items:
+          - const: mode
+          - const: base0
+          - const: base1
+          - const: s0_offset
+          - const: s3_offset
+          - const: s4_offset
+          - const: s5_offset
+          - const: s6_offset
+          - const: s7_offset
+          - const: s8_offset
+          - const: s9_offset
+          - const: s10_offset
+          - const: s11_offset
+          - const: s12_offset
+          - const: s13_offset
+          - const: s14_offset
+          - const: s15_offset
 
   "#qcom,sensors":
     description: