[PATCH] pwm: atmel: enable clk when pwm already enabled in bootloader

[PATCH] pwm: atmel: enable clk when pwm already enabled in bootloader

[Guiting Shen]

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.

Add atmel_pwm_enanle_clk_if_on() in probe function to enable clk if the
pwm channel already enabled in bootloader.

Signed-off-by: Guiting Shen <aarongt.shen@gmail.com>
---
 drivers/pwm/pwm-atmel.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

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[] = {
 };
 MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
 
+static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
+{
+	u32 sr, i;
+	int err;
+
+	sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
+	if (!sr)
+		return 0;
+
+	for (i = 0; i < atmel_pwm->chip.npwm; i++) {
+		if (!(sr & (1 << i)))
+			continue;
+
+		err = clk_enable(atmel_pwm->clk);
+		if (err) {
+			dev_err(atmel_pwm->chip.dev, "enable clock error\n");
+			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 *pdev)
 
 	platform_set_drvdata(pdev, atmel_pwm);
 
+	ret = atmel_pwm_enable_clk_if_on(atmel_pwm);
+	if (ret < 0)
+		goto unprepare_clk;
+
 	return ret;
 
 unprepare_clk:
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2706 bytes --]

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

> 
> Signed-off-by: Guiting Shen <aarongt.shen@gmail.com>
> ---
>  drivers/pwm/pwm-atmel.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> 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[] = {
>  };
>  MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
>  
> +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 = atmel_pwm_readl(atmel_pwm, PWM_SR);
> +	if (!sr)
> +		return 0;
> +
> +	for (i = 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 = 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 *pdev)
>  
>  	platform_set_drvdata(pdev, atmel_pwm);
>  
> +	ret = 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;
>  
>  unprepare_clk:
> -- 
> 2.25.1
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 766 bytes --]

Hello,

On Mon, Jul 10, 2023 at 05:00:45PM +0200, Thierry Reding wrote:
> On Mon, Jul 10, 2023 at 10:42:14PM +0800, Guiting Shen wrote:
> > +		err = 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);

Or (IMHO) still better:

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Guiting Shen]

On Tue, Jul 11, 2023 at 03:15:50AM GMT+8, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Jul 10, 2023 at 05:00:45PM +0200, Thierry Reding wrote:
>> On Mon, Jul 10, 2023 at 10:42:14PM +0800, Guiting Shen wrote:
>>> +		err = 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);
> 
> Or (IMHO) still better:
> 
>  	dev_err(atmel_pwm->chip.dev, "failed to enable clock: %pe\n", ERR_PTR(err));
> 

Ok, I will add it in v2 patch.

I also found that the clk_enable() of atmel_pwm_apply() do not include
the error code in the error message. Do I also add this in v2 patch or
in separate patch?

-- 
Regards,
Guiting Shen


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1205 bytes --]

On Tue, Jul 11, 2023 at 10:37:56AM +0800, Guiting Shen wrote:
> On Tue, Jul 11, 2023 at 03:15:50AM GMT+8, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Jul 10, 2023 at 05:00:45PM +0200, Thierry Reding wrote:
> >> On Mon, Jul 10, 2023 at 10:42:14PM +0800, Guiting Shen wrote:
> >>> +		err = 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);
> > 
> > Or (IMHO) still better:
> > 
> >  	dev_err(atmel_pwm->chip.dev, "failed to enable clock: %pe\n", ERR_PTR(err));
> > 
> 
> Ok, I will add it in v2 patch.
> 
> I also found that the clk_enable() of atmel_pwm_apply() do not include
> the error code in the error message. Do I also add this in v2 patch or
> in separate patch?

Sounds to me like a separate patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Guiting Shen]

On Mon, Jul 10, 2023 at 23:00:45PM GMT+8, Thierry Reding wrote:
> 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.
>>
>> 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.

Got it, Thank you. How about this commit message:

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 turn off the PWM channel.

Add atmel_pwm_enable_clk_if_on() in probe function to enable clk if the
PWM channel was already enabled in bootloader.

>>
>> Signed-off-by: Guiting Shen <aarongt.shen@gmail.com>
>> ---
>>  drivers/pwm/pwm-atmel.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> 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[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids);
>>  
>> +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.

Ok, I will correct it in v2 patch.

>> +	int err;
>> +
>> +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
>> +	if (!sr)
>> +		return 0;
>> +
>> +	for (i = 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 = 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 *pdev)
>>  
>>  	platform_set_drvdata(pdev, atmel_pwm);
>>  
>> +	ret = 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.

Sorry, I will call this before adding the PWM chip in v2 patch.

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

-- 
Regards,
Guiting Shen


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1298 bytes --]

On Tue, Jul 11, 2023 at 10:30:54AM +0800, Guiting Shen wrote:
> On Mon, Jul 10, 2023 at 23:00:45PM GMT+8, Thierry Reding wrote:
> > 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.
> >>
> >> 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.
> 
> Got it, Thank you. How about this commit message:
> 
> 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 turn off the PWM channel.
> 
> Add atmel_pwm_enable_clk_if_on() in probe function to enable clk if the
> PWM channel was already enabled in bootloader.

s/clk/clock/ but otherwise looks good.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel