Re: [PATCH] pwm: pwm-mxs: Apply configuration before disabling PWM.

Re: [PATCH] pwm: pwm-mxs: Apply configuration before disabling PWM.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]

Please use my new email address and Cc the linux-pwm mailing list.

The subject implies some active procedure is used to make sure the
configuration is applied, but you really only wait for some amount of
time. Perhaps something like:

	pwm: pwm-mxs: Wait for configuration to apply before disabling PWM

is more accurate?

On Wed, Jun 19, 2013 at 01:51:26PM +0200, Robin van der Gracht wrote:
> When disabling the pwm, the output state locks at its current state.

Please use the proper spelling "PWM" in prose.

> We have to be sure the last configuration applied. Which in most
> cases sets duty cycle to 0%. To prevent the pwm from taking on
> 100% duty cycle when disabled during a high state.
> 
> Configuration applies at the beginning of a new output period.

I have some trouble understanding this, but I think you mean:

We have to be sure that the last configuration has been applied. In most
cases drivers will have set the duty-cycle to 0%. To prevent the PWM
from locking at a 100% duty-cycle for example, we delay disabling the
PWM for a whole period to make sure any new configuration has been
latched.

> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> index 3febddd..4ddc063 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -21,6 +21,7 @@
>  #include <linux/pwm.h>
>  #include <linux/slab.h>
>  #include <linux/stmp_device.h>
> +#include <linux/delay.h>

Please keep the includes sorted alphabetically.

>  
>  #define SET	0x4
>  #define CLR	0x8
> @@ -40,6 +41,7 @@ struct mxs_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
>  	void __iomem *base;
> +	unsigned long period_ns;

This is not the proper place to put it. The period can be different for
each PWM channel. But you also don't need to store this separately as in
latest linux-next this is already done by the core. You can use the
pwm_get_period() function to obtain the current period from a PWM
device.

> @@ -113,6 +116,11 @@ static void mxs_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct mxs_pwm_chip *mxs = to_mxs_pwm_chip(chip);
>  
> +	/*
> +	 * Ensure latest configuration applied.
> +	 */

This comment can go on a single line.

> +	ndelay(mxs->period_ns);

This introduces a potentially long delay. How about changing this to
something like:

	period = pwm_get_period(pwm);
	period = DIV_ROUND_UP(period, 1000);
	usleep_range(period, period + 1000);

?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] pwm: pwm-mxs: Apply configuration before disabling PWM.

[Robin van der Gracht]

Hello Thierry,


On 06/21/2013 12:02 PM, Thierry Reding wrote:
> Please use my new email address and Cc the linux-pwm mailing list.
>
> The subject implies some active procedure is used to make sure the
> configuration is applied, but you really only wait for some amount of
> time. Perhaps something like:
>
> 	pwm: pwm-mxs: Wait for configuration to apply before disabling PWM
>
> is more accurate?
Agreed
>
> On Wed, Jun 19, 2013 at 01:51:26PM +0200, Robin van der Gracht wrote:
>> When disabling the pwm, the output state locks at its current state.
> Please use the proper spelling "PWM" in prose.
Ok
>
>> We have to be sure the last configuration applied. Which in most
>> cases sets duty cycle to 0%. To prevent the pwm from taking on
>> 100% duty cycle when disabled during a high state.
>>
>> Configuration applies at the beginning of a new output period.
> I have some trouble understanding this, but I think you mean:
>
> We have to be sure that the last configuration has been applied. In most
> cases drivers will have set the duty-cycle to 0%. To prevent the PWM
> from locking at a 100% duty-cycle for example, we delay disabling the
> PWM for a whole period to make sure any new configuration has been
> latched.
Yes that is correct.
>
>> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
>> index 3febddd..4ddc063 100644
>> --- a/drivers/pwm/pwm-mxs.c
>> +++ b/drivers/pwm/pwm-mxs.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/pwm.h>
>>   #include <linux/slab.h>
>>   #include <linux/stmp_device.h>
>> +#include <linux/delay.h>
> Please keep the includes sorted alphabetically.
I'll update that.
>
>>   
>>   #define SET	0x4
>>   #define CLR	0x8
>> @@ -40,6 +41,7 @@ struct mxs_pwm_chip {
>>   	struct pwm_chip chip;
>>   	struct clk *clk;
>>   	void __iomem *base;
>> +	unsigned long period_ns;
> This is not the proper place to put it. The period can be different for
> each PWM channel. But you also don't need to store this separately as in
> latest linux-next this is already done by the core. You can use the
> pwm_get_period() function to obtain the current period from a PWM
> device.
>
>> @@ -113,6 +116,11 @@ static void mxs_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>>   {
>>   	struct mxs_pwm_chip *mxs = to_mxs_pwm_chip(chip);
>>   
>> +	/*
>> +	 * Ensure latest configuration applied.
>> +	 */
> This comment can go on a single line.
>
>> +	ndelay(mxs->period_ns);
> This introduces a potentially long delay. How about changing this to
> something like:
>
> 	period = pwm_get_period(pwm);
> 	period = DIV_ROUND_UP(period, 1000);
> 	usleep_range(period, period + 1000);
Thanks for the input, I agree on your comment. I'll resubmit the patch.
>
> ?
>
> Thierry


-- 
Robin van der Gracht
Protonic Holland.
tel.: +31 (0) 229 212928
fax.: +31 (0) 229 210930
Factorij 36 / 1689 AL Zwaag