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
