Re: [PATCH v2 1/5] dt-bindings: clock: add Amlogic T7 PLL clock controller

[Jian Hu <jian.hu@amlogic.com>]

Hi, Jerome


Thanks for your review.


On 2025/1/14 1:50, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 10 Jan 2025 at 09:54, Rob Herring <robh@kernel.org> wrote:
>
>> On Wed, Jan 08, 2025 at 05:40:21PM +0800, Jian Hu wrote:
>>> Add DT bindings for the PLL clock controller of the Amlogic T7 SoC family.
>>>
>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>> ---
>>>   .../bindings/clock/amlogic,t7-pll-clkc.yaml   | 103 ++++++++++++++++++
>>>   .../dt-bindings/clock/amlogic,t7-pll-clkc.h   |  57 ++++++++++
>>>   2 files changed, 160 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,t7-pll-clkc.yaml
>>>   create mode 100644 include/dt-bindings/clock/amlogic,t7-pll-clkc.h
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/clock/amlogic,t7-pll-clkc.yaml
>>> b/Documentation/devicetree/bindings/clock/amlogic,t7-pll-clkc.yaml
>>> new file mode 100644
>>> index 000000000000..fd0323678d37
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,t7-pll-clkc.yaml
>>> @@ -0,0 +1,103 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +# Copyright (C) 2024 Amlogic, Inc. All rights reserved
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/amlogic,t7-pll-clkc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Amlogic T7 PLL Clock Control Controller
>>> +
>>> +maintainers:
>>> +  - Neil Armstrong <neil.armstrong@linaro.org>
>>> +  - Jerome Brunet <jbrunet@baylibre.com>
>>> +  - Jian Hu <jian.hu@amlogic.com>
>>> +  - Xianwei Zhao <xianwei.zhao@amlogic.com>
>>> +
>>> +if:
>> Move this after 'required' section.
>>
>> Generally we put 'if' under 'allOf' because we're likely to have another
>> if/then schema on the next compatible added. If you don't think this
>> binding will ever get used on another chip, then it is fine as-is.
>>
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        const: amlogic,t7-pll-mclk
>>> +
>>> +then:
>>> +  properties:
>>> +    clocks:
>>> +      items:
>>> +        - description: mclk pll input oscillator gate
>>> +        - description: 24M oscillator input clock source for mclk_sel_0
>>> +        - description: fix 50Mhz input clock source for mclk_sel_0
> The rate is whatever the clock will actually be. Better not to mention
> it in this doc.


OK, I will remove the rate here.

>>> +
>>> +    clock-names:
>>> +      items:
> one being "input" and other suffixed "_in" looks really odd
>
>>> +        - const: input
>>> +        - const: mclk_in0
>>> +        - const: mclk_in1
> or just in0, in1, in2 if you are going with Rob's suggestion.
> Having "mclk_" in the top level would be confusing.


Ok, I will use in0/in1/in2 for them.

> --
> 2.47.1
>
> --
> Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic