[v8,01/11] dt-bindings: clock: meson: add A1 PLL clock controller bindings

Commit Message

Dmitry Rokosov Dec. 1, 2022, 10:56 p.m. UTC

From: Jian Hu <[e-mail]>

Add the documentation to support Amlogic A1 PLL clock driver,
and add A1 PLL clock controller bindings.

Signed-off-by: Jian Hu <[e-mail]>
Signed-off-by: Dmitry Rokosov <[e-mail]>
---
 .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
 include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
 2 files changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
 create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h

Comments

Rob Herring (Arm) Dec. 2, 2022, 4:10 a.m. UTC | #1

On Fri, 02 Dec 2022 01:56:53 +0300, Dmitry Rokosov wrote:
> From: Jian Hu <[e-mail]>
> 
> Add the documentation to support Amlogic A1 PLL clock driver,
> and add A1 PLL clock controller bindings.
> 
> Signed-off-by: Jian Hu <[e-mail]>
> Signed-off-by: Dmitry Rokosov <[e-mail]>
> ---
>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>  2 files changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
> 

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:
./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml:26:6: [warning] wrong indentation: expected 6 but found 5 (indentation)

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.example.dtb: pll-clock-controller@7c80: reg: [[0, 31872], [0, 396]] is too long
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[e-mail]

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.

Dmitry Rokosov Dec. 2, 2022, 9:51 a.m. UTC | #2

On Thu, Dec 01, 2022 at 10:10:04PM -0600, Rob Herring wrote:
> 
> On Fri, 02 Dec 2022 01:56:53 +0300, Dmitry Rokosov wrote:
> > From: Jian Hu <[e-mail]>
> > 
> > Add the documentation to support Amlogic A1 PLL clock driver,
> > and add A1 PLL clock controller bindings.
> > 
> > Signed-off-by: Jian Hu <[e-mail]>
> > Signed-off-by: Dmitry Rokosov <[e-mail]>
> > ---
> >  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
> >  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
> >  2 files changed, 68 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> >  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
> > 
> 
> 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:
> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml:26:6: [warning] wrong indentation: expected 6 but found 5 (indentation)
> 
> dtschema/dtc warnings/errors:
> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml: $id: relative path/filename doesn't match actual path or filename
> 	expected: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.example.dtb: pll-clock-controller@7c80: reg: [[0, 31872], [0, 396]] is too long
> 	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[e-mail]
> 
> 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.
> 

Please find all fixes of above warnings and errors in the my patch
located at the link:

https://lore.kernel.org/linux-amlogic/[e-mail]/

Krzysztof Kozlowski Dec. 2, 2022, 10:39 a.m. UTC | #3

On 02/12/2022 10:51, Dmitry Rokosov wrote:
> On Thu, Dec 01, 2022 at 10:10:04PM -0600, Rob Herring wrote:
>>
>> On Fri, 02 Dec 2022 01:56:53 +0300, Dmitry Rokosov wrote:
>>> From: Jian Hu <[e-mail]>
>>>
>>> Add the documentation to support Amlogic A1 PLL clock driver,
>>> and add A1 PLL clock controller bindings.
>>>
>>> Signed-off-by: Jian Hu <[e-mail]>
>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
>>> ---
>>>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
>>>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>>>  2 files changed, 68 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
>>>
>>
>> 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:
>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml:26:6: [warning] wrong indentation: expected 6 but found 5 (indentation)
>>
>> dtschema/dtc warnings/errors:
>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml: $id: relative path/filename doesn't match actual path or filename
>> 	expected: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.example.dtb: pll-clock-controller@7c80: reg: [[0, 31872], [0, 396]] is too long
>> 	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>
>> doc reference errors (make refcheckdocs):
>>
>> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[e-mail]
>>
>> 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.
>>
> 
> Please find all fixes of above warnings and errors in the my patch
> located at the link:
> 
> https://lore.kernel.org/linux-amlogic/[e-mail]/

Why? This patch here is broken and it should be fixed. Don't apply
broken patches...

Best regards,
Krzysztof

Krzysztof Kozlowski Dec. 2, 2022, 10:42 a.m. UTC | #4

On 01/12/2022 23:56, Dmitry Rokosov wrote:
> From: Jian Hu <[e-mail]>
> 
> Add the documentation to support Amlogic A1 PLL clock driver,
> and add A1 PLL clock controller bindings.
> 
> Signed-off-by: Jian Hu <[e-mail]>
> Signed-off-by: Dmitry Rokosov <[e-mail]>
> ---
>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>  2 files changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> new file mode 100644
> index 000000000000..d67250fbeece
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/amlogic,a1-pll-clkc.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

Drop quotes from both.

> +
> +title: Amlogic Meson A/C serials PLL Clock Control Unit Device Tree Bindings

Drop "Device Tree Bindings"

> +
> +maintainers:
> +  - Neil Armstrong <[e-mail]>

Not correct email address.

> +  - Jerome Brunet <[e-mail]>
> +  - Jian Hu <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: amlogic,a1-pll-clkc
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +     - description: input xtal_fixpll
> +     - description: input xtal_hifipll

As pointed already - this patch cannot work and must not be merged.

> +
> +  clock-names:
> +    items:
> +      - const: xtal_fixpll
> +      - const: xtal_hifipll
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clkc_pll: pll-clock-controller@7c80 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +                compatible = "amlogic,a1-pll-clkc";

Use 4 spaces for example indentation.

> +                reg = <0 0x7c80 0 0x18c>;
> +                #clock-cells = <1>;
> +                clocks = <&clkc_periphs 1>,
> +                         <&clkc_periphs 4>;
> +                clock-names = "xtal_fixpll", "xtal_hifipll";
> +    };
> diff --git a/include/dt-bindings/clock/a1-pll-clkc.h b/include/dt-bindings/clock/a1-pll-clkc.h
> new file mode 100644
> index 000000000000..58eae237e503
> --- /dev/null
> +++ b/include/dt-bindings/clock/a1-pll-clkc.h 

Filename matching compatible / bindings file.

> @@ -0,0 +1,16 @@

Best regards,
Krzysztof

Jerome Brunet Dec. 2, 2022, 11:04 a.m. UTC | #5

On Fri 02 Dec 2022 at 11:39, Krzysztof Kozlowski <[e-mail]> wrote:

> On 02/12/2022 10:51, Dmitry Rokosov wrote:
>> On Thu, Dec 01, 2022 at 10:10:04PM -0600, Rob Herring wrote:
>>>
>>> On Fri, 02 Dec 2022 01:56:53 +0300, Dmitry Rokosov wrote:
>>>> From: Jian Hu <[e-mail]>
>>>>
>>>> Add the documentation to support Amlogic A1 PLL clock driver,
>>>> and add A1 PLL clock controller bindings.
>>>>
>>>> Signed-off-by: Jian Hu <[e-mail]>
>>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
>>>> ---
>>>>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
>>>>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>>>>  2 files changed, 68 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>>  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
>>>>
>>>
>>> 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:
>>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml:26:6: [warning] wrong indentation: expected 6 but found 5 (indentation)
>>>
>>> dtschema/dtc warnings/errors:
>>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml: $id: relative path/filename doesn't match actual path or filename
>>> 	expected: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
>>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.example.dtb: pll-clock-controller@7c80: reg: [[0, 31872], [0, 396]] is too long
>>> 	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>
>>> doc reference errors (make refcheckdocs):
>>>
>>> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[e-mail]
>>>
>>> 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.
>>>
>> 
>> Please find all fixes of above warnings and errors in the my patch
>> located at the link:
>> 
>> https://lore.kernel.org/linux-amlogic/[e-mail]/
>
> Why? This patch here is broken and it should be fixed. Don't apply
> broken patches...

Dmitry is ressurecting a series that is several years old and not his to
begin with.

He was unsure about take the code of somebody else.
To be fair, he even asked for advice on IRC about to proceed.

Dmitry, as you may have guessed, for next revision, please fix Jian Hu
original patches in place, not with fixup patches.

If your fixes are minor (not complete rewrite), please keep the original
author and add your SoB

>
> Best regards,
> Krzysztof

Jerome Brunet Dec. 2, 2022, 11:11 a.m. UTC | #6

On Fri 02 Dec 2022 at 01:56, Dmitry Rokosov <[e-mail]> wrote:

> From: Jian Hu <[e-mail]>
>
> Add the documentation to support Amlogic A1 PLL clock driver,
> and add A1 PLL clock controller bindings.
>
> Signed-off-by: Jian Hu <[e-mail]>
> Signed-off-by: Dmitry Rokosov <[e-mail]>
> ---
>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>  2 files changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
>
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> new file mode 100644
> index 000000000000..d67250fbeece
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/amlogic,a1-pll-clkc.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Amlogic Meson A/C serials PLL Clock Control Unit Device Tree Bindings
> +
> +maintainers:
> +  - Neil Armstrong <[e-mail]>
> +  - Jerome Brunet <[e-mail]>
> +  - Jian Hu <[e-mail]>
> +
> +properties:
> +  compatible:
> +    const: amlogic,a1-pll-clkc
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +     - description: input xtal_fixpll
> +     - description: input xtal_hifipll
> +
> +  clock-names:
> +    items:
> +      - const: xtal_fixpll
> +      - const: xtal_hifipll

Do we really need the "xtal_" prefix ?

Seems like the clock is the PLL, not the xtal

> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clkc_pll: pll-clock-controller@7c80 {
> +                compatible = "amlogic,a1-pll-clkc";
> +                reg = <0 0x7c80 0 0x18c>;
> +                #clock-cells = <1>;
> +                clocks = <&clkc_periphs 1>,
> +                         <&clkc_periphs 4>;
> +                clock-names = "xtal_fixpll", "xtal_hifipll";
> +    };
> diff --git a/include/dt-bindings/clock/a1-pll-clkc.h b/include/dt-bindings/clock/a1-pll-clkc.h
> new file mode 100644
> index 000000000000..58eae237e503
> --- /dev/null
> +++ b/include/dt-bindings/clock/a1-pll-clkc.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
> + */
> +
> +#ifndef __A1_PLL_CLKC_H
> +#define __A1_PLL_CLKC_H
> +
> +#define CLKID_FIXED_PLL				1
> +#define CLKID_FCLK_DIV2				6
> +#define CLKID_FCLK_DIV3				7
> +#define CLKID_FCLK_DIV5				8
> +#define CLKID_FCLK_DIV7				9
> +#define CLKID_HIFI_PLL				10
> +
> +#endif /* __A1_PLL_CLKC_H */

Krzysztof Kozlowski Dec. 2, 2022, 11:16 a.m. UTC | #7

On 02/12/2022 12:04, Jerome Brunet wrote:
> 
> On Fri 02 Dec 2022 at 11:39, Krzysztof Kozlowski <[e-mail]> wrote:
> 
>> On 02/12/2022 10:51, Dmitry Rokosov wrote:
>>> On Thu, Dec 01, 2022 at 10:10:04PM -0600, Rob Herring wrote:
>>>>
>>>> On Fri, 02 Dec 2022 01:56:53 +0300, Dmitry Rokosov wrote:
>>>>> From: Jian Hu <[e-mail]>
>>>>>
>>>>> Add the documentation to support Amlogic A1 PLL clock driver,
>>>>> and add A1 PLL clock controller bindings.
>>>>>
>>>>> Signed-off-by: Jian Hu <[e-mail]>
>>>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
>>>>> ---
>>>>>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
>>>>>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>>>>>  2 files changed, 68 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>>>  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
>>>>>
>>>>
>>>> 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:
>>>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml:26:6: [warning] wrong indentation: expected 6 but found 5 (indentation)
>>>>
>>>> dtschema/dtc warnings/errors:
>>>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml: $id: relative path/filename doesn't match actual path or filename
>>>> 	expected: http://devicetree.org/schemas/clock/amlogic,a1-pll-clkc.yaml#
>>>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.example.dtb: pll-clock-controller@7c80: reg: [[0, 31872], [0, 396]] is too long
>>>> 	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>>
>>>> doc reference errors (make refcheckdocs):
>>>>
>>>> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[e-mail]
>>>>
>>>> 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.
>>>>
>>>
>>> Please find all fixes of above warnings and errors in the my patch
>>> located at the link:
>>>
>>> https://lore.kernel.org/linux-amlogic/[e-mail]/
>>
>> Why? This patch here is broken and it should be fixed. Don't apply
>> broken patches...
> 
> Dmitry is ressurecting a series that is several years old and not his to
> begin with.
> 
> He was unsure about take the code of somebody else.
> To be fair, he even asked for advice on IRC about to proceed.
> 
> Dmitry, as you may have guessed, for next revision, please fix Jian Hu
> original patches in place, not with fixup patches.
> 
> If your fixes are minor (not complete rewrite), please keep the original
> author and add your SoB

We never take intentionally wrong patches, e.g. code which does not even
compile, and immediately fix it in next patch.

Can you imagine adding such drivers? Which are broken in the commit they
are added?

So the patchset is old or abandoned, take ownership, add co-developed
etc. Just don't add known broken code.

Best regards,
Krzysztof

Dmitry Rokosov Dec. 2, 2022, 11:18 a.m. UTC | #8

> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Thank you for the link, will change it in the next version.

> 
> > +                compatible = "amlogic,a1-pll-clkc";
> 
> Use 4 spaces for example indentation.

Good point, I've fixed it in the my patches above. But as you mentioned
in the all changes, I have to squash them....

Dmitry Rokosov Dec. 2, 2022, 11:28 a.m. UTC | #9

> >>>> On Fri, 02 Dec 2022 01:56:53 +0300, Dmitry Rokosov wrote:
> >>>>> From: Jian Hu <[e-mail]>
> >>>>>
> >>>>> Add the documentation to support Amlogic A1 PLL clock driver,
> >>>>> and add A1 PLL clock controller bindings.
> >>>>>
> >>>>> Signed-off-by: Jian Hu <[e-mail]>
> >>>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
> >>>>> ---
> >>>>>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
> >>>>>  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
> >>>>>  2 files changed, 68 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> >>>>>  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
> >>>>>
> >>>>
> >>>> 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:
> >>>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml:26:6: [warning] wrong indentation: expected 6 but found 5 (indentation)
> >>>>
> >>>> dtschema/dtc warnings/errors:
> >>>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml: $id: relative path/filename doesn't match actual path or filename

...

> >>>
> >>> Please find all fixes of above warnings and errors in the my patch
> >>> located at the link:
> >>>
> >>> https://lore.kernel.org/linux-amlogic/[e-mail]/
> >>
> >> Why? This patch here is broken and it should be fixed. Don't apply
> >> broken patches...
> > 
> > Dmitry is ressurecting a series that is several years old and not his to
> > begin with.
> > 
> > He was unsure about take the code of somebody else.
> > To be fair, he even asked for advice on IRC about to proceed.
> > 
> > Dmitry, as you may have guessed, for next revision, please fix Jian Hu
> > original patches in place, not with fixup patches.
> > 
> > If your fixes are minor (not complete rewrite), please keep the original
> > author and add your SoB
> 
> We never take intentionally wrong patches, e.g. code which does not even
> compile, and immediately fix it in next patch.
> 
> Can you imagine adding such drivers? Which are broken in the commit they
> are added?
> 
> So the patchset is old or abandoned, take ownership, add co-developed
> etc. Just don't add known broken code.

Okay, I've got your point. It's reasonable.
I will fix Jian Hu's patches (squash with mine) and mark all of them
with co-developed and SoB Jian Hu tags. Thank you for explanation.

Dmitry Rokosov Dec. 2, 2022, 12:26 p.m. UTC | #10

On Fri, Dec 02, 2022 at 12:11:53PM +0100, Jerome Brunet wrote:
> 
> On Fri 02 Dec 2022 at 01:56, Dmitry Rokosov <[e-mail]> wrote:
> 
> > From: Jian Hu <[e-mail]>
> >
> > Add the documentation to support Amlogic A1 PLL clock driver,
> > and add A1 PLL clock controller bindings.
> >
> > Signed-off-by: Jian Hu <[e-mail]>
> > Signed-off-by: Dmitry Rokosov <[e-mail]>
> > ---
> >  .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
> >  include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
> >  2 files changed, 68 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> >  create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
> >
> > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > new file mode 100644
> > index 000000000000..d67250fbeece
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/amlogic,a1-pll-clkc.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Amlogic Meson A/C serials PLL Clock Control Unit Device Tree Bindings
> > +
> > +maintainers:
> > +  - Neil Armstrong <[e-mail]>
> > +  - Jerome Brunet <[e-mail]>
> > +  - Jian Hu <[e-mail]>
> > +
> > +properties:
> > +  compatible:
> > +    const: amlogic,a1-pll-clkc
> > +
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +     - description: input xtal_fixpll
> > +     - description: input xtal_hifipll
> > +
> > +  clock-names:
> > +    items:
> > +      - const: xtal_fixpll
> > +      - const: xtal_hifipll
> 
> Do we really need the "xtal_" prefix ?
> 
> Seems like the clock is the PLL, not the xtal
> 

This name was formed from specification registers description. Register
CLKTREE_SYS_OSCIN_CTRL has "gate en" field which calls "xtal ->
HIFIPLL", therefore if was transformed to xtal_hifipll name.

But I agree with you, that "hifipll" is better name choice.

Neil Armstrong Dec. 2, 2022, 1:36 p.m. UTC | #11

On 02/12/2022 12:28, Dmitry Rokosov wrote:
>>>>>> On Fri, 02 Dec 2022 01:56:53 +0300, Dmitry Rokosov wrote:
>>>>>>> From: Jian Hu <[e-mail]>
>>>>>>>
>>>>>>> Add the documentation to support Amlogic A1 PLL clock driver,
>>>>>>> and add A1 PLL clock controller bindings.
>>>>>>>
>>>>>>> Signed-off-by: Jian Hu <[e-mail]>
>>>>>>> Signed-off-by: Dmitry Rokosov <[e-mail]>
>>>>>>> ---
>>>>>>>   .../bindings/clock/amlogic,a1-pll-clkc.yaml   | 52 +++++++++++++++++++
>>>>>>>   include/dt-bindings/clock/a1-pll-clkc.h       | 16 ++++++
>>>>>>>   2 files changed, 68 insertions(+)
>>>>>>>   create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>>>>>>>   create mode 100644 include/dt-bindings/clock/a1-pll-clkc.h
>>>>>>>
>>>>>>
>>>>>> 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:
>>>>>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml:26:6: [warning] wrong indentation: expected 6 but found 5 (indentation)
>>>>>>
>>>>>> dtschema/dtc warnings/errors:
>>>>>> ./Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml: $id: relative path/filename doesn't match actual path or filename
> 
> ...
> 
>>>>>
>>>>> Please find all fixes of above warnings and errors in the my patch
>>>>> located at the link:
>>>>>
>>>>> https://lore.kernel.org/linux-amlogic/[e-mail]/
>>>>
>>>> Why? This patch here is broken and it should be fixed. Don't apply
>>>> broken patches...
>>>
>>> Dmitry is ressurecting a series that is several years old and not his to
>>> begin with.
>>>
>>> He was unsure about take the code of somebody else.
>>> To be fair, he even asked for advice on IRC about to proceed.
>>>
>>> Dmitry, as you may have guessed, for next revision, please fix Jian Hu
>>> original patches in place, not with fixup patches.
>>>
>>> If your fixes are minor (not complete rewrite), please keep the original
>>> author and add your SoB
>>
>> We never take intentionally wrong patches, e.g. code which does not even
>> compile, and immediately fix it in next patch.
>>
>> Can you imagine adding such drivers? Which are broken in the commit they
>> are added?
>>
>> So the patchset is old or abandoned, take ownership, add co-developed
>> etc. Just don't add known broken code.
> 
> Okay, I've got your point. It's reasonable.
> I will fix Jian Hu's patches (squash with mine) and mark all of them
> with co-developed and SoB Jian Hu tags. Thank you for explanation.

It was clearly explained in the cover-letter, nobody expected these patches
to be applied as-is.

Using RFC would have been the best solution, but this situation is rather
specific and he made the right decisions to trigger this current discussion
toward an acceptable solution for everybody.

Neil

>

Patch

diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
new file mode 100644
index 000000000000..d67250fbeece
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
@@ -0,0 +1,52 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/amlogic,a1-pll-clkc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic Meson A/C serials PLL Clock Control Unit Device Tree Bindings
+
+maintainers:
+  - Neil Armstrong <[e-mail]>
+  - Jerome Brunet <[e-mail]>
+  - Jian Hu <[e-mail]>
+
+properties:
+  compatible:
+    const: amlogic,a1-pll-clkc
+
+  "#clock-cells":
+    const: 1
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+     - description: input xtal_fixpll
+     - description: input xtal_hifipll
+
+  clock-names:
+    items:
+      - const: xtal_fixpll
+      - const: xtal_hifipll
+
+required:
+  - compatible
+  - "#clock-cells"
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    clkc_pll: pll-clock-controller@7c80 {
+                compatible = "amlogic,a1-pll-clkc";
+                reg = <0 0x7c80 0 0x18c>;
+                #clock-cells = <1>;
+                clocks = <&clkc_periphs 1>,
+                         <&clkc_periphs 4>;
+                clock-names = "xtal_fixpll", "xtal_hifipll";
+    };
diff --git a/include/dt-bindings/clock/a1-pll-clkc.h b/include/dt-bindings/clock/a1-pll-clkc.h
new file mode 100644
index 000000000000..58eae237e503
--- /dev/null
+++ b/include/dt-bindings/clock/a1-pll-clkc.h
@@ -0,0 +1,16 @@ 
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ */
+
+#ifndef __A1_PLL_CLKC_H
+#define __A1_PLL_CLKC_H
+
+#define CLKID_FIXED_PLL				1
+#define CLKID_FCLK_DIV2				6
+#define CLKID_FCLK_DIV3				7
+#define CLKID_FCLK_DIV5				8
+#define CLKID_FCLK_DIV7				9
+#define CLKID_HIFI_PLL				10
+
+#endif /* __A1_PLL_CLKC_H */