Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in bootloader

[Thierry Reding]

On Sun, Jul 16, 2023 at 10:06:52AM +0800, Guiting Shen wrote:
> The driver would never call clk_enable() if the PWM channel was already
> enabled in bootloader which lead to dump the warning message "the PWM
> clock already disabled" when turning off the PWM channel.
> 
> Add atmel_pwm_enable_clk_if_on() in probe function to enable clock if
> the PWM channel was already enabled in bootloader.
> 
> Signed-off-by: Guiting Shen <aarongt.shen@xxxxxxxxx>
> ---
>  drivers/pwm/pwm-atmel.c | 47 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index cdbc23649032..fc89282db645 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -36,7 +36,7 @@
>  #define PWM_SR			0x0C
>  #define PWM_ISR			0x1C
>  /* Bit field in SR */
> -#define PWM_SR_ALL_CH_ON	0x0F
> +#define PWM_SR_ALL_CH_MASK	0x0F
>  
>  /* The following register is PWM channel related registers */
>  #define PWM_CH_REG_OFFSET	0x200
> @@ -464,6 +464,42 @@ static const struct of_device_id atmel_pwm_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
>  
> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm, bool on)
> +{
> +	unsigned int i, cnt = 0;
> +	int ret = 0;
> +	u32 sr;
> +
> +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR) & PWM_SR_ALL_CH_MASK;
> +	if (!sr)
> +		return 0;
> +
> +	cnt = bitmap_weight((unsigned long *)&sr, atmel_pwm->chip.npwm);

Tiny nit here: not sure if that cast is safe to do. You've got a 32-bit
variable, but if you cast &sr to unsigned long * on a 64-bit machine it
would cause hweight64() to get called and that would then read 64 bits
from a 32-bit variable. This probably works most of the time because we
don't read any of the upper bits, but it is strictly an illegal access
and could be unaligned as well.

Should we just turn sr into an unsigned long to be safe here? No need to
resend, I can make that change when I apply.

Thierry

Attachment: signature.asc
Description: PGP signature