[PATCH 5/5] arm: dts: lpc32xx: add device node for the second pwm controller

[Vladimir Zapolskiy]

Hi Joachim,

On 14.10.2015 21:04, Joachim Eastwood wrote:
> Hi Vladimir,
> 
> On 13 October 2015 at 01:54, Vladimir Zapolskiy <vz at mleia.com> wrote:
>> LPC32xx SoCs have two independent PWM controllers, they have different
>> clock parents, clock gates and even slightly different controls,
>> each of these two PWM controllers has one output channel. Due to
>> almost similar controls arranged in a row it is incorrectly assumed
>> that there is one PWM controller with two channels, fix this problem
>> in lpc32xx.dtsi, which at the moment prevents separate configuration
>> of different clock parents and gates for both PWM controllers.
>>
>> Signed-off-by: Vladimir Zapolskiy <vz at mleia.com>
>> ---
>>  arch/arm/boot/dts/lpc32xx.dtsi | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/lpc32xx.dtsi b/arch/arm/boot/dts/lpc32xx.dtsi
>> index 929458d..f4d2a0e 100644
>> --- a/arch/arm/boot/dts/lpc32xx.dtsi
>> +++ b/arch/arm/boot/dts/lpc32xx.dtsi
>> @@ -286,9 +286,15 @@
>>                                 status = "disabled";
>>                         };
>>
>> -                       pwm: pwm at 4005C000 {
>> +                       pwm1: pwm at 4005C000 {
>>                                 compatible = "nxp,lpc3220-pwm";
>> -                               reg = <0x4005C000 0x8>;
>> +                               reg = <0x4005C000 0x4>;
>> +                               status = "disabled";
>> +                       };
>> +
>> +                       pwm2: pwm at 4005C004 {
>> +                               compatible = "nxp,lpc3220-pwm";
>> +                               reg = <0x4005C004 0x4>;
>>                                 status = "disabled";
>>                         };
>>                 };
> 
> I am not really against your change, but...
> 
> What's wrong with a binding like the one below?
> pwm: pwm at 0x4005c000 {
>     compatible = "nxp,lpc3220-pwm";
>     reg = <0x4005C000 0x8>;
>     clocks =<&clk CLK_PWM1, &clk CLK_PWM2>;
>     clock-names = "pwm1", "pwm2";
>     #pwm-cells = <3>;
> };
> 
> With two clocks and where the first pwm-cell would select either PWM1 or PWM2.
> 
> Seems like the driver only handle one clock, but should be fairly easy to fix.

I thought about it and IMHO it is a more complicated change in DTS (and
no doubts in the driver), which hides the structure of hardware. There
is no one PWM with two channels.

There are two independent PWMs, even control registers are different,
PWM2 can be programmed to output the internal interrupt status, and it
means that possibly in future I may want to describe one of the PWMs
with a different "compatible" property.

> Note: with your DT change you would also need to change the driver
> since it currently sets npwm to 2.
> 

It is done -- http://permalink.gmane.org/gmane.linux.pwm/2831

Thanks for review.

--
With best wishes,
Vladimir