Re: [PATCH v2 15/21] dt-bindings: i2c: document support for SA8255p

[Nikunj Kela <quic_nkela@xxxxxxxxxxx>]

On 9/3/2024 11:31 PM, Krzysztof Kozlowski wrote:
> On Tue, Sep 03, 2024 at 03:02:34PM -0700, Nikunj Kela wrote:
>> Add compatible representing i2c support on SA8255p.
>>
>> Clocks and interconnects are being configured in Firmware VM
>> on SA8255p, therefore making them optional.
>>
>> CC: Praveen Talari <quic_ptalari@xxxxxxxxxxx>
>> Signed-off-by: Nikunj Kela <quic_nkela@xxxxxxxxxxx>
>> ---
>>  .../bindings/i2c/qcom,i2c-geni-qcom.yaml      | 33 +++++++++++++++++--
>>  1 file changed, 31 insertions(+), 2 deletions(-)
>>
> I don't know what to do with this patch. Using specific compatibles next
> to generic compatible is just wrong, although mistake was probably
> allowing generic compatible. The patch does not explain the differences
> in interface which would explain why devices are not compatible.

I mentioned in the description that clocks and interconnects on this
platform are configured in Firmware VM(over SCMI using power and perf
domains) therefore this is not compatible with existing generic compatible.


>  In the
> same time my advice of separate binding was not followed, because maybe
> these devices are compatible? But then it should be expressed...

Sorry, I missed that. You want me to use 'oneOf' expression with this
compatible?


>
> You have entire commit msg to explain what and why.

Will put more details in description.


>> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> index 9f66a3bb1f80..b477fae734b6 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -15,6 +15,7 @@ properties:
>>      enum:
>>        - qcom,geni-i2c
>>        - qcom,geni-i2c-master-hub
>> +      - qcom,sa8255p-geni-i2c
>>  
>>    clocks:
>>      minItems: 1
>> @@ -69,8 +70,6 @@ properties:
>>  required:
>>    - compatible
>>    - interrupts
>> -  - clocks
>> -  - clock-names
>>    - reg
>>  
>>  allOf:
>> @@ -81,6 +80,10 @@ allOf:
>>            contains:
>>              const: qcom,geni-i2c-master-hub
>>      then:
>> +      required:
>> +        - clocks
>> +        - clock-names
>
> So it is required here?

We are removing clocks from generic required list and enforcing rules
for all compatibles other than sa8255p.


>> +
>>        properties:
>>          clocks:
>>            minItems: 2
>> @@ -100,7 +103,21 @@ allOf:
>>            items:
>>              - const: qup-core
>>              - const: qup-config
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: qcom,sa8255p-geni-i2c
>> +    then:
>> +      required:
>> +        - power-domains
>> +
> And possible here? I assume with the same clocks? The same for
> interconnects - same values are valid?

I guess I need to put here the same description as in the cover letter
to make it more clear. We are not using clocks and interconnects in this
platform in Linux. Instead, sending request to Firmware VM over
SCMI(using power and perf protocols)


>
>>      else:
>> +      required:
>> +        - clocks
>> +        - clock-names
> And clocks are required again?
Explained above.
>> +
>>        properties:
>>          clocks:
>>            maxItems: 1
> Eeee? So now all other variants have max 1 clock?

I will make if block for sa8255p up so else is not applied to rest of
the platforms.


>
> Nope, this wasn't ever tested on real DTS.

This is tested on SA8255p DTS and I ran DT schema check on SA8775p DT as
well.


>
> Best regards,
> Krzysztof
>