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

[Praveenkumar I]

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@xxxxxxxxxxx>
> > > > ---
> > > >    .../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