| Message ID | 20181107093613.26734-2-peron.clem@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [1/2] pwm: kconfig: enable kona pwm to be built for cygnus arch | expand |
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
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/ |
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
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.
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(-) I see v2 and v3 of patch 1 of the series. Did you work on v2 of this patch (2/2) as well? I don't see it on the list and it didn't seem like everyone was happy with this version. Thierry
Hi Thierry, On Wed, 12 Dec 2018 at 12:04, Thierry Reding <thierry.reding@gmail.com> 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(-) > > I see v2 and v3 of patch 1 of the series. Did you work on v2 of this > patch (2/2) as well? I don't see it on the list and it didn't seem like > everyone was happy with this version. No, I dropped it. Was working on an atomic convert of the driver but I don't have access to my cygnus board anymore. Regards, Clement > > Thierry
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;