diff mbox series

[v2,1/2] dt-bindings: Add bindings for aspeed pwm-tach and pwm.

Message ID 20210414104939.1093-2-billy_tsai@aspeedtech.com
State New
Headers show
Series Support pwm driver for aspeed ast26xx | expand

Commit Message

Billy Tsai April 14, 2021, 10:49 a.m. UTC 
This patch adds device bindings for aspeed pwm-tach device which is a
multi-function device include pwn and tach function and pwm device which
should be the sub-node of pwm-tach device.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Change-Id: I18d9dea14c3a04e1b7e38ffecd49d45917b9b545
---
 .../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 60 +++++++++++++++++++
 .../bindings/pwm/aspeed,ast2600-pwm.yaml      | 44 ++++++++++++++
 2 files changed, 104 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
 create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml

Comments

Rob Herring (Arm) April 14, 2021, 2:50 p.m. UTC | #1 
On Wed, 14 Apr 2021 18:49:38 +0800, Billy Tsai wrote:
> This patch adds device bindings for aspeed pwm-tach device which is a
> multi-function device include pwn and tach function and pwm device which
> should be the sub-node of pwm-tach device.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Change-Id: I18d9dea14c3a04e1b7e38ffecd49d45917b9b545
> ---
>  .../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 60 +++++++++++++++++++
>  .../bindings/pwm/aspeed,ast2600-pwm.yaml      | 44 ++++++++++++++
>  2 files changed, 104 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
>  create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> 

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:
Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.example.dt.yaml:0:0: /example-0/pwm_tach@1e610000/tach@1: failed to match any schema with compatible: ['aspeed,ast2600-tach']

See https://patchwork.ozlabs.org/patch/1466127

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.
Rob Herring (Arm) April 14, 2021, 10:15 p.m. UTC | #2 
On Wed, Apr 14, 2021 at 06:49:38PM +0800, Billy Tsai wrote:
> This patch adds device bindings for aspeed pwm-tach device which is a
> multi-function device include pwn and tach function and pwm device which
> should be the sub-node of pwm-tach device.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Change-Id: I18d9dea14c3a04e1b7e38ffecd49d45917b9b545

Drop

> ---
>  .../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 60 +++++++++++++++++++
>  .../bindings/pwm/aspeed,ast2600-pwm.yaml      | 44 ++++++++++++++
>  2 files changed, 104 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
>  create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
> new file mode 100644
> index 000000000000..eaf8bdf8d44e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 ASPEED, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/aspeed,ast2600-pwm-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: PWM Tach controller Device Tree Bindings
> +
> +description: |
> +  The PWM Tach controller is represented as a multi-function device which
> +  includes:
> +    PWM
> +    Tach

But is it really? A PWM and tach sounds like a fan controller. Look at 
other existing PWM+tach bindings we have for fans.

> +
> +maintainers:
> +  - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - aspeed,ast2600-pwm-tach
> +      - const: syscon
> +      - const: simple-mfd
> +  reg:
> +    maxItems: 1
> +  "#address-cells":
> +    const: 1
> +  "#size-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +additionalProperties:
> +  type: object

As you know the 2 node names, they should be documented. However, see 
below.

> +
> +examples:
> +  - |
> +    pwm_tach: pwm_tach@1e610000 {
> +      compatible = "aspeed,ast2600-pwm-tach", "syscon", "simple-mfd";
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      reg = <0x1e610000 0x100>;
> +
> +      pwm: pwm@0 {
> +        compatible = "aspeed,ast2600-pwm";
> +        #pwm-cells = <3>;
> +        reg = <0x0 0x100>;
> +      };
> +
> +      tach: tach@1 {
> +        compatible = "aspeed,ast2600-tach";
> +        reg = <0x0 0x100>;

You have 2 nodes at the same address. Not valid.

> +      };

There's no real need for 2 child nodes. The parent node can be a PWM 
provider.

> +    };
> diff --git a/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> new file mode 100644
> index 000000000000..97923e68ccb9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 ASPEED, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/aspeed,ast2600-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED AST2600 PWM controller
> +
> +maintainers:
> +  - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> +  The ASPEED PWM controller can support upto 16 PWM outputs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-pwm
> +
> +  "#pwm-cells":
> +    const: 3
> +
> +  reg:
> +    maxItems: 1
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    // The PWM should be a subnode of a "aspeed,ast2600-pwm-tach" compatible
> +    // node.
> +    pwm_tach: pwm_tach@1e610000 {
> +      compatible = "aspeed,ast2600-pwm-tach", "syscon", "simple-mfd";
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      reg = <0x1e610000 0x100>;
> +
> +      pwm: pwm@0 {
> +        compatible = "aspeed,ast2600-pwm";
> +        #pwm-cells = <3>;
> +        reg = <0x0 0x100>;
> +      };
> +    };
> -- 
> 2.25.1
>
Billy Tsai April 15, 2021, 3:44 a.m. UTC | #3 
Hi Rob,

On 2021/4/15, 6:16 AM,Rob Herringwrote:

    On Wed, Apr 14, 2021 at 06:49:38PM +0800, Billy Tsai wrote:
    >> This patch adds device bindings for aspeed pwm-tach device which is a

    >> multi-function device include pwn and tach function and pwm device which

    >> should be the sub-node of pwm-tach device.

    >> 

    >> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

    >> Change-Id: I18d9dea14c3a04e1b7e38ffecd49d45917b9b545

    >

    >Drop

    >

    >> ---

    >>  .../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 60 +++++++++++++++++++

    >>  .../bindings/pwm/aspeed,ast2600-pwm.yaml      | 44 ++++++++++++++

    >>  2 files changed, 104 insertions(+)

    >>  create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml

    >>  create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml

    >> 

    >> diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml

    >> new file mode 100644

    >> index 000000000000..eaf8bdf8d44e

    >> --- /dev/null

    >> +++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml

    >> @@ -0,0 +1,60 @@

    >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

    >> +# Copyright (C) 2021 ASPEED, Inc.

    >> +%YAML 1.2

    >> +---

    >> +$id: http://devicetree.org/schemas/mfd/aspeed,ast2600-pwm-tach.yaml#

    >> +$schema: http://devicetree.org/meta-schemas/core.yaml#

    >> +

    >> +title: PWM Tach controller Device Tree Bindings

    >> +

    >> +description: |

    >> +  The PWM Tach controller is represented as a multi-function device which

    >> +  includes:

    >> +    PWM

    >> +    Tach


    > But is it really? A PWM and tach sounds like a fan controller. Look at 


Our PWM is not only for fans but also used for the motor, led, buzzer, and so on. 
So I want to split the function into two devices with a multi-function device. 
One for PWM output and one for tach monitor.

    > other existing PWM+tach bindings we have for fans.


I didn't see the PWM+tach bindings can you give some example for me, thanks.

    >> +

    >> +maintainers:

    >> +  - Billy Tsai <billy_tsai@aspeedtech.com>

    >> +

    >> +properties:

    >> +  compatible:

    >> +    items:

    >> +      - enum:

    >> +          - aspeed,ast2600-pwm-tach

    >> +      - const: syscon

    >> +      - const: simple-mfd

    >> +  reg:

    >> +    maxItems: 1

    >> +  "#address-cells":

    >> +    const: 1

    >> +  "#size-cells":

    >> +    const: 1

    >> +

    >> +required:

    >> +  - compatible

    >> +  - reg

    >> +  - "#address-cells"

    >> +  - "#size-cells"

    >> +

    >> +additionalProperties:

    >> +  type: object


    > As you know the 2 node names, they should be documented. However, see 

    > below.


    >> +

    >> +examples:

    >> +  - |

    >> +    pwm_tach: pwm_tach@1e610000 {

    >> +      compatible = "aspeed,ast2600-pwm-tach", "syscon", "simple-mfd";

    >> +      #address-cells = <1>;

    >> +      #size-cells = <1>;

    >> +      reg = <0x1e610000 0x100>;

    >> +

    >> +      pwm: pwm@0 {

    >> +        compatible = "aspeed,ast2600-pwm";

    >> +        #pwm-cells = <3>;

    >> +        reg = <0x0 0x100>;

    >> +      };

    >> +

    >> +      tach: tach@1 {

    >> +        compatible = "aspeed,ast2600-tach";

    >> +        reg = <0x0 0x100>;


    > You have 2 nodes at the same address. Not valid.


Our pwm and tach is used the same base address and the offset is like below:

PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
...

I will remove the reg property from pwm and tach node and remove the "#address-cells" and
"#size-cells" from the parent node.

    >> +      };


    > There's no real need for 2 child nodes. The parent node can be a PWM 

    > provider.


However, In our usage, the parent node is a mfd, not a simple PWM device only. I don't want to
combine the different functions with the one device node.


    >> +    };

    >> diff --git a/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml

    >> new file mode 100644

    >> index 000000000000..97923e68ccb9

    >> --- /dev/null

    >> +++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml

    >> @@ -0,0 +1,44 @@

    >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

    >> +# Copyright (C) 2021 ASPEED, Inc.

    >> +%YAML 1.2

    >> +---

    >> +$id: http://devicetree.org/schemas/pwm/aspeed,ast2600-pwm.yaml#

    >> +$schema: http://devicetree.org/meta-schemas/core.yaml#

    >> +

    >> +title: ASPEED AST2600 PWM controller

    >> +

    >> +maintainers:

    >> +  - Billy Tsai <billy_tsai@aspeedtech.com>

    >> +

    >> +description: |

    >> +  The ASPEED PWM controller can support upto 16 PWM outputs.

    >> +

    >> +properties:

    >> +  compatible:

    >> +    enum:

    >> +      - aspeed,ast2600-pwm

    >> +

    >> +  "#pwm-cells":

    >> +    const: 3

    >> +

    >> +  reg:

    >> +    maxItems: 1

    >> +

    >> +additionalProperties: false

    >> +

    >> +examples:

    >> +  - |

    >> +    // The PWM should be a subnode of a "aspeed,ast2600-pwm-tach" compatible

    >> +    // node.

    >> +    pwm_tach: pwm_tach@1e610000 {

    >> +      compatible = "aspeed,ast2600-pwm-tach", "syscon", "simple-mfd";

    >> +      #address-cells = <1>;

    >> +      #size-cells = <1>;

    >> +      reg = <0x1e610000 0x100>;

    >> +

    >> +      pwm: pwm@0 {

    >> +        compatible = "aspeed,ast2600-pwm";

    >> +        #pwm-cells = <3>;

    >> +        reg = <0x0 0x100>;

    >> +      };

    >> +    };

    >> -- 

    >> 2.25.1

    >>
Rob Herring (Arm) April 15, 2021, 2:43 p.m. UTC | #4 
On Wed, Apr 14, 2021 at 10:44 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote:
>

> Hi Rob,

>

> On 2021/4/15, 6:16 AM,Rob Herringwrote:

>

>     On Wed, Apr 14, 2021 at 06:49:38PM +0800, Billy Tsai wrote:

>     >> This patch adds device bindings for aspeed pwm-tach device which is a

>     >> multi-function device include pwn and tach function and pwm device which

>     >> should be the sub-node of pwm-tach device.

>     >>

>     >> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

>     >> Change-Id: I18d9dea14c3a04e1b7e38ffecd49d45917b9b545

>     >

>     >Drop

>     >

>     >> ---

>     >>  .../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 60 +++++++++++++++++++

>     >>  .../bindings/pwm/aspeed,ast2600-pwm.yaml      | 44 ++++++++++++++

>     >>  2 files changed, 104 insertions(+)

>     >>  create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml

>     >>  create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml

>     >>

>     >> diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml

>     >> new file mode 100644

>     >> index 000000000000..eaf8bdf8d44e

>     >> --- /dev/null

>     >> +++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml

>     >> @@ -0,0 +1,60 @@

>     >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

>     >> +# Copyright (C) 2021 ASPEED, Inc.

>     >> +%YAML 1.2

>     >> +---

>     >> +$id: http://devicetree.org/schemas/mfd/aspeed,ast2600-pwm-tach.yaml#

>     >> +$schema: http://devicetree.org/meta-schemas/core.yaml#

>     >> +

>     >> +title: PWM Tach controller Device Tree Bindings

>     >> +

>     >> +description: |

>     >> +  The PWM Tach controller is represented as a multi-function device which

>     >> +  includes:

>     >> +    PWM

>     >> +    Tach

>

>     > But is it really? A PWM and tach sounds like a fan controller. Look at

>

> Our PWM is not only for fans but also used for the motor, led, buzzer, and so on.

> So I want to split the function into two devices with a multi-function device.

> One for PWM output and one for tach monitor.

>

>     > other existing PWM+tach bindings we have for fans.

>

> I didn't see the PWM+tach bindings can you give some example for me, thanks.


Let me grep 'tach' for you:

Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
Documentation/devicetree/bindings/hwmon/npcm750-pwm-fan.txt

Hey, look at that, there's already one for ASpeed. So the question is
how is the newer h/w different?

>

>     >> +

>     >> +maintainers:

>     >> +  - Billy Tsai <billy_tsai@aspeedtech.com>

>     >> +

>     >> +properties:

>     >> +  compatible:

>     >> +    items:

>     >> +      - enum:

>     >> +          - aspeed,ast2600-pwm-tach

>     >> +      - const: syscon

>     >> +      - const: simple-mfd

>     >> +  reg:

>     >> +    maxItems: 1

>     >> +  "#address-cells":

>     >> +    const: 1

>     >> +  "#size-cells":

>     >> +    const: 1

>     >> +

>     >> +required:

>     >> +  - compatible

>     >> +  - reg

>     >> +  - "#address-cells"

>     >> +  - "#size-cells"

>     >> +

>     >> +additionalProperties:

>     >> +  type: object

>

>     > As you know the 2 node names, they should be documented. However, see

>     > below.

>

>     >> +

>     >> +examples:

>     >> +  - |

>     >> +    pwm_tach: pwm_tach@1e610000 {

>     >> +      compatible = "aspeed,ast2600-pwm-tach", "syscon", "simple-mfd";

>     >> +      #address-cells = <1>;

>     >> +      #size-cells = <1>;

>     >> +      reg = <0x1e610000 0x100>;

>     >> +

>     >> +      pwm: pwm@0 {

>     >> +        compatible = "aspeed,ast2600-pwm";

>     >> +        #pwm-cells = <3>;

>     >> +        reg = <0x0 0x100>;

>     >> +      };

>     >> +

>     >> +      tach: tach@1 {

>     >> +        compatible = "aspeed,ast2600-tach";

>     >> +        reg = <0x0 0x100>;

>

>     > You have 2 nodes at the same address. Not valid.

>

> Our pwm and tach is used the same base address and the offset is like below:

>

> PWM0 used 0x0 0x4, Tach0 used 0x8 0xc

> PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c

> ...

>

> I will remove the reg property from pwm and tach node and remove the "#address-cells" and

> "#size-cells" from the parent node.


That's not really the solution...

>

>     >> +      };

>

>     > There's no real need for 2 child nodes. The parent node can be a PWM

>     > provider.

>

> However, In our usage, the parent node is a mfd, not a simple PWM device only. I don't want to

> combine the different functions with the one device node.


Looks like a single h/w block to me. If you want to divide that up
into multiple drivers, then that's an OS problem. A single node can be
multiple providers. For example, on the existing aspeed binding, just
add '#pwm-cells' to the node if you want to also expose it as a PWM
provider.

Rob
Billy Tsai April 16, 2021, 8:44 a.m. UTC | #5 
On 2021/4/15, 10:44 PM,Rob Herringwrote:

    >On Wed, Apr 14, 2021 at 10:44 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote:

    >>

    >> Hi Rob,

    >>

    >> On 2021/4/15, 6:16 AM,Rob Herringwrote:

    >>

    >>     On Wed, Apr 14, 2021 at 06:49:38PM +0800, Billy Tsai wrote:

    >>     >> This patch adds device bindings for aspeed pwm-tach device which is a

    >>     >> multi-function device include pwn and tach function and pwm device which

    >>     >> should be the sub-node of pwm-tach device.

    >>     >>

    >>     >> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

    >>     >> Change-Id: I18d9dea14c3a04e1b7e38ffecd49d45917b9b545

    >>     >

    >>     >Drop

    >>     >

    >>     >> ---

    >>     >>  .../bindings/mfd/aspeed,ast2600-pwm-tach.yaml | 60 +++++++++++++++++++

    >>     >>  .../bindings/pwm/aspeed,ast2600-pwm.yaml      | 44 ++++++++++++++

    >>     >>  2 files changed, 104 insertions(+)

    >>     >>  create mode 100644 Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml

    >>     >>  create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml

    >>     >>

    >>     >> diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml

    >>     >> new file mode 100644

    >>     >> index 000000000000..eaf8bdf8d44e

    >>     >> --- /dev/null

    >>     >> +++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml

    >>     >> @@ -0,0 +1,60 @@

    >>     >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

    >>     >> +# Copyright (C) 2021 ASPEED, Inc.

    >>     >> +%YAML 1.2

    >>     >> +---

    >>     >> +$id: http://devicetree.org/schemas/mfd/aspeed,ast2600-pwm-tach.yaml#

    >>     >> +$schema: http://devicetree.org/meta-schemas/core.yaml#

    >>     >> +

    >>     >> +title: PWM Tach controller Device Tree Bindings

    >>     >> +

    >>     >> +description: |

    >>     >> +  The PWM Tach controller is represented as a multi-function device which

    >>     >> +  includes:

    >>     >> +    PWM

    >>     >> +    Tach

    >>

    >>     > But is it really? A PWM and tach sounds like a fan controller. Look at

    >>

    >> Our PWM is not only for fans but also used for the motor, led, buzzer, and so on.

    >> So I want to split the function into two devices with a multi-function device.

    >> One for PWM output and one for tach monitor.

    >>

    >>     > other existing PWM+tach bindings we have for fans.

    >>

    >> I didn't see the PWM+tach bindings can you give some example for me, thanks.

    >

    >Let me grep 'tach' for you:

    >

    >Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt

    >Documentation/devicetree/bindings/hwmon/npcm750-pwm-fan.txt

    >

    >Hey, look at that, there's already one for ASpeed. So the question is

    >how is the newer h/w different?

    >>

    >>     >> +

    >>     >> +maintainers:

    >>     >> +  - Billy Tsai <billy_tsai@aspeedtech.com>

    >>     >> +

    >>     >> +properties:

    >>     >> +  compatible:

    >>     >> +    items:

    >>     >> +      - enum:

    >>     >> +          - aspeed,ast2600-pwm-tach

    >>     >> +      - const: syscon

    >>     >> +      - const: simple-mfd

    >>     >> +  reg:

    >>     >> +    maxItems: 1

    >>     >> +  "#address-cells":

    >>     >> +    const: 1

    >>     >> +  "#size-cells":

    >>     >> +    const: 1

    >>     >> +

    >>     >> +required:

    >>     >> +  - compatible

    >>     >> +  - reg

    >>     >> +  - "#address-cells"

    >>     >> +  - "#size-cells"

    >>     >> +

    >>     >> +additionalProperties:

    >>     >> +  type: object

    >>

    >>     > As you know the 2 node names, they should be documented. However, see

    >>     > below.

    >>

    >>     >> +

    >>     >> +examples:

    >>     >> +  - |

    >>     >> +    pwm_tach: pwm_tach@1e610000 {

    >>     >> +      compatible = "aspeed,ast2600-pwm-tach", "syscon", "simple-mfd";

    >>     >> +      #address-cells = <1>;

    >>     >> +      #size-cells = <1>;

    >>     >> +      reg = <0x1e610000 0x100>;

    >>     >> +

    >>     >> +      pwm: pwm@0 {

    >>     >> +        compatible = "aspeed,ast2600-pwm";

    >>     >> +        #pwm-cells = <3>;

    >>     >> +        reg = <0x0 0x100>;

    >>     >> +      };

    >>     >> +

    >>     >> +      tach: tach@1 {

    >>     >> +        compatible = "aspeed,ast2600-tach";

    >>     >> +        reg = <0x0 0x100>;

    >>

    >>     > You have 2 nodes at the same address. Not valid.

    >>

    >> Our pwm and tach is used the same base address and the offset is like below:

    >>

    >> PWM0 used 0x0 0x4, Tach0 used 0x8 0xc

    >> PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c

    >> ...

    >>

    >> I will remove the reg property from pwm and tach node and remove the "#address-cells" and

    >> "#size-cells" from the parent node.

    >

    >That's not really the solution...

    >

    >>

    >>     >> +      };

    >>

    >>     > There's no real need for 2 child nodes. The parent node can be a PWM

    >>     > provider.

    >>

    >> However, In our usage, the parent node is a mfd, not a simple PWM device only. I don't want to

    >> combine the different functions with the one device node.

    >

    >Looks like a single h/w block to me. If you want to divide that up

    >into multiple drivers, then that's an OS problem. A single node can be

    >multiple providers. For example, on the existing aspeed binding, just

    >add '#pwm-cells' to the node if you want to also expose it as a PWM

    >provider.

    >

    >Rob


I don't think the pwm/tach should be a single h/w block. 
See the kontron,sl28cpld.yaml:
hwmon@b {
        compatible = "kontron,sl28cpld-fan";
        reg = <0xb>;
};

pwm@c {
        compatible = "kontron,sl28cpld-pwm";
        reg = <0xc>;
        #pwm-cells = <2>;
};
They can be the different function from the dts.

Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
Documentation/devicetree/bindings/hwmon/npcm750-pwm-fan.txt

These two devices are specific to the used for fan like
Documentation/devicetree/bindings/hwmon/pwm-fan.txt, but with our usage,
it doesn't just for the fan. In our scenario, the customer wants to
create a pwm-beeper, so he needs one pwm input and he doesn't need the
tach function. If I used the pwm-fan as the beeper's pwm input, it look
weird.
Therefore, I want to split these two functions into different devices.

If my thinking is wrong, please let me know.

Very appreciate.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
new file mode 100644
index 000000000000..eaf8bdf8d44e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2600-pwm-tach.yaml
@@ -0,0 +1,60 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 ASPEED, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/aspeed,ast2600-pwm-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PWM Tach controller Device Tree Bindings
+
+description: |
+  The PWM Tach controller is represented as a multi-function device which
+  includes:
+    PWM
+    Tach
+
+maintainers:
+  - Billy Tsai <billy_tsai@aspeedtech.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - aspeed,ast2600-pwm-tach
+      - const: syscon
+      - const: simple-mfd
+  reg:
+    maxItems: 1
+  "#address-cells":
+    const: 1
+  "#size-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties:
+  type: object
+
+examples:
+  - |
+    pwm_tach: pwm_tach@1e610000 {
+      compatible = "aspeed,ast2600-pwm-tach", "syscon", "simple-mfd";
+      #address-cells = <1>;
+      #size-cells = <1>;
+      reg = <0x1e610000 0x100>;
+
+      pwm: pwm@0 {
+        compatible = "aspeed,ast2600-pwm";
+        #pwm-cells = <3>;
+        reg = <0x0 0x100>;
+      };
+
+      tach: tach@1 {
+        compatible = "aspeed,ast2600-tach";
+        reg = <0x0 0x100>;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
new file mode 100644
index 000000000000..97923e68ccb9
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
@@ -0,0 +1,44 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 ASPEED, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/aspeed,ast2600-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED AST2600 PWM controller
+
+maintainers:
+  - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+  The ASPEED PWM controller can support upto 16 PWM outputs.
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-pwm
+
+  "#pwm-cells":
+    const: 3
+
+  reg:
+    maxItems: 1
+
+additionalProperties: false
+
+examples:
+  - |
+    // The PWM should be a subnode of a "aspeed,ast2600-pwm-tach" compatible
+    // node.
+    pwm_tach: pwm_tach@1e610000 {
+      compatible = "aspeed,ast2600-pwm-tach", "syscon", "simple-mfd";
+      #address-cells = <1>;
+      #size-cells = <1>;
+      reg = <0x1e610000 0x100>;
+
+      pwm: pwm@0 {
+        compatible = "aspeed,ast2600-pwm";
+        #pwm-cells = <3>;
+        reg = <0x0 0x100>;
+      };
+    };