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

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 04/09/2024 14:41, Nikunj Kela wrote:
> 
> 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.

It is not obvious to me. I doubt it is obvious to others. Commit msg
does not say they are compatible and usually difference in
clocks/interconnects is not reason of incompatibility. So why suddenly
here we would understand it differently?


> 
> 
>>  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?

I proposed separate binding file. But your commit msg suggested these
are compatible. Lack of driver change is also proof of that.

I don't want to keep discussing this because it does not lead to
anywhere. We keep repeating the same.

> 
> 
>>
>> 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.

You just affected all the DTS everywhere. It's your task to check all
DTS everywhere. Not ours.

Best regards,
Krzysztof