[PATCH 1/2] pwm: pca9685: fix period change with same duty cycle

[PATCH 1/2] pwm: pca9685: fix period change with same duty cycle

[Clemens Gruber]

When first implementing support for changing the output frequency, an
optimization was added to continue the PWM after changing the prescaler
without having to reprogram the ON and OFF registers for the duty cycle,
in case the duty cycle stayed the same.
This was flawed, because we compared the absolute value of the duty
cycle in nanoseconds instead of the ratio to the period.

Fix the problem by removing the shortcut.

Fixes: 01ec8472009c9 ("pwm-pca9685: Support changing the output
frequency")
Cc: <stable@vger.kernel.org> # v4.3+
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/pwm/pwm-pca9685.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 117fccf..01a6a83 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -65,7 +65,6 @@
 #define PCA9685_MAXCHAN		0x10
 
 #define LED_FULL		(1 << 4)
-#define MODE1_RESTART		(1 << 7)
 #define MODE1_SLEEP		(1 << 4)
 #define MODE2_INVRT		(1 << 4)
 #define MODE2_OUTDRV		(1 << 2)
@@ -117,16 +116,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			udelay(500);
 
 			pca->period_ns = period_ns;
-
-			/*
-			 * If the duty cycle did not change, restart PWM with
-			 * the same duty cycle to period ratio and return.
-			 */
-			if (duty_ns == pca->duty_ns) {
-				regmap_update_bits(pca->regmap, PCA9685_MODE1,
-						   MODE1_RESTART, 0x1);
-				return 0;
-			}
 		} else {
 			dev_err(chip->dev,
 				"prescaler not set: period out of bounds!\n");
-- 
2.10.2

[PATCH 2/2] pwm: pca9685: fix prescaler initialization

[Clemens Gruber]

Until now, we assumed that the period is the hardware default of 1/200Hz
at probe time, but if the period was changed and the user reboots, this
assumption is wrong.

Solution: Check if the prescaler is set to the hardware default. If not,
reprogram the prescaler at first configuration.

Cc: <stable@vger.kernel.org> # v4.3+
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/pwm/pwm-pca9685.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 01a6a83..efc657e 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -55,6 +55,7 @@
 #define PCA9685_PRESCALE	0xFE
 
 #define PCA9685_PRESCALE_MIN	0x03	/* => max. frequency of 1526 Hz */
+#define PCA9685_PRESCALE_DEF	0x1E	/* => default frequency of 200 Hz */
 #define PCA9685_PRESCALE_MAX	0xFF	/* => min. frequency of 24 Hz */
 
 #define PCA9685_COUNTER_RANGE	4096
@@ -289,8 +290,8 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 				const struct i2c_device_id *id)
 {
 	struct pca9685 *pca;
+	int prescale, mode2;
 	int ret;
-	int mode2;
 
 	pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
 	if (!pca)
@@ -304,10 +305,15 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 		return ret;
 	}
 	pca->duty_ns = 0;
-	pca->period_ns = PCA9685_DEFAULT_PERIOD;
 
 	i2c_set_clientdata(client, pca);
 
+	regmap_read(pca->regmap, PCA9685_PRESCALE, &prescale);
+	if (prescale == PCA9685_PRESCALE_DEF)
+		pca->period_ns = PCA9685_DEFAULT_PERIOD;
+	else
+		pca->period_ns = 0;
+
 	regmap_read(pca->regmap, PCA9685_MODE2, &mode2);
 
 	if (device_property_read_bool(&client->dev, "invert"))
-- 
2.10.2

Re: [PATCH 1/2] pwm: pca9685: fix period change with same duty cycle

[Thierry Reding]

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

On Tue, Dec 13, 2016 at 04:52:50PM +0100, Clemens Gruber wrote:
> When first implementing support for changing the output frequency, an
> optimization was added to continue the PWM after changing the prescaler
> without having to reprogram the ON and OFF registers for the duty cycle,
> in case the duty cycle stayed the same.
> This was flawed, because we compared the absolute value of the duty
> cycle in nanoseconds instead of the ratio to the period.
> 
> Fix the problem by removing the shortcut.
> 
> Fixes: 01ec8472009c9 ("pwm-pca9685: Support changing the output frequency")
> Cc: <stable@vger.kernel.org> # v4.3+
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
>  drivers/pwm/pwm-pca9685.c | 11 -----------
>  1 file changed, 11 deletions(-)

Mika, Andy, can you guys help review this and 2/2?

Thanks,
Thierry

> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 117fccf..01a6a83 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -65,7 +65,6 @@
>  #define PCA9685_MAXCHAN		0x10
>  
>  #define LED_FULL		(1 << 4)
> -#define MODE1_RESTART		(1 << 7)
>  #define MODE1_SLEEP		(1 << 4)
>  #define MODE2_INVRT		(1 << 4)
>  #define MODE2_OUTDRV		(1 << 2)
> @@ -117,16 +116,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			udelay(500);
>  
>  			pca->period_ns = period_ns;
> -
> -			/*
> -			 * If the duty cycle did not change, restart PWM with
> -			 * the same duty cycle to period ratio and return.
> -			 */
> -			if (duty_ns == pca->duty_ns) {
> -				regmap_update_bits(pca->regmap, PCA9685_MODE1,
> -						   MODE1_RESTART, 0x1);
> -				return 0;
> -			}
>  		} else {
>  			dev_err(chip->dev,
>  				"prescaler not set: period out of bounds!\n");
> -- 
> 2.10.2
> 

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

Re: [PATCH 2/2] pwm: pca9685: fix prescaler initialization

[Thierry Reding]

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

On Tue, Dec 13, 2016 at 04:52:51PM +0100, Clemens Gruber wrote:
> Until now, we assumed that the period is the hardware default of 1/200Hz
> at probe time, but if the period was changed and the user reboots, this
> assumption is wrong.
> 
> Solution: Check if the prescaler is set to the hardware default. If not,
> reprogram the prescaler at first configuration.
> 
> Cc: <stable@vger.kernel.org> # v4.3+
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
>  drivers/pwm/pwm-pca9685.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Cc'ing Mika and Andy and quoting verbatim for review.

Thierry

> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 01a6a83..efc657e 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -55,6 +55,7 @@
>  #define PCA9685_PRESCALE	0xFE
>  
>  #define PCA9685_PRESCALE_MIN	0x03	/* => max. frequency of 1526 Hz */
> +#define PCA9685_PRESCALE_DEF	0x1E	/* => default frequency of 200 Hz */
>  #define PCA9685_PRESCALE_MAX	0xFF	/* => min. frequency of 24 Hz */
>  
>  #define PCA9685_COUNTER_RANGE	4096
> @@ -289,8 +290,8 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  				const struct i2c_device_id *id)
>  {
>  	struct pca9685 *pca;
> +	int prescale, mode2;
>  	int ret;
> -	int mode2;
>  
>  	pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
>  	if (!pca)
> @@ -304,10 +305,15 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  	pca->duty_ns = 0;
> -	pca->period_ns = PCA9685_DEFAULT_PERIOD;
>  
>  	i2c_set_clientdata(client, pca);
>  
> +	regmap_read(pca->regmap, PCA9685_PRESCALE, &prescale);
> +	if (prescale == PCA9685_PRESCALE_DEF)
> +		pca->period_ns = PCA9685_DEFAULT_PERIOD;
> +	else
> +		pca->period_ns = 0;
> +
>  	regmap_read(pca->regmap, PCA9685_MODE2, &mode2);
>  
>  	if (device_property_read_bool(&client->dev, "invert"))
> -- 
> 2.10.2
> 

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

Re: [PATCH 2/2] pwm: pca9685: fix prescaler initialization

[Andy Shevchenko]

On Wed, 2017-01-18 at 11:57 +0100, Thierry Reding wrote:
> On Tue, Dec 13, 2016 at 04:52:51PM +0100, Clemens Gruber wrote:
> > Until now, we assumed that the period is the hardware default of
> > 1/200Hz
> > at probe time, but if the period was changed and the user reboots,
> > this
> > assumption is wrong.
> > 
> > Solution: Check if the prescaler is set to the hardware default. If
> > not,
> > reprogram the prescaler at first configuration.

AFAIU the PWM is off until user space or kernel user enables it
explicitly after reboot. Is it correct?

In such case we may just enforce default period to be one we are
expecting.

> > 
> > Cc: <stable@vger.kernel.org> # v4.3+
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> >  drivers/pwm/pwm-pca9685.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Cc'ing Mika and Andy and quoting verbatim for review.
> 
> Thierry
> 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 01a6a83..efc657e 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -55,6 +55,7 @@
> >  #define PCA9685_PRESCALE	0xFE
> >  
> >  #define PCA9685_PRESCALE_MIN	0x03	/* => max.
> > frequency of 1526 Hz */
> > +#define PCA9685_PRESCALE_DEF	0x1E	/* => default
> > frequency of 200 Hz */
> >  #define PCA9685_PRESCALE_MAX	0xFF	/* => min.
> > frequency of 24 Hz */
> >  
> >  #define PCA9685_COUNTER_RANGE	4096
> > @@ -289,8 +290,8 @@ static int pca9685_pwm_probe(struct i2c_client
> > *client,
> >  				const struct i2c_device_id *id)
> >  {
> >  	struct pca9685 *pca;
> > +	int prescale, mode2;
> >  	int ret;
> > -	int mode2;
> >  
> >  	pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
> >  	if (!pca)
> > @@ -304,10 +305,15 @@ static int pca9685_pwm_probe(struct i2c_client
> > *client,
> >  		return ret;
> >  	}
> >  	pca->duty_ns = 0;
> > -	pca->period_ns = PCA9685_DEFAULT_PERIOD;
> >  
> >  	i2c_set_clientdata(client, pca);
> >  
> > +	regmap_read(pca->regmap, PCA9685_PRESCALE, &prescale);
> > +	if (prescale == PCA9685_PRESCALE_DEF)
> > +		pca->period_ns = PCA9685_DEFAULT_PERIOD;
> > +	else
> > +		pca->period_ns = 0;
> > +
> >  	regmap_read(pca->regmap, PCA9685_MODE2, &mode2);
> >  
> >  	if (device_property_read_bool(&client->dev, "invert"))
> > -- 
> > 2.10.2
> > 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] pwm: pca9685: fix period change with same duty cycle

[Mika Westerberg]

On Wed, Jan 18, 2017 at 11:56:50AM +0100, Thierry Reding wrote:
> On Tue, Dec 13, 2016 at 04:52:50PM +0100, Clemens Gruber wrote:
> > When first implementing support for changing the output frequency, an
> > optimization was added to continue the PWM after changing the prescaler
> > without having to reprogram the ON and OFF registers for the duty cycle,
> > in case the duty cycle stayed the same.
> > This was flawed, because we compared the absolute value of the duty
> > cycle in nanoseconds instead of the ratio to the period.
> > 
> > Fix the problem by removing the shortcut.
> > 
> > Fixes: 01ec8472009c9 ("pwm-pca9685: Support changing the output frequency")
> > Cc: <stable@vger.kernel.org> # v4.3+
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> >  drivers/pwm/pwm-pca9685.c | 11 -----------
> >  1 file changed, 11 deletions(-)
> 
> Mika, Andy, can you guys help review this and 2/2?

Looks good to me.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 2/2] pwm: pca9685: fix prescaler initialization

[Clemens Gruber]

On Wed, Jan 18, 2017 at 01:09:24PM +0200, Andy Shevchenko wrote:
> On Wed, 2017-01-18 at 11:57 +0100, Thierry Reding wrote:
> > On Tue, Dec 13, 2016 at 04:52:51PM +0100, Clemens Gruber wrote:
> > > Until now, we assumed that the period is the hardware default of
> > > 1/200Hz
> > > at probe time, but if the period was changed and the user reboots,
> > > this
> > > assumption is wrong.
> > > 
> > > Solution: Check if the prescaler is set to the hardware default. If
> > > not,
> > > reprogram the prescaler at first configuration.
> 
> AFAIU the PWM is off until user space or kernel user enables it
> explicitly after reboot. Is it correct?

Yes, but the period could be different, maybe modified in the bootloader
or at a previous boot without hardware reset in between. (We do not send
a SWRST to the chip, so the period register could be different)

Until now, we assumed it is always 1/200 Hz and skipped the lengthy
prescale configuration (put chip into sleep mode, set prescaler, go out
of sleep mode, udelay for 0.5ms until the oscillator is back up) if the
user wants a period of 1/200 Hz.

With this patch, we check if it is in fact set to the hardware default.
If not, we set pca->period_ns to 0 which leads to changing the prescaler
in the next call to pca9685_pwm_config.

Thanks,
Clemens

Re: [PATCH 2/2] pwm: pca9685: fix prescaler initialization

[Andy Shevchenko]

On Wed, 2017-01-18 at 14:53 +0100, Clemens Gruber wrote:
> On Wed, Jan 18, 2017 at 01:09:24PM +0200, Andy Shevchenko wrote:
> > On Wed, 2017-01-18 at 11:57 +0100, Thierry Reding wrote:
> > > On Tue, Dec 13, 2016 at 04:52:51PM +0100, Clemens Gruber wrote:
> > > > Until now, we assumed that the period is the hardware default of
> > > > 1/200Hz
> > > > at probe time, but if the period was changed and the user
> > > > reboots,
> > > > this
> > > > assumption is wrong.
> > > > 
> > > > Solution: Check if the prescaler is set to the hardware default.
> > > > If
> > > > not,
> > > > reprogram the prescaler at first configuration.
> > 
> > AFAIU the PWM is off until user space or kernel user enables it
> > explicitly after reboot. Is it correct?
> 
> Yes, but the period could be different, maybe modified in the
> bootloader
> or at a previous boot without hardware reset in between. (We do not
> send
> a SWRST to the chip, so the period register could be different)

It's fragile to rely on some external settings, right? Wouldn't be
better to leave device in a known state after ->probe()?

> Until now, we assumed it is always 1/200 Hz and skipped the lengthy
> prescale configuration (put chip into sleep mode, set prescaler, go
> out
> of sleep mode, udelay for 0.5ms until the oscillator is back up) if
> the
> user wants a period of 1/200 Hz.
> 
> With this patch, we check if it is in fact set to the hardware
> default.
> If not, we set pca->period_ns to 0 which leads to changing the
> prescaler
> in the next call to pca9685_pwm_config.

And this contradicts, for my opinion, to the logic you referred in the
first paragraph. If you would like to use preset values, you need to
read and recalculate period correctly.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] pwm: pca9685: fix period change with same duty cycle

[Thierry Reding]

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

On Tue, Dec 13, 2016 at 04:52:50PM +0100, Clemens Gruber wrote:
> When first implementing support for changing the output frequency, an
> optimization was added to continue the PWM after changing the prescaler
> without having to reprogram the ON and OFF registers for the duty cycle,
> in case the duty cycle stayed the same.
> This was flawed, because we compared the absolute value of the duty
> cycle in nanoseconds instead of the ratio to the period.
> 
> Fix the problem by removing the shortcut.
> 
> Fixes: 01ec8472009c9 ("pwm-pca9685: Support changing the output
> frequency")
> Cc: <stable@vger.kernel.org> # v4.3+
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
>  drivers/pwm/pwm-pca9685.c | 11 -----------
>  1 file changed, 11 deletions(-)

Applied, thanks.

Thierry

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