[2/2] pwm: bcm-kona: apply pwm settings on enable

Message ID 20181107093613.26734-2-peron.clem@gmail.com
State New
Headers show
Series
  • [1/2] pwm: kconfig: enable kona pwm to be built for cygnus arch
Related show

Commit Message

Clément Péron Nov. 7, 2018, 9:36 a.m.
From: Suji Velupillai <suji.velupillai@broadcom.com>

When pwm_bl framework calls enable, a call to pwm_is_enabled(pwm) still
return false, this prevents the backlight being turn on at boot time.

Signed-off-by: Suji Velupillai <suji.velupillai@broadcom.com>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/pwm/pwm-bcm-kona.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Uwe Kleine-König Nov. 7, 2018, 4:29 p.m. | #1
Hello,

On Wed, Nov 07, 2018 at 10:36:13AM +0100, Clément Péron wrote:
> From: Suji Velupillai <suji.velupillai@broadcom.com>
> 
> When pwm_bl framework calls enable, a call to pwm_is_enabled(pwm) still
> return false, this prevents the backlight being turn on at boot time.
> 
> Signed-off-by: Suji Velupillai <suji.velupillai@broadcom.com>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  drivers/pwm/pwm-bcm-kona.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> index 09a95aeb3a70..d991d53c4b38 100644
> --- a/drivers/pwm/pwm-bcm-kona.c
> +++ b/drivers/pwm/pwm-bcm-kona.c
> @@ -108,8 +108,8 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
>  	ndelay(400);
>  }
>  
> -static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			    int duty_ns, int period_ns)
> +static int __pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 int duty_ns, int period_ns, bool pwmc_enabled)
>  {
>  	struct kona_pwmc *kp = to_kona_pwmc(chip);
>  	u64 val, div, rate;
> @@ -155,7 +155,7 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * always calculated above to ensure the new values are
>  	 * validated immediately instead of on enable.
>  	 */
> -	if (pwm_is_enabled(pwm)) {
> +	if (pwm_is_enabled(pwm) || pwmc_enabled) {

Having pwm-API-calls in hw-drivers is ugly. Apart from not giving the
intended return code this function should IMHO be reserved to pwm
consumers. The underlaying problem is that pwm-bl does:

	pwm_config(pwm, duty_cycle, period);
	pwm_enable(pwm);

and expects that the duty_cycle and period is used then. Doesn't
everything works just fine if the if-block is always executed?

The better fix here would be to convert the driver to the atomic API
(i.e. implement .apply instead of .config, .set_polarity, .enable and
.disable).

Alternatively in .enable ensure that the hardware is programmed with the
parameters from pwm->state. (But converting to the atomic API is the
better approach.)

Best regards
Uwe
Clément Péron Nov. 8, 2018, 10:47 a.m. | #2
Hi Uwe,

On Wed, 7 Nov 2018 at 17:29, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Wed, Nov 07, 2018 at 10:36:13AM +0100, Clément Péron wrote:
> > From: Suji Velupillai <suji.velupillai@broadcom.com>
> >
> > When pwm_bl framework calls enable, a call to pwm_is_enabled(pwm) still
> > return false, this prevents the backlight being turn on at boot time.
> >
> > Signed-off-by: Suji Velupillai <suji.velupillai@broadcom.com>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >  drivers/pwm/pwm-bcm-kona.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> > index 09a95aeb3a70..d991d53c4b38 100644
> > --- a/drivers/pwm/pwm-bcm-kona.c
> > +++ b/drivers/pwm/pwm-bcm-kona.c
> > @@ -108,8 +108,8 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> >       ndelay(400);
> >  }
> >
> > -static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > -                         int duty_ns, int period_ns)
> > +static int __pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                      int duty_ns, int period_ns, bool pwmc_enabled)
> >  {
> >       struct kona_pwmc *kp = to_kona_pwmc(chip);
> >       u64 val, div, rate;
> > @@ -155,7 +155,7 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >        * always calculated above to ensure the new values are
> >        * validated immediately instead of on enable.
> >        */
> > -     if (pwm_is_enabled(pwm)) {
> > +     if (pwm_is_enabled(pwm) || pwmc_enabled) {
>
> Having pwm-API-calls in hw-drivers is ugly. Apart from not giving the
> intended return code this function should IMHO be reserved to pwm
> consumers. The underlaying problem is that pwm-bl does:
>
>         pwm_config(pwm, duty_cycle, period);
>         pwm_enable(pwm);
>
> and expects that the duty_cycle and period is used then. Doesn't
> everything works just fine if the if-block is always executed?

Tested and works fine for me. But I only have a Cygnus proc.
Maybe there is some issue with Kona as explained by the comment (even
if I don't understand it well).

 * Don't apply settings if disabled. The period and duty cycle are
 * always calculated above to ensure the new values are
 * validated immediately instead of on enable.

Regards,
Clement

>
> The better fix here would be to convert the driver to the atomic API
> (i.e. implement .apply instead of .config, .set_polarity, .enable and
> .disable).
>
> Alternatively in .enable ensure that the hardware is programmed with the
> parameters from pwm->state. (But converting to the atomic API is the
> better approach.)
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Uwe Kleine-König Nov. 8, 2018, 10:59 a.m. | #3
Hello,

adding Tim Kryger as the initial author of the bcm-kona driver to Cc:.
Maybe he can shed some light to the questions below?

On Thu, Nov 08, 2018 at 11:47:17AM +0100, Clément Péron wrote:
> On Wed, 7 Nov 2018 at 17:29, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Nov 07, 2018 at 10:36:13AM +0100, Clément Péron wrote:
> > > From: Suji Velupillai <suji.velupillai@broadcom.com>
> > >
> > > When pwm_bl framework calls enable, a call to pwm_is_enabled(pwm) still
> > > return false, this prevents the backlight being turn on at boot time.
> > >
> > > Signed-off-by: Suji Velupillai <suji.velupillai@broadcom.com>
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > >  drivers/pwm/pwm-bcm-kona.c | 16 +++++++++++-----
> > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> > > index 09a95aeb3a70..d991d53c4b38 100644
> > > --- a/drivers/pwm/pwm-bcm-kona.c
> > > +++ b/drivers/pwm/pwm-bcm-kona.c
> > > @@ -108,8 +108,8 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> > >       ndelay(400);
> > >  }
> > >
> > > -static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > -                         int duty_ns, int period_ns)
> > > +static int __pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +                      int duty_ns, int period_ns, bool pwmc_enabled)
> > >  {
> > >       struct kona_pwmc *kp = to_kona_pwmc(chip);
> > >       u64 val, div, rate;
> > > @@ -155,7 +155,7 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > >        * always calculated above to ensure the new values are
> > >        * validated immediately instead of on enable.
> > >        */
> > > -     if (pwm_is_enabled(pwm)) {
> > > +     if (pwm_is_enabled(pwm) || pwmc_enabled) {
> >
> > Having pwm-API-calls in hw-drivers is ugly. Apart from not giving the
> > intended return code this function should IMHO be reserved to pwm
> > consumers. The underlaying problem is that pwm-bl does:
> >
> >         pwm_config(pwm, duty_cycle, period);
> >         pwm_enable(pwm);
> >
> > and expects that the duty_cycle and period is used then. Doesn't
> > everything works just fine if the if-block is always executed?
> 
> Tested and works fine for me. But I only have a Cygnus proc.
> Maybe there is some issue with Kona as explained by the comment (even
> if I don't understand it well).
> 
>  * Don't apply settings if disabled. The period and duty cycle are
>  * always calculated above to ensure the new values are
>  * validated immediately instead of on enable.

I wouldn't understand that as "If you apply settings on a disabled PWM a
kitten dies". I think it only means: At the current point in time
duty_cycle and period are not important as the hardware is off. So don't
bother to write these values to the hardware.

@Tim: Do you think (or can test if) there is a problem when doing

-       if (pwm_is_enabled(pwm)) {
+       if (1) {

in kona_pwmc_config? (For sure the comment needs adaption and the if (1)
shouldn't make it into the driver, just used that as shorthand for the
change I want to suggest.)

But still better than dropping the check is to convert the driver to the
atomic API. With that this problem would simply not occur.

Best regards
Uwe
Tim Kryger Nov. 8, 2018, 3:32 p.m. | #4
On Thu, Nov 8, 2018 at 2:59 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> adding Tim Kryger as the initial author of the bcm-kona driver to Cc:.
> Maybe he can shed some light to the questions below?
>
> On Thu, Nov 08, 2018 at 11:47:17AM +0100, Clément Péron wrote:
> > On Wed, 7 Nov 2018 at 17:29, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Wed, Nov 07, 2018 at 10:36:13AM +0100, Clément Péron wrote:
> > > > From: Suji Velupillai <suji.velupillai@broadcom.com>
> > > >
> > > > When pwm_bl framework calls enable, a call to pwm_is_enabled(pwm) still
> > > > return false, this prevents the backlight being turn on at boot time.
> > > >
> > > > Signed-off-by: Suji Velupillai <suji.velupillai@broadcom.com>
> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > ---
> > > >  drivers/pwm/pwm-bcm-kona.c | 16 +++++++++++-----
> > > >  1 file changed, 11 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
> > > > index 09a95aeb3a70..d991d53c4b38 100644
> > > > --- a/drivers/pwm/pwm-bcm-kona.c
> > > > +++ b/drivers/pwm/pwm-bcm-kona.c
> > > > @@ -108,8 +108,8 @@ static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
> > > >       ndelay(400);
> > > >  }
> > > >
> > > > -static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > -                         int duty_ns, int period_ns)
> > > > +static int __pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +                      int duty_ns, int period_ns, bool pwmc_enabled)
> > > >  {
> > > >       struct kona_pwmc *kp = to_kona_pwmc(chip);
> > > >       u64 val, div, rate;
> > > > @@ -155,7 +155,7 @@ static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >        * always calculated above to ensure the new values are
> > > >        * validated immediately instead of on enable.
> > > >        */
> > > > -     if (pwm_is_enabled(pwm)) {
> > > > +     if (pwm_is_enabled(pwm) || pwmc_enabled) {
> > >
> > > Having pwm-API-calls in hw-drivers is ugly. Apart from not giving the
> > > intended return code this function should IMHO be reserved to pwm
> > > consumers. The underlaying problem is that pwm-bl does:
> > >
> > >         pwm_config(pwm, duty_cycle, period);
> > >         pwm_enable(pwm);
> > >
> > > and expects that the duty_cycle and period is used then. Doesn't
> > > everything works just fine if the if-block is always executed?
> >
> > Tested and works fine for me. But I only have a Cygnus proc.
> > Maybe there is some issue with Kona as explained by the comment (even
> > if I don't understand it well).
> >
> >  * Don't apply settings if disabled. The period and duty cycle are
> >  * always calculated above to ensure the new values are
> >  * validated immediately instead of on enable.
>
> I wouldn't understand that as "If you apply settings on a disabled PWM a
> kitten dies". I think it only means: At the current point in time
> duty_cycle and period are not important as the hardware is off. So don't
> bother to write these values to the hardware.
>
> @Tim: Do you think (or can test if) there is a problem when doing
>
> -       if (pwm_is_enabled(pwm)) {
> +       if (1) {
>
> in kona_pwmc_config? (For sure the comment needs adaption and the if (1)
> shouldn't make it into the driver, just used that as shorthand for the
> change I want to suggest.)
>
> But still better than dropping the check is to convert the driver to the
> atomic API. With that this problem would simply not occur.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

There is no per channel disable in the hardware so to simulate a
disable, duty is set to zero.

The check is there to prevent new config values from applying until
the channel is enabled.

Patch

diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
index 09a95aeb3a70..d991d53c4b38 100644
--- a/drivers/pwm/pwm-bcm-kona.c
+++ b/drivers/pwm/pwm-bcm-kona.c
@@ -108,8 +108,8 @@  static void kona_pwmc_apply_settings(struct kona_pwmc *kp, unsigned int chan)
 	ndelay(400);
 }
 
-static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			    int duty_ns, int period_ns)
+static int __pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			 int duty_ns, int period_ns, bool pwmc_enabled)
 {
 	struct kona_pwmc *kp = to_kona_pwmc(chip);
 	u64 val, div, rate;
@@ -155,7 +155,7 @@  static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * always calculated above to ensure the new values are
 	 * validated immediately instead of on enable.
 	 */
-	if (pwm_is_enabled(pwm)) {
+	if (pwm_is_enabled(pwm) || pwmc_enabled) {
 		kona_pwmc_prepare_for_settings(kp, chan);
 
 		value = readl(kp->base + PRESCALE_OFFSET);
@@ -173,6 +173,12 @@  static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			    int duty_ns, int period_ns)
+{
+	return __pwmc_config(chip, pwm, duty_ns, period_ns, false);
+}
+
 static int kona_pwmc_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
 				  enum pwm_polarity polarity)
 {
@@ -216,8 +222,8 @@  static int kona_pwmc_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 		return ret;
 	}
 
-	ret = kona_pwmc_config(chip, pwm, pwm_get_duty_cycle(pwm),
-			       pwm_get_period(pwm));
+	ret = __pwmc_config(chip, pwm, pwm_get_duty_cycle(pwm),
+			    pwm_get_period(pwm), true);
 	if (ret < 0) {
 		clk_disable_unprepare(kp->clk);
 		return ret;