linux-kernel - Re: [PATCH v1 03/11] dt-bindings: pwm: rockchip: add rockchip,rk3128-pwm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Sep 2022 12:26:37 +0200
From:   Johan Jonker <jbx6244@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Rob Herring <robh@...nel.org>, u.kleine-koenig@...gutronix.de,
        linux-rockchip@...ts.infradead.org, philipp.tomsich@...ll.eu,
        linux-arm-kernel@...ts.infradead.org,
        krzysztof.kozlowski+dt@...aro.org, devicetree@...r.kernel.org,
        linux-pwm@...r.kernel.org, kever.yang@...k-chips.com,
        zhangqing@...k-chips.com, linux-kernel@...r.kernel.org,
        heiko@...ech.de
Subject: Re: [PATCH v1 03/11] dt-bindings: pwm: rockchip: add
 rockchip,rk3128-pwm



On 9/28/22 13:59, Thierry Reding wrote:
> On Tue, Sep 13, 2022 at 04:38:32PM +0200, Johan Jonker wrote:
>>
>>
>> On 9/12/22 18:21, Rob Herring wrote:
>>> On Sat, Sep 10, 2022 at 09:48:04PM +0200, Johan Jonker wrote:
>>>> Reduced CC.
>>>>
>>>> Hi Rob,
>>>>
>>>
>>> Seemed like a simple enough warning to fix...
>>
>> Some examples for comment.
>> Let us know what would be the better solution?
>>
>> ===========================================================================
>>
>> option1:
>>
>> 	combpwm0: combpwm0 {
>> 		compatible = "rockchip,rv1108-combpwm";
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 		#address-cells = <2>;
>> 		#size-cells = <2>;
>>
>> 		pwm0: pwm@...40000 {
>> 			compatible = "rockchip,rv1108-pwm";
>> 			reg = <0x20040000 0x10>;
>> 		};
>>
>> 		pwm1: pwm@...40010 {
>> 			compatible = "rockchip,rv1108-pwm";
>> 			reg = <0x20040010 0x10>;
>> 		};
>>
>> 		pwm2: pwm@...40020 {
>> 			compatible = "rockchip,rv1108-pwm";
>> 			reg = <0x20040020 0x10>;
>> 		};
>>
>> 		pwm3: pwm@...40030 {
>> 			compatible = "rockchip,rv1108-pwm";
>> 			reg = <0x20040030 0x10>;
>> 		};
>> 	};
>>
>> PRO:
>> - Existing driver might still work.
>> CON:
>> - New compatible needed to service the combined interrupts.
>> - Driver change needed.
>>
>> ===========================================================================
>> option 2:
>>
>> 	combpwm0: pwm@...80000 {
>> 		compatible = "rockchip,rv1108-pwm";
>> 		reg = <0x10280000 0x40>;
>> 		interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		pwm4: pwm-4@0 {
>> 			reg = <0x0>;
>> 		};
>>
>> 		pwm5: pwm-5@10 {
>> 			reg = <0x10>;
>> 		};
>>
>> 		pwm6: pwm-6@20 {
>> 			reg = <0x20>;
>> 		};
>>
>> 		pwm7: pwm-7@30 {
>> 			reg = <0x30>;
>> 		};
>> 	};
>>
>> CON:
>> - Driver change needed.
>> - Not compatible with current drivers.
>>
>> ===========================================================================
>>
>> Current situation:
>>
>> 	pwm0: pwm@...40000 {
>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>> 		reg = <0x20040000 0x10>;
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> 	pwm1: pwm@...40010 {
>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>> 		reg = <0x20040010 0x10>;
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> 	pwm2: pwm@...40020 {
>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>> 		reg = <0x20040020 0x10>;
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> 	pwm3: pwm@...40030 {
>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>> 		reg = <0x20040030 0x10>;
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> CON:
>> - The property "interrupts 39" can only be claimed ones by one probe function at the time.
>> - Has a fall-back string for rk3288, but unknown identical behavior for interrupts ???
> 

> To be honest, all three descriptions look wrong to me. From the above it
> looks like this is simply one PWM controller with four channels, so it
> should really be described as such, i.e.:
> 
> 	pwm@...40030 {
> 		compatible = "rockchip,rv1108-pwm";
> 		reg = <0x20040030 0x40>;
> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 

Each PWM channel has it's own pinctrl.
Not all channel pins are always in use for PWM exclusively.
Your proposal would not allow pins to be used for other functions.
More ideas with this interrupt? Please advise.

===

The SoCs PWM are configurable to operate in continuous mode (default mainline) or one-shot mode or capture mode.
Is there any good example for one-shot mode interrupt use?


> Looking through existing Rockchip SoC DTSI files, though, it looks like
> this has been done the wrong way since the beginning, so not sure if you
> still want to fix it up.
> 
> This whole problem of dealing with a shared interrupt wouldn't be a
> problem if this was described properly.
> 
> Thierry