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

From: <Claudiu.Beznea@microchip.com>
To: <uwe@kleine-koenig.org>, <thierry.reding@gmail.com>
Cc: linux-pwm@vger.kernel.org, alexandre.belloni@bootlin.com,
	Ludovic.Desroches@microchip.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 4/3] pwm: atmel: document known weaknesses of both hardware and software
Date: Mon, 19 Aug 2019 09:26:04 +0000	[thread overview]
Message-ID: <0a389abe-15ef-0e63-109f-2db4cb36f4b9@microchip.com> (raw)
In-Reply-To: <20190816093748.11769-1-uwe@kleine-koenig.org>

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.)

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.

> + *
> + * 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(). 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()).

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