| Message ID | 20210411131007.21757-3-jbx6244@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] dt-bindings: pwm: convert pwm-rockchip.txt to YAML | expand |
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 >
diff --git a/arch/arm/boot/dts/rv1108.dtsi b/arch/arm/boot/dts/rv1108.dtsi index 68e2282f7..af033d4c9 100644 --- a/arch/arm/boot/dts/rv1108.dtsi +++ b/arch/arm/boot/dts/rv1108.dtsi @@ -217,7 +217,6 @@ pwm4: pwm@10280000 { compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm"; reg = <0x10280000 0x10>; - interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>; clock-names = "pwm", "pclk"; pinctrl-names = "default"; @@ -229,7 +228,6 @@ pwm5: pwm@10280010 { compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm"; reg = <0x10280010 0x10>; - interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>; clock-names = "pwm", "pclk"; pinctrl-names = "default"; @@ -241,7 +239,6 @@ pwm6: pwm@10280020 { compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm"; reg = <0x10280020 0x10>; - interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>; clock-names = "pwm", "pclk"; pinctrl-names = "default"; @@ -253,7 +250,6 @@ pwm7: pwm@10280030 { compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm"; reg = <0x10280030 0x10>; - interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>; clock-names = "pwm", "pclk"; pinctrl-names = "default"; @@ -391,7 +387,6 @@ pwm0: pwm@20040000 { compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm"; reg = <0x20040000 0x10>; - interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru SCLK_PWM0_PMU>, <&cru PCLK_PWM0_PMU>; clock-names = "pwm", "pclk"; pinctrl-names = "default"; @@ -403,7 +398,6 @@ pwm1: pwm@20040010 { compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm"; reg = <0x20040010 0x10>; - interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru SCLK_PWM0_PMU>, <&cru PCLK_PWM0_PMU>; clock-names = "pwm", "pclk"; pinctrl-names = "default"; @@ -415,7 +409,6 @@ pwm2: pwm@20040020 { compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm"; reg = <0x20040020 0x10>; - interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru SCLK_PWM0_PMU>, <&cru PCLK_PWM0_PMU>; clock-names = "pwm", "pclk"; pinctrl-names = "default"; @@ -427,7 +420,6 @@ pwm3: pwm@20040030 { compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm"; reg = <0x20040030 0x10>; - interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>; clocks = <&cru SCLK_PWM0_PMU>, <&cru PCLK_PWM0_PMU>; clock-names = "pwm", "pclk"; pinctrl-names = "default";
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> --- arch/arm/boot/dts/rv1108.dtsi | 8 -------- 1 file changed, 8 deletions(-)