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

[Jian Hu]

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@xxxxxxxxxx> 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@xxxxxxxxxxx>
> > > ---
> > >   .../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@xxxxxxxxxx>
> > > +  - Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> > > +  - Jian Hu <jian.hu@xxxxxxxxxxx>
> > > +  - Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
> > > +
> > > +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