diff mbox series

[v6,2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation

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

Commit Message

Billy Tsai June 8, 2023, 2:18 a.m. UTC 
  Document the compatible for aspeed,ast2600-tach device.

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

Comments

Guenter Roeck June 8, 2023, 4:58 a.m. UTC | #1 
On 6/7/23 19:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-tach device.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>   .../bindings/hwmon/aspeed,ast2600-tach.yaml   | 32 +++++++++++++++++++
>   1 file changed, 32 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> new file mode 100644
> index 000000000000..627aa00f2e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Ast2600 Tach controller
> +
> +maintainers:
> +  - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> +  The Aspeed Tach controller can support upto 1 fan input.
> +

The code says:

In Aspeed AST2600 SoC features 16 TACH controllers, with each
controller capable of supporting up to 1 input.

which is a bit different. I guess there are no examples anymore,
but I'd really like to see how this looks like in the devicetree file,
and how the driver is supposed to distinguish/select the 16 inputs.

> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-tach
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - clocks
> +  - resets
> +
> +additionalProperties: false
Krzysztof Kozlowski June 8, 2023, 6:40 a.m. UTC | #2 
On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-tach device.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  .../bindings/hwmon/aspeed,ast2600-tach.yaml   | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> new file mode 100644
> index 000000000000..627aa00f2e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Ast2600 Tach controller
> +
> +maintainers:
> +  - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> +  The Aspeed Tach controller can support upto 1 fan input.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-tach
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1

NAK, not true based on previous discussions. Device does not come with
resets and clocks.


Best regards,
Krzysztof
Guenter Roeck June 8, 2023, 1:18 p.m. UTC | #3 
On 6/7/23 23:21, Billy Tsai wrote:
>          > The code says:
> 
>          > In Aspeed AST2600 SoC features 16 TACH controllers, with each
> 
>          > controller capable of supporting up to 1 input.
> 
>          > which is a bit different. I guess there are no examples anymore,
> 
>          > but I'd really like to see how this looks like in the devicetree file,
> 
>          > and how the driver is supposed to distinguish/select the 16 inputs.
> 
> Hi Roeck,
> 
> The node in the devicetree file will looks like following:
> 
> tach0: tach0@1e610008 {
> 
>          compatible = "aspeed,ast2600-tach";
> 
>          reg = <0x1e610008 0x8>;
> 
>          #address-cells = <1>;
> 
>          #size-cells = <0>;
> 
>          pinctrl-names = "default";
> 
>          pinctrl-0 = <&pinctrl_tach0_default>;
> 
>          clocks = <&syscon ASPEED_CLK_AHB>;
> 
>          resets = <&syscon ASPEED_RESET_PWM>;
> 
>          status = "disabled";
> 
> };
> 

Neither reg nor pinctrl is mentioned in the bindings. Maybe that is not needed nowadays,
but I find it confusing.

Either case, it is highly unusual that there would be 16 instances of this device
instead of one. Why is this done ? It doesn't really make sense to me.

Guenter
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
new file mode 100644
index 000000000000..627aa00f2e92
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
@@ -0,0 +1,32 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 Aspeed, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed Ast2600 Tach controller
+
+maintainers:
+  - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+  The Aspeed Tach controller can support upto 1 fan input.
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-tach
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - resets
+
+additionalProperties: false