diff mbox series

[v2,1/6] dt-bindings: pwm: convert pwm-rockchip.txt to YAML

Message ID 20210411131007.21757-1-jbx6244@gmail.com
State Superseded
Headers show
Series [v2,1/6] dt-bindings: pwm: convert pwm-rockchip.txt to YAML | expand

Commit Message

Johan Jonker April 11, 2021, 1:10 p.m. UTC
Current dts files with 'pwm' nodes are manually verified.
In order to automate this process pwm-rockchip.txt
has to be converted to yaml.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
Changed V2:
  changed schema for clocks and clock-names
---
 .../devicetree/bindings/pwm/pwm-rockchip.txt       | 27 -------
 .../devicetree/bindings/pwm/pwm-rockchip.yaml      | 91 ++++++++++++++++++++++
 2 files changed, 91 insertions(+), 27 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml

Comments

Chen-Yu Tsai April 12, 2021, 3:15 a.m. UTC | #1
On Sun, Apr 11, 2021 at 9:11 PM Johan Jonker <jbx6244@gmail.com> wrote:
>

> A test with the command below gives this error:

>

> /arch/arm/boot/dts/rv1108-evb.dt.yaml:

> pwm@10280000: 'interrupts' does not match any of the regexes:

> 'pinctrl-[0-9]+'

>

> "interrupts" is an undocumented property, so remove them

> from pwm nodes in rv1108.dtsi.

>

> make ARCH=arm dtbs_check

> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml

>

> Signed-off-by: Johan Jonker <jbx6244@gmail.com>


Given that the interrupts were specified, meaning they are wired up in hardware,
shouldn't the solution be to add the interrupts property to the binding instead?

After all, the device tree describes the actual hardware, not just what the
implementations need.

ChenYu
Johan Jonker April 12, 2021, 10:03 a.m. UTC | #2
On 4/12/21 5:15 AM, Chen-Yu Tsai wrote:
> On Sun, Apr 11, 2021 at 9:11 PM Johan Jonker <jbx6244@gmail.com> wrote:

>>

>> A test with the command below gives this error:

>>

>> /arch/arm/boot/dts/rv1108-evb.dt.yaml:

>> pwm@10280000: 'interrupts' does not match any of the regexes:

>> 'pinctrl-[0-9]+'

>>

>> "interrupts" is an undocumented property, so remove them

>> from pwm nodes in rv1108.dtsi.

>>

>> make ARCH=arm dtbs_check

>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml

>>

>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>

> 

> Given that the interrupts were specified, meaning they are wired up in hardware,

> shouldn't the solution be to add the interrupts property to the binding instead?

> 

> After all, the device tree describes the actual hardware, not just what the

> implementations need.

> 

> ChenYu

> 


Hi,

The question of what to do with it was asked in version 1, but no answer
was given, so I made a proposal.
The device tree description should be complete, but also as lean as
possible. If someone manages to sneak in undocumented properties without
reason then the ultimate consequence should be removal I think.

Not sure about the (missing?) rv1108 TRM, but for rk3328 the interrupt
is used for:

PWM_INTSTS 0x0040 W 0x00000000 Interrupt Status Register
  Channel Interrupt Polarity Flag
    This bit is used in capture mode in order to identify the
    transition of the input waveform when interrupt is generated.
  Channel Interrupt Status
    Interrupt generated

PWM_INT_EN 0x0044 W 0x00000000 Interrupt Enable Register
  Channel Interrupt Enable

Is there any current realistic use/setup for it to convince rob+dt this
should be added to pwm-rockchip.yaml?

The rk3328 interrupt rkpwm_int seems shared between channels, but only
included to pwm3. What is the proper way for that?

Johan
Chen-Yu Tsai April 12, 2021, 10:33 a.m. UTC | #3
On Mon, Apr 12, 2021 at 6:03 PM Johan Jonker <jbx6244@gmail.com> wrote:
>

> On 4/12/21 5:15 AM, Chen-Yu Tsai wrote:

> > On Sun, Apr 11, 2021 at 9:11 PM Johan Jonker <jbx6244@gmail.com> wrote:

> >>

> >> A test with the command below gives this error:

> >>

> >> /arch/arm/boot/dts/rv1108-evb.dt.yaml:

> >> pwm@10280000: 'interrupts' does not match any of the regexes:

> >> 'pinctrl-[0-9]+'

> >>

> >> "interrupts" is an undocumented property, so remove them

> >> from pwm nodes in rv1108.dtsi.

> >>

> >> make ARCH=arm dtbs_check

> >> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml

> >>

> >> Signed-off-by: Johan Jonker <jbx6244@gmail.com>

> >

> > Given that the interrupts were specified, meaning they are wired up in hardware,

> > shouldn't the solution be to add the interrupts property to the binding instead?

> >

> > After all, the device tree describes the actual hardware, not just what the

> > implementations need.

> >

> > ChenYu

> >

>

> Hi,

>

> The question of what to do with it was asked in version 1, but no answer

> was given, so I made a proposal.

> The device tree description should be complete, but also as lean as

> possible. If someone manages to sneak in undocumented properties without

> reason then the ultimate consequence should be removal I think.

>

> Not sure about the (missing?) rv1108 TRM, but for rk3328 the interrupt

> is used for:

>

> PWM_INTSTS 0x0040 W 0x00000000 Interrupt Status Register

>   Channel Interrupt Polarity Flag

>     This bit is used in capture mode in order to identify the

>     transition of the input waveform when interrupt is generated.

>   Channel Interrupt Status

>     Interrupt generated

>

> PWM_INT_EN 0x0044 W 0x00000000 Interrupt Enable Register

>   Channel Interrupt Enable

>

> Is there any current realistic use/setup for it to convince rob+dt this

> should be added to pwm-rockchip.yaml?


Well, the PWM core has capture support, and pwm-sti implements it with
interrupt support, so I guess there's at least a legitimate case for
adding that to the binding. Whether someone has an actual use case for
it and adds code to implement it is another story.

> The rk3328 interrupt rkpwm_int seems shared between channels, but only

> included to pwm3. What is the proper way for that?


I guess the bigger question is why was the PWM controller split into
four device nodes, instead of just one encompassing the whole block.
Now we'd have to introduce a new binding to support capture mode and
interrupts.

In that case I agree with dropping the interrupts for now, as it just
won't fit. But I would add this additional information to the commit
message.


Regards
ChenYu
Johan Jonker April 12, 2021, 11:04 a.m. UTC | #4
On 4/12/21 12:33 PM, Chen-Yu Tsai wrote:
> On Mon, Apr 12, 2021 at 6:03 PM Johan Jonker <jbx6244@gmail.com> wrote:

>>

>> On 4/12/21 5:15 AM, Chen-Yu Tsai wrote:

>>> On Sun, Apr 11, 2021 at 9:11 PM Johan Jonker <jbx6244@gmail.com> wrote:

>>>>

>>>> A test with the command below gives this error:

>>>>

>>>> /arch/arm/boot/dts/rv1108-evb.dt.yaml:

>>>> pwm@10280000: 'interrupts' does not match any of the regexes:

>>>> 'pinctrl-[0-9]+'

>>>>

>>>> "interrupts" is an undocumented property, so remove them

>>>> from pwm nodes in rv1108.dtsi.

>>>>

>>>> make ARCH=arm dtbs_check

>>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml

>>>>

>>>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>

>>>

>>> Given that the interrupts were specified, meaning they are wired up in hardware,

>>> shouldn't the solution be to add the interrupts property to the binding instead?

>>>

>>> After all, the device tree describes the actual hardware, not just what the

>>> implementations need.

>>>

>>> ChenYu

>>>

>>

>> Hi,

>>

>> The question of what to do with it was asked in version 1, but no answer

>> was given, so I made a proposal.

>> The device tree description should be complete, but also as lean as

>> possible. If someone manages to sneak in undocumented properties without

>> reason then the ultimate consequence should be removal I think.

>>

>> Not sure about the (missing?) rv1108 TRM, but for rk3328 the interrupt

>> is used for:

>>

>> PWM_INTSTS 0x0040 W 0x00000000 Interrupt Status Register

>>   Channel Interrupt Polarity Flag

>>     This bit is used in capture mode in order to identify the

>>     transition of the input waveform when interrupt is generated.

>>   Channel Interrupt Status

>>     Interrupt generated

>>

>> PWM_INT_EN 0x0044 W 0x00000000 Interrupt Enable Register

>>   Channel Interrupt Enable

>>

>> Is there any current realistic use/setup for it to convince rob+dt this

>> should be added to pwm-rockchip.yaml?


Found:
pwm3 combined with ir uses a irq. Keep that as it is for now.

https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/input/remotectl/rockchip_pwm_remotectl.c

> 

> Well, the PWM core has capture support, and pwm-sti implements it with

> interrupt support, so I guess there's at least a legitimate case for

> adding that to the binding. Whether someone has an actual use case for

> it and adds code to implement it is another story.

> 

>> The rk3328 interrupt rkpwm_int seems shared between channels, but only

>> included to pwm3. What is the proper way for that?

> 

> I guess the bigger question is why was the PWM controller split into

> four device nodes, instead of just one encompassing the whole block.

> Now we'd have to introduce a new binding to support capture mode and

> interrupts.

> 

> In that case I agree with dropping the interrupts for now, as it just

> won't fit. But I would add this additional information to the commit

> message.


Will wait with adding "interrupts" to pwm-rockchip.yaml till someone
makes a solution for the whole block. Convert only current
document/binding to reduce notifications.

For Heiko: patch 3 + 5 can go in the garbage bin:
[PATCH v2 3/6] ARM: dts: rockchip: remove interrupts properties from pwm
nodes rv1108.dtsi
[PATCH v2 5/6] arm64: dts: rockchip: remove interrupts properties from
pwm nodes rk3328.dtsi

Johan

> 

> 

> Regards

> ChenYu

>
Rob Herring (Arm) April 12, 2021, 3:05 p.m. UTC | #5
On Sun, 11 Apr 2021 15:10:03 +0200, Johan Jonker wrote:
> The compatible strings below are already in use in the Rockchip

> dtsi files, but were somehow never added to a document, so add

> 

> "rockchip,rk3328-pwm"

> 

> "rockchip,rk3036-pwm", "rockchip,rk2928-pwm"

> 

> "rockchip,rk3368-pwm", "rockchip,rk3288-pwm"

> "rockchip,rk3399-pwm", "rockchip,rk3288-pwm"

> 

> "rockchip,px30-pwm", "rockchip,rk3328-pwm"

> "rockchip,rk3308-pwm", "rockchip,rk3328-pwm"

> 

> for pwm nodes to pwm-rockchip.yaml.

> 

> Signed-off-by: Johan Jonker <jbx6244@gmail.com>

> ---

> Changed V2:

>   changed schema for clocks and clock-names

> ---

>  Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml | 9 +++++++++

>  1 file changed, 9 insertions(+)

> 



Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.
Johan Jonker April 12, 2021, 5:54 p.m. UTC | #6
Hi,

Sorry, made a little mistake in version 2 with "rockchip,rk3036-pwm",
"rockchip,rk2928-pwm".
Please trash. Will send version 3.
By the change of schema for clocks and clock-names I add
"rockchip,rk3328-pwm" to the "if:", so strictly speaking v1 and (v2) v3
will not be the same.

Johan


On 4/12/21 5:05 PM, Rob Herring wrote:
> On Sun, 11 Apr 2021 15:10:03 +0200, Johan Jonker wrote:

>> The compatible strings below are already in use in the Rockchip

>> dtsi files, but were somehow never added to a document, so add

>>

>> "rockchip,rk3328-pwm"

>>

>> "rockchip,rk3036-pwm", "rockchip,rk2928-pwm"

>>

>> "rockchip,rk3368-pwm", "rockchip,rk3288-pwm"

>> "rockchip,rk3399-pwm", "rockchip,rk3288-pwm"

>>

>> "rockchip,px30-pwm", "rockchip,rk3328-pwm"

>> "rockchip,rk3308-pwm", "rockchip,rk3328-pwm"

>>

>> for pwm nodes to pwm-rockchip.yaml.

>>

>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>

>> ---

>> Changed V2:

>>   changed schema for clocks and clock-names

>> ---

>>  Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml | 9 +++++++++

>>  1 file changed, 9 insertions(+)

>>

> 

> 

> Please add Acked-by/Reviewed-by tags when posting new versions. However,

> there's no need to repost patches *only* to add the tags. The upstream

> maintainer will do that for acks received on the version they apply.

> 

> If a tag was not added on purpose, please state why and what changed.

>
Rob Herring (Arm) April 12, 2021, 8:57 p.m. UTC | #7
On Sun, 11 Apr 2021 15:10:02 +0200, Johan Jonker wrote:
> Current dts files with 'pwm' nodes are manually verified.

> In order to automate this process pwm-rockchip.txt

> has to be converted to yaml.

> 

> Signed-off-by: Johan Jonker <jbx6244@gmail.com>

> ---

> Changed V2:

>   changed schema for clocks and clock-names

> ---

>  .../devicetree/bindings/pwm/pwm-rockchip.txt       | 27 -------

>  .../devicetree/bindings/pwm/pwm-rockchip.yaml      | 91 ++++++++++++++++++++++

>  2 files changed, 91 insertions(+), 27 deletions(-)

>  delete mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.txt

>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml

> 


Reviewed-by: Rob Herring <robh@kernel.org>
Johan Jonker April 12, 2021, 9:46 p.m. UTC | #8
On 4/12/21 10:57 PM, Rob Herring wrote:
> On Sun, 11 Apr 2021 15:10:02 +0200, Johan Jonker wrote:

>> Current dts files with 'pwm' nodes are manually verified.

>> In order to automate this process pwm-rockchip.txt

>> has to be converted to yaml.

>>

>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>

>> ---

>> Changed V2:

>>   changed schema for clocks and clock-names

>> ---

>>  .../devicetree/bindings/pwm/pwm-rockchip.txt       | 27 -------

>>  .../devicetree/bindings/pwm/pwm-rockchip.yaml      | 91 ++++++++++++++++++++++

>>  2 files changed, 91 insertions(+), 27 deletions(-)

>>  delete mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.txt

>>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml

>>

> 

> Reviewed-by: Rob Herring <robh@kernel.org>

> 

Hi

This tags version 2 with a little mistake instead of version 3?
Is that correct?

Johan
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
deleted file mode 100644
index f70956dea..000000000
--- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
+++ /dev/null
@@ -1,27 +0,0 @@ 
-Rockchip PWM controller
-
-Required properties:
- - compatible: should be "rockchip,<name>-pwm"
-   "rockchip,rk2928-pwm": found on RK29XX,RK3066 and RK3188 SoCs
-   "rockchip,rk3288-pwm": found on RK3288 SOC
-   "rockchip,rv1108-pwm", "rockchip,rk3288-pwm": found on RV1108 SoC
-   "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC
- - reg: physical base address and length of the controller's registers
- - clocks: See ../clock/clock-bindings.txt
-   - For older hardware (rk2928, rk3066, rk3188, rk3228, rk3288, rk3399):
-     - There is one clock that's used both to derive the functional clock
-       for the device and as the bus clock.
-   - For newer hardware (rk3328 and future socs): specified by name
-     - "pwm": This is used to derive the functional clock.
-     - "pclk": This is the APB bus clock.
- - #pwm-cells: must be 2 (rk2928) or 3 (rk3288). See pwm.yaml in this directory
-   for a description of the cell format.
-
-Example:
-
-	pwm0: pwm@20030000 {
-		compatible = "rockchip,rk2928-pwm";
-		reg = <0x20030000 0x10>;
-		clocks = <&cru PCLK_PWM01>;
-		#pwm-cells = <2>;
-	};
diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml b/Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml
new file mode 100644
index 000000000..142ce85ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml
@@ -0,0 +1,91 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/pwm-rockchip.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip PWM controller
+
+maintainers:
+  - Heiko Stuebner <heiko@sntech.de>
+
+properties:
+  compatible:
+    oneOf:
+      - const: rockchip,rk2928-pwm
+      - const: rockchip,rk3288-pwm
+      - const: rockchip,vop-pwm
+      - items:
+          - const: rockchip,rk3036-pwm
+          - const: rockchip,rk2928-pwm
+      - items:
+          - enum:
+              - rockchip,rv1108-pwm
+          - const: rockchip,rk3288-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+    maxItems: 2
+
+  clock-names:
+    maxItems: 2
+
+  "#pwm-cells":
+    enum: [2, 3]
+    description:
+      Must be 2 (rk2928) or 3 (rk3288 and later).
+      See pwm.yaml for a description of the cell format.
+
+required:
+  - compatible
+  - reg
+  - "#pwm-cells"
+
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - rockchip,rv1108-pwm
+
+then:
+  properties:
+    clocks:
+      items:
+        - description: Used to derive the functional clock for the device.
+        - description: Used as the APB bus clock.
+
+    clock-names:
+      items:
+        - const: pwm
+        - const: pclk
+
+  required:
+    - clocks
+    - clock-names
+
+else:
+  properties:
+    clocks:
+      maxItems: 1
+      description:
+        Used both to derive the functional clock
+        for the device and as the bus clock.
+
+  required:
+    - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3188-cru-common.h>
+    pwm0: pwm@20030000 {
+      compatible = "rockchip,rk2928-pwm";
+      reg = <0x20030000 0x10>;
+      clocks = <&cru PCLK_PWM01>;
+      #pwm-cells = <2>;
+    };