Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Jul 2023 17:12:30 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Guiting Shen <aarongt.shen@gmail.com>
Cc:     claudiu.beznea@microchip.com, u.kleine-koenig@pengutronix.de,
        nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com,
        linux-arm-kernel@lists.infradead.org, linux-pwm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6] pwm: atmel: Enable clk when pwm already enabled in
 bootloader
Message-ID: <ZLlO3vGr-ECdnmKA@orome>
References: <20230716020652.18557-1-aarongt.shen@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="04hiCDAIozMFNoR9"
Content-Disposition: inline
In-Reply-To: <20230716020652.18557-1-aarongt.shen@gmail.com>
User-Agent: Mutt/2.2.10 (2023-03-25)
Precedence: bulk


--04hiCDAIozMFNoR9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> Add atmel_pwm_enable_clk_if_on() in probe function to enable clock if
> the PWM channel was already enabled in bootloader.
>=20
> Signed-off-by: Guiting Shen <aarongt.shen@gmail.com>
> ---
>  drivers/pwm/pwm-atmel.c | 47 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
>=20
> 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
> =20
>  /* 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[] =
=3D {
>  };
>  MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
> =20
> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm, =
bool on)
> +{
> +	unsigned int i, cnt =3D 0;
> +	int ret =3D 0;
> +	u32 sr;
> +
> +	sr =3D atmel_pwm_readl(atmel_pwm, PWM_SR) & PWM_SR_ALL_CH_MASK;
> +	if (!sr)
> +		return 0;
> +
> +	cnt =3D 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
=66rom 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

--04hiCDAIozMFNoR9
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmS5Tt4ACgkQ3SOs138+
s6Eyyg/+IrDQoD75AWJiosESNAMWjiyDEFKB6A6+L49742LeepmeKIDeRBIe/Kmt
xxg6Ysa36q22YLb91dSMHk8fGJiDyYWz29H7WBSM64o9nrDEEr7cBOMEQDCl1pA9
AHbAseMMd3RgGWzWu8WwZUrI8sTUBviqTBRi5uKgKap9+PwUZV1SF4ffVHIXiXxF
M8K763aA7TxtS36IPRJdHO7g+FL0X/IRG7+7CpAk7AIYJCApuTlyQkSEqLJbgkG3
lyT5UK+wCToazgLIAQX2M79zQOqOmyu5xiNUC40XdEZxb1GCmVy/ey47cPp76+JE
m/zhEJurS+kxBZgmHik9wSngo1YU3EbHsyrwDc1FPJcGsCmmA8xA+m1UYujEYUzU
+eOaw8a8zZICXDsWwx/q3P+E0tOPVV9jIqg59gKfLu/l/gaP5tU7VMdTjM1ETVNh
LX9NQtc6wt4w9iryjJKvs4ItQ4oxRVK+PNVwHGn7GM9koeeEozan1QHLcj8u4G3H
mJQTPd6uSKzkw5hgOWY4DRO+38sAZx/9v+1HaGg6hPsufGGYVU7+Q/U77PsHBuJF
c/CCQ4y5zBREWpslBVT/I7hfmULQgC2jMP7A+xbUUy0ymquTXM77vtdkm5QiZ2bL
GTpTK82piENd9PQWFzLbjgYbJ33LhlTdRbLtGqwndFEBpC9myxE=
=DeRZ
-----END PGP SIGNATURE-----

--04hiCDAIozMFNoR9--