| Message ID | 20210411131007.21757-1-jbx6244@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/6] dt-bindings: pwm: convert pwm-rockchip.txt to YAML | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 3 warnings, 91 lines checked |
| robh/dt-meta-schema | success | |
| robh/dtbs-check | fail | build log |
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
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
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
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 >
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>
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 --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>; + };
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