diff mbox series

[v12,2/3] dt-bindings: hwmon: Support Aspeed g6 PWM TACH Control

Message ID 20240108074348.735014-3-billy_tsai@aspeedtech.com
State New
Headers show
Series Support pwm/tach driver for aspeed ast26xx | expand

Commit Message

Billy Tsai Jan. 8, 2024, 7:43 a.m. UTC
Document the compatible for aspeed,ast2600-pwm-tach device, which can
support up to 16 PWM outputs and 16 fan tach input.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml

Comments

Rob Herring (Arm) Jan. 13, 2024, 1:55 a.m. UTC | #1
On Mon, Jan 08, 2024 at 03:43:47PM +0800, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-pwm-tach device, which can
> support up to 16 PWM outputs and 16 fan tach input.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> new file mode 100644
> index 000000000000..c615fb10705c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2023 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/aspeed,g6-pwm-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED G6 PWM and Fan Tach controller
> +
> +maintainers:
> +  - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> +  The ASPEED PWM controller can support up to 16 PWM outputs.
> +  The ASPEED Fan Tacho controller can support up to 16 fan tach input.
> +  They are independent hardware blocks, which are different from the
> +  previous version of the ASPEED chip.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-pwm-tach
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 3
> +
> +patternProperties:
> +  "^fan-[0-9]+$":
> +    $ref: fan-common.yaml#
> +    unevaluatedProperties: false
> +    required:
> +      - tach-ch
> +
> +required:
> +  - reg
> +  - clocks
> +  - resets
> +  - "#pwm-cells"
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/aspeed-clock.h>
> +    pwm_tach: pwm-tach-controller@1e610000 {
> +      compatible = "aspeed,ast2600-pwm-tach";
> +      reg = <0x1e610000 0x100>;
> +      clocks = <&syscon ASPEED_CLK_AHB>;
> +      resets = <&syscon ASPEED_RESET_PWM>;
> +      #pwm-cells = <3>;
> +
> +      fan-0 {
> +        tach-ch = /bits/ 8 <0x0>;
> +      };
> +
> +      fan-1 {
> +        tach-ch = /bits/ 8 <0x1 0x2>;
> +      };

NAK on this based on how you are using pwm-fan in v10 discussion. See my 
comments there.

Rob
Billy Tsai Jan. 15, 2024, 7:05 a.m. UTC | #2
> > Document the compatible for aspeed,ast2600-pwm-tach device, which can
> > support up to 16 PWM outputs and 16 fan tach input.
> >
> > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> > ---
> >  .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 69 +++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml b/Documentation/> devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > new file mode 100644
> > index 000000000000..c615fb10705c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (C) 2023 Aspeed, Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/aspeed,g6-pwm-tach.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ASPEED G6 PWM and Fan Tach controller
> > +
> > +maintainers:
> > +  - Billy Tsai <billy_tsai@aspeedtech.com>
> > +
> > +description: |
> > +  The ASPEED PWM controller can support up to 16 PWM outputs.
> > +  The ASPEED Fan Tacho controller can support up to 16 fan tach input.
> > +  They are independent hardware blocks, which are different from the
> > +  previous version of the ASPEED chip.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - aspeed,ast2600-pwm-tach
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  "#pwm-cells":
> > +    const: 3
> > +
> > +patternProperties:
> > +  "^fan-[0-9]+$":
> > +    $ref: fan-common.yaml#
> > +    unevaluatedProperties: false
> > +    required:
> > +      - tach-ch
> > +
> > +required:
> > +  - reg
> > +  - clocks
> > +  - resets
> > +  - "#pwm-cells"
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/aspeed-clock.h>
> > +    pwm_tach: pwm-tach-controller@1e610000 {
> > +      compatible = "aspeed,ast2600-pwm-tach";
> > +      reg = <0x1e610000 0x100>;
> > +      clocks = <&syscon ASPEED_CLK_AHB>;
> > +      resets = <&syscon ASPEED_RESET_PWM>;
> > +      #pwm-cells = <3>;
> > +
> > +      fan-0 {
> > +        tach-ch = /bits/ 8 <0x0>;
> > +      };
> > +
> > +      fan-1 {
> > +        tach-ch = /bits/ 8 <0x1 0x2>;
> > +      };

> NAK on this based on how you are using pwm-fan in v10 discussion. See my
> comments there.

Okay, I will merge everything from the pwm-fan0 node into the fan-0 node
and add the 'simple-bus' to the compatible string of the pwm_tach node.

Thanks

Billy Tsai
Krzysztof Kozlowski Jan. 15, 2024, 7:56 a.m. UTC | #3
On 15/01/2024 08:05, Billy Tsai wrote:
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/aspeed-clock.h>
>>> +    pwm_tach: pwm-tach-controller@1e610000 {
>>> +      compatible = "aspeed,ast2600-pwm-tach";
>>> +      reg = <0x1e610000 0x100>;
>>> +      clocks = <&syscon ASPEED_CLK_AHB>;
>>> +      resets = <&syscon ASPEED_RESET_PWM>;
>>> +      #pwm-cells = <3>;
>>> +
>>> +      fan-0 {
>>> +        tach-ch = /bits/ 8 <0x0>;
>>> +      };
>>> +
>>> +      fan-1 {
>>> +        tach-ch = /bits/ 8 <0x1 0x2>;
>>> +      };
> 
>> NAK on this based on how you are using pwm-fan in v10 discussion. See my
>> comments there.
> 
> Okay, I will merge everything from the pwm-fan0 node into the fan-0 node
> and add the 'simple-bus' to the compatible string of the pwm_tach node.

What simple-bus has anything to do with it? This is not a bus. Just to
remind: we talk about bindings, not driver.

Best regards,
Krzysztof
Billy Tsai Jan. 15, 2024, 8:43 a.m. UTC | #4
> >>> +examples:
> >>> +  - |
> >>> +    #include <dt-bindings/clock/aspeed-clock.h>
> >>> +    pwm_tach: pwm-tach-controller@1e610000 {
> >>> +      compatible = "aspeed,ast2600-pwm-tach";
> >>> +      reg = <0x1e610000 0x100>;
> >>> +      clocks = <&syscon ASPEED_CLK_AHB>;
> >>> +      resets = <&syscon ASPEED_RESET_PWM>;
> >>> +      #pwm-cells = <3>;
> >>> +
> >>> +      fan-0 {
> >>> +        tach-ch = /bits/ 8 <0x0>;
> >>> +      };
> >>> +
> >>> +      fan-1 {
> >>> +        tach-ch = /bits/ 8 <0x1 0x2>;
> >>> +      };
> >
> >> NAK on this based on how you are using pwm-fan in v10 discussion. See my
> >> comments there.
> >
> > Okay, I will merge everything from the pwm-fan0 node into the fan-0 node
> > and add the 'simple-bus' to the compatible string of the pwm_tach node.

> What simple-bus has anything to do with it? This is not a bus. Just to
> remind: we talk about bindings, not driver.

Hi Krzysztof,

If I want to create a dt-binding to indicate that the child nodes
should be treated as platform devices, which will be probed based on the
compatible string, can I add "simple-bus" for our pwm_tach node like the
following?
pwm_tach: pwm-tach-controller@1e610000 {
        compatible = "aspeed,ast2600-pwm-tach", "simple-bus";
        reg = <0x1e610000 0x100>;
        clocks = <&syscon ASPEED_CLK_AHB>;
        resets = <&syscon ASPEED_RESET_PWM>;
        #pwm-cells = <3>;
  
        fan-0 {
          tach-ch = /bits/ 8 <0x0>;
          compatible = "pwm-fan";
          pwms = <&pwm_tach 0 40000 0>;
        };
  
        fan-1 {
          tach-ch = /bits/ 8 <0x1 0x2>;
          compatible = "pwm-fan";
          pwms = <&pwm_tach 1 40000 0>;
        };
      };
Or do you have any other suggestions for describing this in the dt-bindings?

Thanks

Billy Tsai.
Krzysztof Kozlowski Jan. 15, 2024, 9:26 a.m. UTC | #5
On 15/01/2024 09:43, Billy Tsai wrote:
>>>>> +examples:
>>>>> +  - |
>>>>> +    #include <dt-bindings/clock/aspeed-clock.h>
>>>>> +    pwm_tach: pwm-tach-controller@1e610000 {
>>>>> +      compatible = "aspeed,ast2600-pwm-tach";
>>>>> +      reg = <0x1e610000 0x100>;
>>>>> +      clocks = <&syscon ASPEED_CLK_AHB>;
>>>>> +      resets = <&syscon ASPEED_RESET_PWM>;
>>>>> +      #pwm-cells = <3>;
>>>>> +
>>>>> +      fan-0 {
>>>>> +        tach-ch = /bits/ 8 <0x0>;
>>>>> +      };
>>>>> +
>>>>> +      fan-1 {
>>>>> +        tach-ch = /bits/ 8 <0x1 0x2>;
>>>>> +      };
>>>
>>>> NAK on this based on how you are using pwm-fan in v10 discussion. See my
>>>> comments there.
>>>
>>> Okay, I will merge everything from the pwm-fan0 node into the fan-0 node
>>> and add the 'simple-bus' to the compatible string of the pwm_tach node.
> 
>> What simple-bus has anything to do with it? This is not a bus. Just to
>> remind: we talk about bindings, not driver.
> 
> Hi Krzysztof,
> 
> If I want to create a dt-binding to indicate that the child nodes
> should be treated as platform devices, which will be probed based on the

probed? Bindings do not probe. You ignored:
"we talk about bindings, not driver."

> compatible string, can I add "simple-bus" for our pwm_tach node like the
> following?

No, because this is not a bus.

> pwm_tach: pwm-tach-controller@1e610000 {
>         compatible = "aspeed,ast2600-pwm-tach", "simple-bus";
>         reg = <0x1e610000 0x100>;
>         clocks = <&syscon ASPEED_CLK_AHB>;
>         resets = <&syscon ASPEED_RESET_PWM>;
>         #pwm-cells = <3>;
>   
>         fan-0 {
>           tach-ch = /bits/ 8 <0x0>;
>           compatible = "pwm-fan";
>           pwms = <&pwm_tach 0 40000 0>;
>         };
>   
>         fan-1 {
>           tach-ch = /bits/ 8 <0x1 0x2>;
>           compatible = "pwm-fan";
>           pwms = <&pwm_tach 1 40000 0>;
>         };
>       };
> Or do you have any other suggestions for describing this in the dt-bindings?


There is no need to describe it in the bindings. The existing compatible
describes it sufficiently. Your pwms now duplicate the tach-ch... I
don't understand what you want to achieve here in terms of hardware
description (again, please steer away from talking about Linux drivers
and probing, it's not related).

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
new file mode 100644
index 000000000000..c615fb10705c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
@@ -0,0 +1,69 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2023 Aspeed, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/aspeed,g6-pwm-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED G6 PWM and Fan Tach controller
+
+maintainers:
+  - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+  The ASPEED PWM controller can support up to 16 PWM outputs.
+  The ASPEED Fan Tacho controller can support up to 16 fan tach input.
+  They are independent hardware blocks, which are different from the
+  previous version of the ASPEED chip.
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-pwm-tach
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+patternProperties:
+  "^fan-[0-9]+$":
+    $ref: fan-common.yaml#
+    unevaluatedProperties: false
+    required:
+      - tach-ch
+
+required:
+  - reg
+  - clocks
+  - resets
+  - "#pwm-cells"
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/aspeed-clock.h>
+    pwm_tach: pwm-tach-controller@1e610000 {
+      compatible = "aspeed,ast2600-pwm-tach";
+      reg = <0x1e610000 0x100>;
+      clocks = <&syscon ASPEED_CLK_AHB>;
+      resets = <&syscon ASPEED_RESET_PWM>;
+      #pwm-cells = <3>;
+
+      fan-0 {
+        tach-ch = /bits/ 8 <0x0>;
+      };
+
+      fan-1 {
+        tach-ch = /bits/ 8 <0x1 0x2>;
+      };
+    };