[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3529c7ef-95e1-42b0-93c3-6ac4266c4b19@kernel.org>
Date: Wed, 4 Sep 2024 15:20:01 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nikunj Kela <quic_nkela@...cinc.com>
Cc: andersson@...nel.org, konradybcio@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, rafael@...nel.org,
viresh.kumar@...aro.org, herbert@...dor.apana.org.au, davem@...emloft.net,
sudeep.holla@....com, andi.shyti@...nel.org, tglx@...utronix.de,
will@...nel.org, robin.murphy@....com, joro@...tes.org,
jassisinghbrar@...il.com, lee@...nel.org, linus.walleij@...aro.org,
amitk@...nel.org, thara.gopinath@...il.com, broonie@...nel.org,
cristian.marussi@....com, rui.zhang@...el.com, lukasz.luba@....com,
wim@...ux-watchdog.org, linux@...ck-us.net, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org, linux-crypto@...r.kernel.org,
arm-scmi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-i2c@...r.kernel.org, iommu@...ts.linux.dev,
linux-gpio@...r.kernel.org, linux-serial@...r.kernel.org,
linux-spi@...r.kernel.org, linux-watchdog@...r.kernel.org,
kernel@...cinc.com, quic_psodagud@...cinc.com,
Praveen Talari <quic_ptalari@...cinc.com>
Subject: Re: [PATCH v2 15/21] dt-bindings: i2c: document support for SA8255p
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@...cinc.com>
>>> Signed-off-by: Nikunj Kela <quic_nkela@...cinc.com>
>>> ---
>>> .../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
Powered by blists - more mailing lists