Re: [PATCH 4/3] pwm: atmel: document known weaknesses of both hardware and software

[<Claudiu.Beznea@microchip.com>]

On 19.08.2019 13:46, Uwe Kleine-König wrote:
> External E-Mail
> 
> 
> On Mon, Aug 19, 2019 at 09:26:04AM +0000, Claudiu.Beznea@microchip.com wrote:
>>
>>
>> On 16.08.2019 12:37, Uwe Kleine-König wrote:
>>> External E-Mail
>>>
>>>
>>> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>>> ---
>>>  drivers/pwm/pwm-atmel.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
>>> index 42fe7bc043a8..1ddb93db9627 100644
>>> --- a/drivers/pwm/pwm-atmel.c
>>> +++ b/drivers/pwm/pwm-atmel.c
>>> @@ -7,6 +7,16 @@
>>>   *
>>>   * Reference manual for "atmel,at91sam9rl-pwm":
>>>   *   http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-11032-32-bit-ARM926EJ-S-Microcontroller-SAM9G25_Datasheet.pdf
>>> + *
>>> + * Limitations:
>>> + * - Periods start with the inactive level.
>>
>> Are you talking here about the normal polarity (from documentation: By
>> definition, normal polarity characterizes a signal starts high for the
>> duration of the duty cycle and goes low for the remainder of the period.)
> 
> When .polarity = PWM_POLARITY_NORMAL is passed to atmel_pwm_apply() the
> drivers sets PWM_CMR_CPOL=0 which according to the datasheet (linked
> above) means: "The output waveform starts at a low level."
> 
> So maybe just the logic has to be inverted there,

Agree.

> but then maybe the
> output gets active instead of inactive when the PWM is disabled.

Yes, this would happen. Playing again with CPOL when disabling may be a
solution.

> (Which in my book is ok, but it's Thierry's opinion that counts here.)
> 
>> If yes, this should be solved by playing with CPOL bit of CMR.
>>
>>> + * - Hardware has to be stopped in general to update settings.
>>
>> Sama5d2 has duty cycle that could be updated on the fly.
> 
> There is some functionality in the 9G25, too. I didn't understand it
> completely but maybe it only helps updating one of period or duty cycle.
>  
>>> + *
>>> + * Software bugs/possible improvements:
>>> + * - When atmel_pwm_apply() is called with state->enabled=false a change in
>>> + *   state->polarity isn't honored.
>>
>> I know that when configuring a PWM one should get the current state of the
>> PWM, change it, then pass it to the driver via pwm_apply_state().
> 
> That seems to be a common pattern at least. IMHO letting the consumer
> just configure the state that should be used should be fine, too.>
>> In case one would call the pwm_apply_state() with state->enabled =
>> false the state would be stored in PWM specific object (of type struct
>> pwm_device). On the next apply, with enabled = true, all the PWM
>> parameters would be actually applied to hardware. So, until
>> enable=true the PWM state would only be cached by PWM core specific
>> objects (in pwm_apply_state()).
> 
> I fail to follow what you mean here. If a PWM runs with (say) normal
> polarity and you call pwm_apply_state(mypwm, { .polarity =
> PWM_POLARITY_INVERSED, .enabled = false, }); the apply callback of the
> lowlevel driver is called and supposed to configure the output to yield
> a constant high.

Ok, I see it now. I'll put it on my queue.

Thank you,
Claudiu Beznea

> 
>>> + * - Instead of sleeping to wait for a completed period, the interrupt
>>> + *   functionality could be used.
>>>   */
>>>  
>>>  #include <linux/clk.h>
>>>
> 
> Best regards
> Uwe
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel