Message-ID: <3bc186e8-40fc-4c92-affa-0e0b6cf25153@kernel.org>
Date: Fri, 26 Jul 2024 16:58:05 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jagadeesh Kona <quic_jkona@...cinc.com>,
 Bjorn Andersson <andersson@...nel.org>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
 Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 Konrad Dybcio <konrad.dybcio@...aro.org>, Taniya Das
 <quic_tdas@...cinc.com>, Satya Priya Kakitapalli
 <quic_skakitap@...cinc.com>, Imran Shaik <quic_imrashai@...cinc.com>,
 Ajit Pandey <quic_ajipan@...cinc.com>, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH V2] dt-bindings: clock: qcom: Remove required-opps from
 required list on SM8650

On 26/07/2024 16:30, Jagadeesh Kona wrote:
> 
> 
> On 7/24/2024 1:38 PM, Krzysztof Kozlowski wrote:
>> On 20/07/2024 07:28, Jagadeesh Kona wrote:
>>> On SM8650, the minimum voltage corner supported on MMCX from cmd-db is
>>> sufficient for clock controllers to operate and there is no need to specify
>>> the required-opps. Hence remove the required-opps property from the list of
>>> required properties for SM8650 camcc and videocc bindings.
>>>
>>> This fixes:
>>> arch/arm64/boot/dts/qcom/sm8650-mtp.dtb: clock-controller@...0000:
>>> 'required-opps' is a required property
>>>
>>> arch/arm64/boot/dts/qcom/sm8650-mtp.dtb: clock-controller@...0000:
>>> 'required-opps' is a required property
>>>
>>> Fixes: a6a61b9701d1 ("dt-bindings: clock: qcom: Add SM8650 video clock controller")
>>> Fixes: 1ae3f0578e0e ("dt-bindings: clock: qcom: Add SM8650 camera clock controller")
>>> Reported-by: kernel test robot <lkp@...el.com>
>>> Closes: https://lore.kernel.org/oe-kbuild-all/202407070147.C9c3oTqS-lkp@intel.com/
>>> Signed-off-by: Jagadeesh Kona <quic_jkona@...cinc.com>
>>> ---
>>> Changes in V2:
>>>   - Made required: conditional and dropped required-opps from it only for SM8650 platform
>>>   - Dropped Krzysztof Acked-by tag due to above changes
>>>   - Link to V1: https://lore.kernel.org/all/20240708130836.19273-1-quic_jkona@quicinc.com/#r
>>>
>>> .../bindings/clock/qcom,sm8450-camcc.yaml     | 26 +++++++++++++------
>>>   .../bindings/clock/qcom,sm8450-videocc.yaml   | 25 +++++++++++++-----
>>>   2 files changed, 36 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
>>> index f58edfc10f4c..8698c801ed11 100644
>>> --- a/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-camcc.yaml
>>> @@ -21,9 +21,6 @@ description: |
>>>       include/dt-bindings/clock/qcom,sm8650-camcc.h
>>>       include/dt-bindings/clock/qcom,x1e80100-camcc.h
>>>   
>>> -allOf:
>>> -  - $ref: qcom,gcc.yaml#
>>> -
>>>   properties:
>>>     compatible:
>>>       enum:
>>> @@ -53,11 +50,24 @@ properties:
>>>     reg:
>>>       maxItems: 1
>>>   
>>> -required:
>>
>> You cannot remove required block.
>>
>>> -  - compatible
>>> -  - clocks
>>> -  - power-domains
>>> -  - required-opps
>>> +allOf:
>>> +  - $ref: qcom,gcc.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: qcom,sm8650-camcc
>>> +    then:
>>> +      required:
>>> +        - compatible
>>> +        - clocks
>>> +        - power-domains
>>> +    else:
>>> +      required:
>>> +        - compatible
>>> +        - clocks
>>> +        - power-domains
>>> +        - required-opps
>>>   
>>>   unevaluatedProperties: false
>>>   
>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>>> index b2792b4bb554..2e5a061f33d6 100644
>>> --- a/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/qcom,sm8450-videocc.yaml
>>> @@ -40,15 +40,26 @@ properties:
>>>       description:
>>>         A phandle to an OPP node describing required MMCX performance point.
>>>   
>>> -required:
>>
>> No, you cannot remove required block.
>>
>> To clarify: there is almost no single binding using your style. Even if
>> there is one, then 99 others are using it differently. Do not implement
>> things entirely different than everyone else. This is the same for C
>> code you send upstream. No difference here...
>>
> 
> Thanks Krzysztof for the explanation.
> 
> Hi Dmitry,
> 
> As we discussed during SM8650 camcc and videocc changes, the MMCX rail's 
> minimum voltage level from cmd-db is adequate for these clock 
> controllers to operate on SM8650. So, we removed the 'required-opps' 
> property from their DT nodes.

Not sure with whom you discuss. With Dmitry or me. Anyway, I said
nothing about required-opps, but the "required:" block.

> 
> Although 'required-opps' will remain in the properties list, it’s not 
> mandatory to be present in 'required:' list, as it is dependent on 
> cmd-db minimum level. So, can I please go ahead and update these 
> bindings to remove 'required-opps' from the 'required:' list, as done in 
> v1 of this series.
> 
> It seems unconventional to make 'required:' conditional based on the 
> platform type.
> 

Obviously. But nothing stops you - and there are plenty of examples - of
requiring one particular property based on the variant.

Best regards,
Krzysztof

Powered by blists - more mailing lists