Re: [PATCH 1/2] clk: qcom: Add DT bindings for ipq6018 apss clock controller

[Sivaprakash Murugesan <sivaprak@codeaurora.org>]

Hi Rob,

Thanks for the review.

On 3/10/2020 2:43 AM, Rob Herring wrote:
> On Thu, Feb 27, 2020 at 03:25:17PM +0530, Sivaprakash Murugesan wrote:
>> add dt-binding for ipq6018 apss clock controller
>>
>> Signed-off-by: Sivaprakash Murugesan <sivaprak@codeaurora.org>
>> ---
>>   .../devicetree/bindings/clock/qcom,apsscc.yaml     | 58 ++++++++++++++++++++++
>>   include/dt-bindings/clock/qcom,apss-ipq6018.h      | 26 ++++++++++
>>   2 files changed, 84 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/qcom,apsscc.yaml
>>   create mode 100644 include/dt-bindings/clock/qcom,apss-ipq6018.h
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,apsscc.yaml b/Documentation/devicetree/bindings/clock/qcom,apsscc.yaml
>> new file mode 100644
>> index 0000000..7433721
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,apsscc.yaml
>> @@ -0,0 +1,58 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
> Dual license new bindings please:
>
> (GPL-2.0-only OR BSD-2-Clause)
ok.
>
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/bindings/clock/qcom,apsscc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm IPQ6018 APSS Clock Controller Binding
>> +
>> +maintainers:
>> +  - Stephen Boyd <sboyd@kernel.org>
> I'd expect this to be a QCom person, not who is applying patches.
>
>> +
>> +description: |
> You can drop '|'.
ok.
>> +  Qualcomm IPQ6018 APSS clock control module which supports the clocks with
>> +  frequencies above 800Mhz.
>> +
>> +properties:
>> +  compatible :
>> +    const: qcom,apss-ipq6018
> Normal ordering is: qcom,ipq6018-apss
ok.
>> +
>> +  clocks:
>> +    description: clocks required for this controller.
>> +    maxItems: 4
> Need to define what each clock is.
ok.
>> +
>> +  clock-names:
>> +    description: clock output names of required clocks.
>> +    maxItems: 4
>> +
>> +  '#clock-cells':
>> +    const: 1
>> +
>> +  '#reset-cells':
>> +    const: 1
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#clock-cells'
>> +  - '#reset-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +      #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
>> +      apss_clk: qcom,apss_clk@b111000 {
> I thought I'd finally seen the last of these Qcom node names...
>
> clock-controller@...
ok.
>> +            compatible = "qcom,apss-ipq6018";
>> +            clocks = <&xo>, <&gcc GPLL0>,
>> +                        <&gcc GPLL2>, <&gcc GPLL4>;
>> +            clock-names = "xo", "gpll0",
>> +                         "gpll2", "gpll4";
>> +            reg = <0xb11100c 0x5ff4>;
>> +            #clock-cells = <1>;
>> +            #reset-cells = <1>;
>> +      };
>> +...
>> diff --git a/include/dt-bindings/clock/qcom,apss-ipq6018.h b/include/dt-bindings/clock/qcom,apss-ipq6018.h
>> new file mode 100644
>> index 0000000..ed9d7d8
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/qcom,apss-ipq6018.h
>> @@ -0,0 +1,26 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> I'm pretty sure your employer would like an additional license here.
my bad. will correct it.