diff mbox series

[v22,1/8] dt-bindings: clock: npcm845: Add reference 25m clock property

Message ID 20240108135421.684263-2-tmaimon77@gmail.com (mailing list archive)
State Changes Requested, archived
Headers show
Series Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand

Commit Message

Tomer Maimon Jan. 8, 2024, 1:54 p.m. UTC
The NPCM8XX clock driver uses 25Mhz external clock, therefor adding
refclk property.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Rob Herring Jan. 8, 2024, 9:09 p.m. UTC | #1
On Mon, 08 Jan 2024 15:54:14 +0200, Tomer Maimon wrote:
> The NPCM8XX clock driver uses 25Mhz external clock, therefor adding
> refclk property.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.example.dtb: clock-controller@f0801000: 'clocks' is a required property
	from schema $id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.example.dtb: clock-controller@f0801000: 'clock-names' is a required property
	from schema $id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240108135421.684263-2-tmaimon77@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Tomer Maimon Jan. 9, 2024, 6:45 a.m. UTC | #2
Hi Rob,

Thanks for your comment.

On Mon, 8 Jan 2024 at 23:09, Rob Herring <robh@kernel.org> wrote:
>
>
> On Mon, 08 Jan 2024 15:54:14 +0200, Tomer Maimon wrote:
> > The NPCM8XX clock driver uses 25Mhz external clock, therefor adding
> > refclk property.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> >  .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.example.dtb: clock-controller@f0801000: 'clocks' is a required property
>         from schema $id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.example.dtb: clock-controller@f0801000: 'clock-names' is a required property
>         from schema $id: http://devicetree.org/schemas/clock/nuvoton,npcm845-clk.yaml#
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240108135421.684263-2-tmaimon77@gmail.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>

probably I missed adding the clock and clock-names to the example
node, will be fixed next version
Rob Herring Jan. 9, 2024, 5:08 p.m. UTC | #3
On Mon, Jan 08, 2024 at 03:54:14PM +0200, Tomer Maimon wrote:
> The NPCM8XX clock driver uses 25Mhz external clock, therefor adding

therefore

> refclk property.

'refclk' is not a property.

> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> index b901ca13cd25..0b642bfce292 100644
> --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> @@ -21,6 +21,14 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  clocks:
> +    items:
> +      - description: 25Mhz referance clock

reference

> +
> +  clock-names:
> +    items:
> +      - const: refclk
> +
>    '#clock-cells':
>      const: 1
>      description:
> @@ -30,12 +38,20 @@ properties:
>  required:
>    - compatible
>    - reg
> +  - clocks
> +  - clock-names

New required properties are an ABI break. That's fine if you explain why 
that's okay in the commit msg.


>    - '#clock-cells'
>  
>  additionalProperties: false
>  
>  examples:
>    - |
> +    refclk: refclk-25mhz {
> +        compatible = "fixed-clock";
> +        #clock-cells = <0>;
> +        clock-frequency = <25000000>;
> +    }; 

Examples don't need to show providers.

> +  
>      ahb {
>          #address-cells = <2>;
>          #size-cells = <2>;
> -- 
> 2.34.1
>
Tomer Maimon Jan. 10, 2024, 1:47 p.m. UTC | #4
Hi Rob,

Thanks for your comment.

On Tue, 9 Jan 2024 at 19:08, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jan 08, 2024 at 03:54:14PM +0200, Tomer Maimon wrote:
> > The NPCM8XX clock driver uses 25Mhz external clock, therefor adding
>
> therefore
>
> > refclk property.
>
> 'refclk' is not a property.
>
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> >  .../bindings/clock/nuvoton,npcm845-clk.yaml      | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > index b901ca13cd25..0b642bfce292 100644
> > --- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > +++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
> > @@ -21,6 +21,14 @@ properties:
> >    reg:
> >      maxItems: 1
> >
> > +  clocks:
> > +    items:
> > +      - description: 25Mhz referance clock
>
> reference
>
> > +
> > +  clock-names:
> > +    items:
> > +      - const: refclk
> > +
> >    '#clock-cells':
> >      const: 1
> >      description:
> > @@ -30,12 +38,20 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > +  - clocks
> > +  - clock-names
>
> New required properties are an ABI break. That's fine if you explain why
> that's okay in the commit msg.
What do you mean?
Could I add the new required properties to the required list?
>
>
> >    - '#clock-cells'
> >
> >  additionalProperties: false
> >
> >  examples:
> >    - |
> > +    refclk: refclk-25mhz {
> > +        compatible = "fixed-clock";
> > +        #clock-cells = <0>;
> > +        clock-frequency = <25000000>;
> > +    };
>
> Examples don't need to show providers.
>
> > +
> >      ahb {
> >          #address-cells = <2>;
> >          #size-cells = <2>;
> > --
> > 2.34.1
> >

Best regards,

Tomer
Krzysztof Kozlowski Jan. 10, 2024, 8:54 p.m. UTC | #5
On 10/01/2024 14:47, Tomer Maimon wrote:
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: refclk
>>> +
>>>    '#clock-cells':
>>>      const: 1
>>>      description:
>>> @@ -30,12 +38,20 @@ properties:
>>>  required:
>>>    - compatible
>>>    - reg
>>> +  - clocks
>>> +  - clock-names
>>
>> New required properties are an ABI break. That's fine if you explain why
>> that's okay in the commit msg.
> What do you mean?

I think it was clear. Which part is not clear?

> Could I add the new required properties to the required list?

You just did, didn't you? And received feedback that you are breaking
the ABI.

Best regards,
Krzysztof
Stephen Boyd Jan. 10, 2024, 9:45 p.m. UTC | #6
Quoting Krzysztof Kozlowski (2024-01-10 12:54:14)
> On 10/01/2024 14:47, Tomer Maimon wrote:
> >>> +
> >>> +  clock-names:
> >>> +    items:
> >>> +      - const: refclk
> >>> +
> >>>    '#clock-cells':
> >>>      const: 1
> >>>      description:
> >>> @@ -30,12 +38,20 @@ properties:
> >>>  required:
> >>>    - compatible
> >>>    - reg
> >>> +  - clocks
> >>> +  - clock-names
> >>
> >> New required properties are an ABI break. That's fine if you explain why
> >> that's okay in the commit msg.
> > What do you mean?
> 
> I think it was clear. Which part is not clear?
> 
> > Could I add the new required properties to the required list?
> 
> You just did, didn't you? And received feedback that you are breaking
> the ABI.
> 

It's fine to break the ABI as long as the commit message explains that
the driver isn't merged yet.
Tomer Maimon Jan. 16, 2024, 3:21 p.m. UTC | #7
Hi Stephen,

On Wed, 10 Jan 2024 at 23:46, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Krzysztof Kozlowski (2024-01-10 12:54:14)
> > On 10/01/2024 14:47, Tomer Maimon wrote:
> > >>> +
> > >>> +  clock-names:
> > >>> +    items:
> > >>> +      - const: refclk
> > >>> +
> > >>>    '#clock-cells':
> > >>>      const: 1
> > >>>      description:
> > >>> @@ -30,12 +38,20 @@ properties:
> > >>>  required:
> > >>>    - compatible
> > >>>    - reg
> > >>> +  - clocks
> > >>> +  - clock-names
> > >>
> > >> New required properties are an ABI break. That's fine if you explain why
> > >> that's okay in the commit msg.
> > > What do you mean?
> >
> > I think it was clear. Which part is not clear?
> >
> > > Could I add the new required properties to the required list?
> >
> > You just did, didn't you? And received feedback that you are breaking
> > the ABI.
> >
>
> It's fine to break the ABI as long as the commit message explains that
> the driver isn't merged yet.

Thanks for your clarification.

Best regards,

Tomer
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
index b901ca13cd25..0b642bfce292 100644
--- a/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
+++ b/Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml
@@ -21,6 +21,14 @@  properties:
   reg:
     maxItems: 1
 
+  clocks:
+    items:
+      - description: 25Mhz referance clock
+
+  clock-names:
+    items:
+      - const: refclk
+
   '#clock-cells':
     const: 1
     description:
@@ -30,12 +38,20 @@  properties:
 required:
   - compatible
   - reg
+  - clocks
+  - clock-names
   - '#clock-cells'
 
 additionalProperties: false
 
 examples:
   - |
+    refclk: refclk-25mhz {
+        compatible = "fixed-clock";
+        #clock-cells = <0>;
+        clock-frequency = <25000000>;
+    }; 
+  
     ahb {
         #address-cells = <2>;
         #size-cells = <2>;