[PATCH] pwm: lpc32xx: Set PWM_PIN_LEVEL bit in lpc32xx_pwm_disable

[Vladimir Zapolskiy]

Hi Sylvain,

On 21.06.2016 15:39, Sylvain Lemieux wrote:
> Hi Vladimir,
> 
> On Tue, 2016-06-21 at 05:11 +0300, Vladimir Zapolskiy wrote:
>> Hi Sylvain,
>>
>> On 03.06.2016 22:37, Sylvain Lemieux wrote:
>>> From: Sylvain Lemieux <slemieux at tycoint.com>
>>>
>>> If the PWM_PIN_LEVEL bit is setup to 1 in the bootloader, when the kernel
>>> disable the PWM, the PWM output is always set as a logic 1.
>>>
>>> Prior to commit 08ee77b5a5de27ad63c92262ebcb4efe0da93b58,
>>> the PWM_PIN_LEVEL bit was always clear when the PWM was disable
>>> and a 0 logic level was apply to the output.
>>>
>>> According to the LPC32x0 User Manual [1],
>>> the default value for bit 30 (PWM_PIN_LEVEL) is 0.
>>>
>>> This change initialize the pin level to 0 (default value) and
>>> update the register value accordingly during the disable process.
>>>
>>> [1] http://www.nxp.com/documents/user_manual/UM10326.pdf
>>>
>>> Signed-off-by: Sylvain Lemieux <slemieux at tycoint.com>
>>
>> It looks like a pin control setting, but because it depends
>> on PWM enabled/disabled status, I suppose it is good enough to
>> manage it from the PWM driver.
>>
>> I think here a new optional DTS property should be introduced,
>> which if present indicates that PWM_OUT pin value is high when
>> PWM is disabled. And if the property is not found then
>> PWMx_PIN_LEVEL is set to 0 on driver probe.
>>
> I will add this option and submit a new revision of the patch.
> 
>> By convention the property name should be prefixed by "nxp,",
>> what name is good? May be "nxp,pwm-disabled-level-high" ?
>> Or add a property "nxp,pwm-disabled-level" with valid values 0/1?
>>
> I prefer to use "nxp,pwm-disabled-level-high".
> 

nice, for v2 please add documentation devicetree bindings bits and
add devicetree mailing list to Cc, hopefully Rob will find time
to review the name.

Have you tried to set PWMx_PIN_LEVEL only once on probe()?
I think there is no need to set/unset it every time when PWM is
enabled or disabled. If it works, then the newly introduced local
state storage can be removed.

>> Also note someone may want to switch the default behaviour
>> in runtime, but this feature may be added later on.
>>
> I agree with you, this can be done later.


--
With best wishes,
Vladimir