[PATCH] dt-bindings: pwm: rockchip: Allow "interrupts" prooperty

[Johan Jonker]

On 1/24/24 09:26, Uwe Kleine-König wrote:
> Hello Johan,
>
> On Sun, Jan 07, 2024 at 11:30:14AM +0100, Krzysztof Kozlowski wrote:
>> On 07/01/2024 00:25, Uwe Kleine-König wrote:
>>> On Sat, Jan 06, 2024 at 10:25:10PM +0100, Johan Jonker wrote:
>>>> On 1/6/24 18:10, Krzysztof Kozlowski wrote:
>>>>> On 06/01/2024 15:26, Uwe Kleine-König wrote:
>>>>>> This fixes the dtbs_check error
>>>>>>
>>>>>> 	arch/arm/boot/dts/rockchip/rv1108-elgin-r1.dtb: pwm at 10280030: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
>>>>>> 	from schema $id: http://devicetree.org/schemas/pwm/pwm-rockchip.yaml#
>>>>>>
>>>>>> in several device trees.
>>>>>>
>>>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig at pengutronix.de>
>>>>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org>
>>>> NAK
>>>>
>>>> There's a reason why this isn't implemented before:
>>>>
>>>> [RFC PATCH v1 1/2] dt-bindings: pwm: rockchip: add interrupts property <https://lore.kernel.org/linux-rockchip/ed3df2c8-ffb5-1723-0ed7-3a2721972852@gmail.com/#r>
>>>>
>>>> https://lore.kernel.org/linux-rockchip/ed3df2c8-ffb5-1723-0ed7-3a2721972852@gmail.com/
>>>>
>>>> [PATCH 1/1] dt-bindings: pwm: rockchip: Add description for rk3588 <https://lore.kernel.org/linux-rockchip/20220901135523.52151-1-sebastian.reichel@collabora.com/#r>
>>>>
>>>> https://lore.kernel.org/linux-rockchip/66b5b616-ae9f-a1aa-e2b5-450f570cfcdd@gmail.com/
>>>>
>>>> [PATCH v1 03/11] dt-bindings: pwm: rockchip: add rockchip,rk3128-pwm <https://lore.kernel.org/linux-rockchip/f5dd0ee4-d97e-d878-ffde-c06e9b233e38@gmail.com/>
>>>>
>>>> https://lore.kernel.org/linux-rockchip/946d8ac2-6ff2-093a-ad3c-aa755e00d1dd@arm.com/
>>>>
>>>>
>>>> On how to correctly model the DT with common interrupts , PWM and one shot as a sort of MFD etc there's no consensus yet.
>>>>
>>>> Leaf it as it is till someone made a working driver demo, so that the coder is free to model a DT solution that fits to him/her.
>>> Having the warnings until this happens is bad though. If describing the
>>> irqs in the schema is considered wrong, we should remove the interrupts
>>> properties from the device tree sources.
>> I think the previous thread mixes bindings with driver. Does the
>> hardware have interrupt? Yes? Add it to the bindings. No? Don't add it.
>>
>> However Johan's reply is saying something about driver, so how is it
>> related?
> Following Krzysztof's argumentation I'm inclined to apply the patch
> despite Johan's objection as the irqs are already described in the
> device trees and not having them in the binding only adds warnings to
> the dt checks.
The interrupts are common interrupts for a group of 4 PWM channels!

The name PWM channels is somewhat confusing.

It has various modes not related to PWM


>From Rockchip RK3368 TRM V2.0.pdf:


Continuous Mode:
No interrupts.


One-shot Mode:
an interrupt is asserted to inform that the operation has been finished

Reference mode:

asserts an interrupt when the polarity of the input waveform changes.

Also IR

===

PWM_INTSTS0x00040 W0x00000000 Interrupt Status Register
PWM_INT_EN0x00044 W0x00000000 Interrupt Enable Register
PWM_FIFO_CTRL0x00050 W0x00000000
PWM Channel 3 FIFO Mode Control Register
PWM_FIFO_INTSTS0x00054 W0x00000000 FIFO Interrupts Status Register


===
    pwm0: pwm at ff680000 {
        compatible = "rockchip,rk3368-pwm", "rockchip,rk3288-pwm";
        reg = <0x0 0xff680000 0x0 0x10>;
    };

    pwm1: pwm at ff680010 {
        compatible = "rockchip,rk3368-pwm", "rockchip,rk3288-pwm";
        reg = <0x0 0xff680010 0x0 0x10>;
    };

    pwm2: pwm at ff680020 {
        compatible = "rockchip,rk3368-pwm", "rockchip,rk3288-pwm";
        reg = <0x0 0xff680020 0x0 0x10>;
    };

    pwm3: pwm at ff680030 {
        compatible = "rockchip,rk3368-pwm", "rockchip,rk3288-pwm";
        reg = <0x0 0xff680030 0x0 0x10>;
    };

===

The interrupt registers are located outside the PWM range and have nothing to do with the PWM driver.
Adding them to a PWM binding is just bogus.
They are a left over from the manufacturer tree that use them in a IR detection driver.
Heiko keeps them because someone outside mainline kernel might use them?
They should be removed and remodeled in a new sort of MFD node that handles all operating 3 modes.


Johan
>
> Best regards
> Uwe
>