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:   Mon, 10 Jul 2023 17:00:45 +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] pwm: atmel: enable clk when pwm already enabled in
 bootloader
Message-ID: <ZKwdHUWzXujfVk0R@orome>
References: <20230710144214.63343-1-aarongt.shen@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="PpEzny3LgeSzGCEI"
Content-Disposition: inline
In-Reply-To: <20230710144214.63343-1-aarongt.shen@gmail.com>
User-Agent: Mutt/2.2.10 (2023-03-25)
Precedence: bulk


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

On Mon, Jul 10, 2023 at 10:42:14PM +0800, Guiting Shen wrote:
> The driver would never call clk_eanble() if the pwm channel already
> enable in bootloader which lead to dump the warning message of "the pwm
> clk already disabled" when poweroff the pwm channel.
>=20
> Add atmel_pwm_enanle_clk_if_on() in probe function to enable clk if the
> pwm channel already enabled in bootloader.

You've got multiple spelling errors in the commit message. Also, PWM is
an abbreviation and so should be all uppercase (except for the subject
prefix). I also prefer spelling out terms like "clock" in the commit
message. This is text that is supposed to be readable. It's not code.

>=20
> Signed-off-by: Guiting Shen <aarongt.shen@gmail.com>
> ---
>  drivers/pwm/pwm-atmel.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>=20
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index cdbc23649032..385f12eb604c 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -464,6 +464,29 @@ 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)
> +{
> +	u32 sr, i;

Maybe make i an unsigned int since you use it to iterate over npwm,
which is unsigned int as well.

> +	int err;
> +
> +	sr =3D atmel_pwm_readl(atmel_pwm, PWM_SR);
> +	if (!sr)
> +		return 0;
> +
> +	for (i =3D 0; i < atmel_pwm->chip.npwm; i++) {
> +		if (!(sr & (1 << i)))

We would usually write this as BIT(i), but I see that the rest of the
driver uses this notation, so it's fine to keep this as-is.

> +			continue;
> +
> +		err =3D clk_enable(atmel_pwm->clk);
> +		if (err) {
> +			dev_err(atmel_pwm->chip.dev, "enable clock error\n");

Might be worth to include the error code in the error message to make it
easier to diagnose where the issue is. Something like:

	dev_err(atmel_pwm->chip.dev, "failed to enable clock: %d\n", err);

> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int atmel_pwm_probe(struct platform_device *pdev)
>  {
>  	struct atmel_pwm_chip *atmel_pwm;
> @@ -504,6 +527,10 @@ static int atmel_pwm_probe(struct platform_device *p=
dev)
> =20
>  	platform_set_drvdata(pdev, atmel_pwm);
> =20
> +	ret =3D atmel_pwm_enable_clk_if_on(atmel_pwm);
> +	if (ret < 0)
> +		goto unprepare_clk;

This is not correct. You call this after pwmchip_add(), so you need to
make sure to also remove the PWM chip on error. Preferably, though, you
should call this before adding the PWM chip in the first place.

> +
>  	return ret;
> =20
>  unprepare_clk:
> --=20
> 2.25.1
>=20

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

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

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmSsHRoACgkQ3SOs138+
s6HwcA/7BFd9ge/azWqrQpQcrEonM4lx7ehmrULQvoO7zYkybxukrb6mzYne8MuM
4Cw4Nw9wUGCglxP2L7epAXLhTQVmZIS16Q+jMSkAHTOshLXuuoqJpifsgTuLz7zk
4aMu4NtZ4dFjDvq3850Y5qbXMRjLUEDMxgpn05TzBQYX1OPO8N4vgk//69NoyaZU
8nqT7TrjOUNHCE9RIrQxzH1YOAonPn3ke5B6CCllxnG4hk9xMQNSQHHwvqgJpZ9n
Kz6CQc44taCP4ERcVjyqG+PTJWEOZUSC1QuWMsDcW2AWiPjzeRdpC44MSoPyFT16
o30TIbIR5Jc1nOpy73Ohku/BWt18T6wqg2xVhacYoT8JdkUuQr7BDCXQeDa4xpg2
MfkRxQU6eTY/RuOfionH0H/XG+M8WIijD0Qtysxkh6c6PMM5Wgr6ES9XiWIco0rJ
KNBDDkVdA57QtozTKIUwYTfWp4IKpwFXA/sHsu8x0tCxsHR/Gn/p7RwByPHXvZVZ
dBDHqbxc0PWY/SqSswi2Zm46dWo0dDBztt42KcQ3fLerHiGFDUno+7YZjSE90O0g
sU+KaXmukBIHiNelpy9gGY5NmwEP2DfRnigDWULRjf5Hen4cCdIwqstLldGk1bS4
BzIowyhBBwrvx2oPPeLYcGD3pKoitpqj9hIwfOrxHOMxMQ3mpjg=
=SrRC
-----END PGP SIGNATURE-----

--PpEzny3LgeSzGCEI--