diff mbox series

[v6,1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation

Message ID 20230608021839.12769-2-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-pwm device.

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

Comments

Krzysztof Kozlowski June 8, 2023, 6:39 a.m. UTC | #1 
On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-pwm 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.

I don't understand why you make the same mistakes, even though I pointed
them out two times already.

I am not going to point third time. Sorry, it's a waste of my time.

NAK.


Best regards,
Krzysztof
Krzysztof Kozlowski June 8, 2023, 6:46 a.m. UTC | #2 
On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-pwm device.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  .../bindings/pwm/aspeed,ast2600-pwm.yaml      | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> 
> 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..a9e040263578
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> @@ -0,0 +1,38 @@
> +# 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 supports up to 1 PWM outputs.

This does not look right.

> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-pwm
> +
> +  "#pwm-cells":
> +    const: 3

3 cells? For one PWM? What are they?

> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - clocks
> +  - resets
> +
> +additionalProperties: false

Missing examples. All bindings need examples.

Best regards,
Krzysztof
Krzysztof Kozlowski June 8, 2023, 7:54 a.m. UTC | #3 
On 08/06/2023 09:47, Billy Tsai wrote:
> 
>   >> +
>   >> +allOf:
>   >> +  - $ref: pwm.yaml#
>   >> +
>   >> +properties:
>   >> +  compatible:
>   >> +    enum:
>   >> +      - aspeed,ast2600-pwm
>   >> +
>   >> +  "#pwm-cells":
>   >> +    const: 3
> 
>   > 3 cells? For one PWM? What are they?
> 
> channel, period and polarity.

Don't cut my responses. You wrote you have one PWM output, so only one
channel. What do you put then in the channel?

I will start NAKing such patches without DTS user. It's like reviewing
fake code for some unknown solution and trying to get from you piece of
answers one by one, because you do not want to share entire part.

Best regards,
Krzysztof
Krzysztof Kozlowski June 8, 2023, 8:36 a.m. UTC | #4 
On 08/06/2023 10:21, Billy Tsai wrote:
>         On 08/06/2023 09:47, Billy Tsai wrote:
>         >>
>         >>   >> +
>         >>   >> +allOf:
>         >>   >> +  - $ref: pwm.yaml#
>         >>   >> +
>         >>   >> +properties:
>         >>   >> +  compatible:
>         >>   >> +    enum:
>         >>   >> +      - aspeed,ast2600-pwm
>         >>   >> +
>         >>   >> +  "#pwm-cells":
>         >>   >> +    const: 3
>         >>
>         >>   > 3 cells? For one PWM? What are they?
>         >>
>         >> channel, period and polarity.
> 
>         > Don't cut my responses. You wrote you have one PWM output, so only one
>         > channel. What do you put then in the channel?
> 
> You need to put 0 in the cell of the channel, the example of the dts usage will like following:

If you always put 0 isn't this a proof that it's wrong?



Best regards,
Krzysztof
Krzysztof Kozlowski June 8, 2023, 8:39 a.m. UTC | #5 
On 08/06/2023 10:21, Billy Tsai wrote:
>         On 08/06/2023 09:47, Billy Tsai wrote:
>         >>
>         >>   >> +
>         >>   >> +allOf:
>         >>   >> +  - $ref: pwm.yaml#
>         >>   >> +
>         >>   >> +properties:
>         >>   >> +  compatible:
>         >>   >> +    enum:
>         >>   >> +      - aspeed,ast2600-pwm
>         >>   >> +
>         >>   >> +  "#pwm-cells":
>         >>   >> +    const: 3
>         >>
>         >>   > 3 cells? For one PWM? What are they?
>         >>
>         >> channel, period and polarity.
> 
>         > Don't cut my responses. You wrote you have one PWM output, so only one
>         > channel. What do you put then in the channel?
> 
> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
> 
> pwm0: pwm0@1e610000 {
>         compatible = "aspeed,ast2600-pwm";
>         reg = <0x1e610000 0x8>;
>         #pwm-cells = <3>;
>         #address-cells = <1>;
>         #size-cells = <0>;
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_pwm0_default>;
>         clocks = <&syscon ASPEED_CLK_AHB>;
>         resets = <&syscon ASPEED_RESET_PWM>;
>         status = "okay";
> };
> 
> pwm1: pwm1@1e610010 {
>         compatible = "aspeed,ast2600-pwm";
>         reg = <0x1e610010 0x8>;
>         #pwm-cells = <3>;
>         #address-cells = <1>;
>         #size-cells = <0>;
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_pwm1_default>;
>         clocks = <&syscon ASPEED_CLK_AHB>;
>         resets = <&syscon ASPEED_RESET_PWM>;
>         status = "okay";

BTW, these are not two PWM devices but one. I don't understand why you
changed previous design into something like this, but this is not
representing your hardware.

Best regards,
Krzysztof
Krzysztof Kozlowski June 8, 2023, 9:26 a.m. UTC | #6 
On 08/06/2023 10:57, Billy Tsai wrote:
> On 08/06/2023 10:21, Billy Tsai wrote:
>         >>         On 08/06/2023 09:47, Billy Tsai wrote:
>         >>         >>
>         >>         >>   >> +
>         >>         >>   >> +allOf:
>         >>         >>   >> +  - $ref: pwm.yaml#
>         >>         >>   >> +
>         >>         >>   >> +properties:
>         >>         >>   >> +  compatible:
>         >>         >>   >> +    enum:
>         >>         >>   >> +      - aspeed,ast2600-pwm
>         >>         >>   >> +
>         >>         >>   >> +  "#pwm-cells":
>         >>         >>   >> +    const: 3
>         >>         >>
>         >>         >>   > 3 cells? For one PWM? What are they?
>         >>         >>
>         >>         >> channel, period and polarity.
>         >>
>         >>         > Don't cut my responses. You wrote you have one PWM output, so only one
>         >>         > channel. What do you put then in the channel?
>         >>
>         >> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
> 
>         > If you always put 0 isn't this a proof that it's wrong?
> 
> No, if your PWM controller only has one pwm output, then it should only be configured as 0.
> This is the usage of the pwm-cells property.
> https://github.com/torvalds/linux/blob/master/drivers/pwm/core.c#L129-L158

This is only when you use generic of_xlate. You do not have to use
generic of_xlate if it does not suite you. Again you speak about the
drivers, but we talk about bindings:

https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/pwm/pwm.txt#L13

"controller specific"

> https://github.com/torvalds/linux/blob/master/include/linux/pwm.h#LL299C20-L299C20
> All of the pwm driver with npwm = 1 will has the same usage.

So it seems many simplified their drivers...

Best regards,
Krzysztof
Krzysztof Kozlowski June 8, 2023, 9:27 a.m. UTC | #7 
On 08/06/2023 11:15, Billy Tsai wrote:
> On 08/06/2023 10:21, Billy Tsai wrote:
>         >>         On 08/06/2023 09:47, Billy Tsai wrote:
>         >>         >>
>         >>         >>   >> +
>         >>         >>   >> +allOf:
>         >>         >>   >> +  - $ref: pwm.yaml#
>         >>         >>   >> +
>         >>         >>   >> +properties:
>         >>         >>   >> +  compatible:
>         >>         >>   >> +    enum:
>         >>         >>   >> +      - aspeed,ast2600-pwm
>         >>         >>   >> +
>         >>         >>   >> +  "#pwm-cells":
>         >>         >>   >> +    const: 3
>         >>         >>
>         >>         >>   > 3 cells? For one PWM? What are they?
>         >>         >>
>         >>         >> channel, period and polarity.
>         >>
>         >>         > Don't cut my responses. You wrote you have one PWM output, so only one
>         >>         > channel. What do you put then in the channel?
>         >>
>         >> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
>         >>
>         >> pwm0: pwm0@1e610000 {
>         >>         compatible = "aspeed,ast2600-pwm";
>         >>         reg = <0x1e610000 0x8>;
>         >>         #pwm-cells = <3>;
>         >>         #address-cells = <1>;
>         >>         #size-cells = <0>;
>         >>         pinctrl-names = "default";
>         >>         pinctrl-0 = <&pinctrl_pwm0_default>;
>         >>         clocks = <&syscon ASPEED_CLK_AHB>;
>         >>         resets = <&syscon ASPEED_RESET_PWM>;
>         >>         status = "okay";
>         >> };
>         >>
>         >> pwm1: pwm1@1e610010 {
>         >>         compatible = "aspeed,ast2600-pwm";
>         >>         reg = <0x1e610010 0x8>;
>         >>         #pwm-cells = <3>;
>         >>         #address-cells = <1>;
>         >>         #size-cells = <0>;
>         >>         pinctrl-names = "default";
>         >>         pinctrl-0 = <&pinctrl_pwm1_default>;
>         >>         clocks = <&syscon ASPEED_CLK_AHB>;
>         >>         resets = <&syscon ASPEED_RESET_PWM>;
>         >>         status = "okay";
> 
>         > BTW, these are not two PWM devices but one. I don't understand why you
>         > changed previous design into something like this, but this is not
>         > representing your hardware.
> 
> The previous design of my patch treated our PWM controller as having 16 PWM channels.
> However, from a hardware perspective, it consists of 16 individual PWM chips, each
> with its own set of two 4-byte control registers. These chips operate independently
> and are not affected by each other.

They are affected by each other - you use the same clock and reset line.
I really doubt you have 16 PWM controllers. Anyway, I cannot judge.
Either your previous submissions were totally bogus or this one is.

Best regards,
Krzysztof
diff mbox series

Patch

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..a9e040263578
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
@@ -0,0 +1,38 @@ 
+# 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 supports up to 1 PWM outputs.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-pwm
+
+  "#pwm-cells":
+    const: 3
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - resets
+
+additionalProperties: false