[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 2/2] pwm: pca9685: fix prescaler initialization

[Clemens Gruber]

On Wed, Jan 18, 2017 at 04:01:58PM +0200, Andy Shevchenko wrote:
> On Wed, 2017-01-18 at 14:53 +0100, Clemens Gruber wrote:
> > 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()?

Yes, that's what this patch tries to solve by verifying that the
external setting (the prescale register) is set to its hardware default
value of 0x1E (corresponding to a period of 1/200 Hz).
If it is not 0x1E, the driver will reconfigure the prescaler according
to the desired period at the time of the next configuration.

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

I don't want to use preset values. I wanted to be sure that we do not
skip the prescaler configuration if the prescale register was modified.
We should only skip it, if it is already at 0x1E.

Thanks,
Clemens

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

[Andy Shevchenko]

On Wed, 2017-01-18 at 15:25 +0100, Clemens Gruber wrote:
> On Wed, Jan 18, 2017 at 04:01:58PM +0200, Andy Shevchenko wrote:
> > On Wed, 2017-01-18 at 14:53 +0100, Clemens Gruber wrote:
> > > 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()?
> 
> Yes, that's what this patch tries to solve by verifying that the
> external setting (the prescale register) is set to its hardware
> default
> value of 0x1E (corresponding to a period of 1/200 Hz).
> If it is not 0x1E, the driver will reconfigure the prescaler according
> to the desired period at the time of the next configuration.

Yes, and my question is what is possible go wrong if you just enforce
prescaler to be 1/200Hz?

> > > 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.
> 
> I don't want to use preset values. I wanted to be sure that we do not
> skip the prescaler configuration if the prescale register was
> modified.
> We should only skip it, if it is already at 0x1E.

Same question here.

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

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

[Clemens Gruber]

On Thu, Jan 19, 2017 at 02:34:39PM +0200, Andy Shevchenko wrote:
> On Wed, 2017-01-18 at 15:25 +0100, Clemens Gruber wrote:
> > Yes, that's what this patch tries to solve by verifying that the
> > external setting (the prescale register) is set to its hardware
> > default
> > value of 0x1E (corresponding to a period of 1/200 Hz).
> > If it is not 0x1E, the driver will reconfigure the prescaler according
> > to the desired period at the time of the next configuration.
> 
> Yes, and my question is what is possible go wrong if you just enforce
> prescaler to be 1/200Hz?

If we enforce a default of 1 / 200 Hz, we have to go through the SLEEP
mode and udelay for 0.5ms once for our default and then again for the
user, if he does not want a period of 1 / 200 Hz.
-> Number of prescaler changes: 1 or 2

I think it is better as it is now + my patch applied: We verify if the
prescaler is already set to 1 / 200 Hz.
Then, as soon as the user configures his PWM channels, we either do not
have to change the prescaler at all (if he wants 1 / 200 Hz) or do it
once at the time of configuration.
-> Number of prescaler changes: 0 or 1

What's the advantage of enforcing the prescaler to 1 / 200 Hz in the
probe function when we do not know yet if 1 / 200 Hz is the period the
user is going to configure?

Thanks,
Clemens

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

[Andy Shevchenko]

On Thu, 2017-01-19 at 15:49 +0100, Clemens Gruber wrote:
> On Thu, Jan 19, 2017 at 02:34:39PM +0200, Andy Shevchenko wrote:
> > On Wed, 2017-01-18 at 15:25 +0100, Clemens Gruber wrote:
> > > Yes, that's what this patch tries to solve by verifying that the
> > > external setting (the prescale register) is set to its hardware
> > > default
> > > value of 0x1E (corresponding to a period of 1/200 Hz).
> > > If it is not 0x1E, the driver will reconfigure the prescaler
> > > according
> > > to the desired period at the time of the next configuration.
> > 
> > Yes, and my question is what is possible go wrong if you just
> > enforce
> > prescaler to be 1/200Hz?
> 
> If we enforce a default of 1 / 200 Hz, we have to go through the SLEEP
> mode and udelay for 0.5ms once for our default and then again for the
> user, if he does not want a period of 1 / 200 Hz.
> -> Number of prescaler changes: 1 or 2
> 
> I think it is better as it is now + my patch applied: We verify if the
> prescaler is already set to 1 / 200 Hz.
> Then, as soon as the user configures his PWM channels, we either do
> not
> have to change the prescaler at all (if he wants 1 / 200 Hz) or do it
> once at the time of configuration.
> -> Number of prescaler changes: 0 or 1
> 
> What's the advantage of enforcing the prescaler to 1 / 200 Hz in the
> probe function when we do not know yet if 1 / 200 Hz is the period the
> user is going to configure?

Advantage of this proposal is to get to known state.
Combining with your proposal I would see the best approach is to set
pca->period_ns accordingly to current prescaler value if you want to.

In your patch I see no benefit, since it's quite unlikely user will want
to have 5ms period among all possibilities.

The 500us delay at the _first_ configuration is not a big deal.

So, summarize, I prefer (in order of preference from high to low):
- enforce default prescaler value based on default period (it's just one
line to write a register)
- calculate default period based on actual prescaler value
- your or similar solution

If you still disagree with that, I rest my case.

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

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

[Clemens Gruber]

On Thu, Jan 19, 2017 at 06:10:08PM +0200, Andy Shevchenko wrote:
> Combining with your proposal I would see the best approach is to set
> pca->period_ns accordingly to current prescaler value if you want to.

Yes, I agree.

> In your patch I see no benefit, since it's quite unlikely user will want
> to have 5ms period among all possibilities.

It is the hardware default, but you are right, the user probably does
not care about that.

> So, summarize, I prefer (in order of preference from high to low):
> - enforce default prescaler value based on default period (it's just one
> line to write a register)
> - calculate default period based on actual prescaler value

Let's go with this one. I'll spin up a v2 where I calculate the
period_ns value from the current prescaler value in the probe function.

--

Thierry: I think you could merge v1 of patch 1/2 from my series
independently.

I'll send v2 of patch 2/2 with aforementioned changes in the next days.

Thanks,
Clemens

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

[Andy Shevchenko]

On Thu, Jan 19, 2017 at 6:52 PM, Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
> On Thu, Jan 19, 2017 at 06:10:08PM +0200, Andy Shevchenko wrote:

>> So, summarize, I prefer (in order of preference from high to low):
>> - enforce default prescaler value based on default period (it's just one
>> line to write a register)
>> - calculate default period based on actual prescaler value
>
> Let's go with this one. I'll spin up a v2 where I calculate the
> period_ns value from the current prescaler value in the probe function.

I'm fine with it. Thanks!

-- 
With Best Regards,
Andy Shevchenko

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

[Thierry Reding]

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

On Thu, Jan 19, 2017 at 05:52:10PM +0100, Clemens Gruber wrote:
> On Thu, Jan 19, 2017 at 06:10:08PM +0200, Andy Shevchenko wrote:
> > Combining with your proposal I would see the best approach is to set
> > pca->period_ns accordingly to current prescaler value if you want to.
> 
> Yes, I agree.
> 
> > In your patch I see no benefit, since it's quite unlikely user will want
> > to have 5ms period among all possibilities.
> 
> It is the hardware default, but you are right, the user probably does
> not care about that.
> 
> > So, summarize, I prefer (in order of preference from high to low):
> > - enforce default prescaler value based on default period (it's just one
> > line to write a register)
> > - calculate default period based on actual prescaler value
> 
> Let's go with this one. I'll spin up a v2 where I calculate the
> period_ns value from the current prescaler value in the probe function.

This effectively ends up duplicating much of what the atomic API is
supposed to be doing.

So generally before the atomic API there is no way for the PWM driver
(and consequently the PWM users) to know what the current setting is.
That implies that we either can't care about the settings that were
programmed by some bootloader or that we force the PWM to a setting
on ->probe().

The result is inconsistent behaviour across drivers, and that's just
fine for many cases. But for some cases it is really not something that
we can work with.

So perhaps another possibility would be for this driver to implement the
atomic API. This should give you the necessary infrastructure to do
exactly what you want.

> Thierry: I think you could merge v1 of patch 1/2 from my series
> independently.

Okay, will do.

> I'll send v2 of patch 2/2 with aforementioned changes in the next
> days.

Like I said above, I think atomic API conversion wouldn't be very
difficult for this driver and it has the added advantage of giving you
the proper infrastructure to do this rather than having to duplicate
it in the driver.

That would be my preference, but I'm willing to take v2 of 2/2 as well
if it ends up being really nice and compact. =)

Thierry

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

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

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

[Andy Shevchenko]

On Fri, 2017-01-20 at 07:39 +0100, Thierry Reding wrote:
> On Thu, Jan 19, 2017 at 05:52:10PM +0100, Clemens Gruber wrote:
> > On Thu, Jan 19, 2017 at 06:10:08PM +0200, Andy Shevchenko wrote:
> > > Combining with your proposal I would see the best approach is to
> > > set
> > > pca->period_ns accordingly to current prescaler value if you want
> > > to.

> > I'll send v2 of patch 2/2 with aforementioned changes in the next
> > days.
> 
> Like I said above, I think atomic API conversion wouldn't be very
> difficult for this driver and it has the added advantage of giving you
> the proper infrastructure to do this rather than having to duplicate
> it in the driver.
> 
> That would be my preference, but I'm willing to take v2 of 2/2 as well
> if it ends up being really nice and compact. =)

I think we may split to this fix and separate change to move to atomic
API. Does it sound reasonable?

P.S. <offtopic>Can you comment further pwm-lpss changes Mika and I
did?</offtopic>

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

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

[Clemens Gruber]

On Fri, Jan 20, 2017 at 07:39:07AM +0100, Thierry Reding wrote:
> On Thu, Jan 19, 2017 at 05:52:10PM +0100, Clemens Gruber wrote:
> > Let's go with this one. I'll spin up a v2 where I calculate the
> > period_ns value from the current prescaler value in the probe function.
> 
> This effectively ends up duplicating much of what the atomic API is
> supposed to be doing.
> 
> So generally before the atomic API there is no way for the PWM driver
> (and consequently the PWM users) to know what the current setting is.
> That implies that we either can't care about the settings that were
> programmed by some bootloader or that we force the PWM to a setting
> on ->probe().
> 
> The result is inconsistent behaviour across drivers, and that's just
> fine for many cases. But for some cases it is really not something that
> we can work with.
> 
> So perhaps another possibility would be for this driver to implement the
> atomic API. This should give you the necessary infrastructure to do
> exactly what you want.

Sounds good. I'll work on that.

One oddity remains though:
The chip has one prescaler, so changing the period of one channel
changes the period of all other channels as well.
If somebody configures different periods for each channel, the last one
wins, but the PWM core still stores the period values the user set
initially. Should we update the period in pwm_state of all pwms in the
chip if the period of one pwm is changed?
Then, when the user calls pwm_get_state, the pwm_state would contain the
correct period?

Thanks,
Clemens