diff mbox series

[v2,3/6] ARM: dts: rockchip: remove interrupts properties from pwm nodes rv1108.dtsi

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

Commit Message

Johan Jonker April 11, 2021, 1:10 p.m. UTC 
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(-)

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
>
diff mbox series

Patch

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";