2018-11-07 09:37:16

by Clément Péron

[permalink] [raw]
Subject: [PATCH 1/2] pwm: kconfig: enable kona pwm to be built for cygnus arch

The Cygnus architecture use a Kona PWM. This is already present
in the device tree but can't be built actually. Hence, allow the
Kona PWM to be built for Cygnus arch.

Signed-off-by: Clément Péron <[email protected]>
---
drivers/pwm/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 763ee50ea57d..76d56fc8b1b7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -88,7 +88,8 @@ config PWM_BCM_IPROC

config PWM_BCM_KONA
tristate "Kona PWM support"
- depends on ARCH_BCM_MOBILE
+ depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS
+ default ARCH_BCM_CYGNUS
help
Generic PWM framework driver for Broadcom Kona PWM block.

--
2.19.1



2018-11-07 09:37:52

by Clément Péron

[permalink] [raw]
Subject: [PATCH 2/2] pwm: bcm-kona: apply pwm settings on enable

From: Suji Velupillai <[email protected]>

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 <[email protected]>
Signed-off-by: Clément Péron <[email protected]>
---
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) {
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;
--
2.19.1


2018-11-07 16:14:44

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH 1/2] pwm: kconfig: enable kona pwm to be built for cygnus arch

On Wed, Nov 07, 2018 at 10:36:12AM +0100, Cl?ment P?ron wrote:
> The Cygnus architecture use a Kona PWM. This is already present
> in the device tree but can't be built actually. Hence, allow the
> Kona PWM to be built for Cygnus arch.
>
> Signed-off-by: Cl?ment P?ron <[email protected]>
> ---
> drivers/pwm/Kconfig | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 763ee50ea57d..76d56fc8b1b7 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -88,7 +88,8 @@ config PWM_BCM_IPROC
>
> config PWM_BCM_KONA
> tristate "Kona PWM support"
> - depends on ARCH_BCM_MOBILE
> + depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS
> + default ARCH_BCM_CYGNUS

Is it possible to build this driver also on other arches? Then you might
want to consider

depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

2018-11-07 16:29:51

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH 2/2] pwm: bcm-kona: apply pwm settings on enable

Hello,

On Wed, Nov 07, 2018 at 10:36:13AM +0100, Cl?ment P?ron wrote:
> From: Suji Velupillai <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Cl?ment P?ron <[email protected]>
> ---
> 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

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

2018-11-07 16:50:53

by Scott Branden

[permalink] [raw]
Subject: Re: [PATCH 1/2] pwm: kconfig: enable kona pwm to be built for cygnus arch


On 2018-11-07 8:12 a.m., Uwe Kleine-König wrote:
> On Wed, Nov 07, 2018 at 10:36:12AM +0100, Clément Péron wrote:
>> The Cygnus architecture use a Kona PWM. This is already present
>> in the device tree but can't be built actually. Hence, allow the
>> Kona PWM to be built for Cygnus arch.
>>
>> Signed-off-by: Clément Péron <[email protected]>
>> ---
>> drivers/pwm/Kconfig | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 763ee50ea57d..76d56fc8b1b7 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -88,7 +88,8 @@ config PWM_BCM_IPROC
>>
>> config PWM_BCM_KONA
>> tristate "Kona PWM support"
>> - depends on ARCH_BCM_MOBILE
>> + depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS
>> + default ARCH_BCM_CYGNUS
> Is it possible to build this driver also on other arches? Then you might
> want to consider
>
> depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST
good idea to add the COMPILE_TEST just to increase compile coverage
>
> Best regards
> Uwe
>

2018-11-08 10:48:08

by Clément Péron

[permalink] [raw]
Subject: Re: [PATCH 2/2] pwm: bcm-kona: apply pwm settings on enable

Hi Uwe,

On Wed, 7 Nov 2018 at 17:29, Uwe Kleine-König
<[email protected]> wrote:
>
> Hello,
>
> On Wed, Nov 07, 2018 at 10:36:13AM +0100, Clément Péron wrote:
> > From: Suji Velupillai <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Clément Péron <[email protected]>
> > ---
> > 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/ |

2018-11-08 10:48:45

by Clément Péron

[permalink] [raw]
Subject: Re: [PATCH 1/2] pwm: kconfig: enable kona pwm to be built for cygnus arch

Hi Uwe, Scott,

On Wed, 7 Nov 2018 at 17:48, Scott Branden <[email protected]> wrote:
>
>
> On 2018-11-07 8:12 a.m., Uwe Kleine-König wrote:
> > On Wed, Nov 07, 2018 at 10:36:12AM +0100, Clément Péron wrote:
> >> The Cygnus architecture use a Kona PWM. This is already present
> >> in the device tree but can't be built actually. Hence, allow the
> >> Kona PWM to be built for Cygnus arch.
> >>
> >> Signed-off-by: Clément Péron <[email protected]>
> >> ---
> >> drivers/pwm/Kconfig | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index 763ee50ea57d..76d56fc8b1b7 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -88,7 +88,8 @@ config PWM_BCM_IPROC
> >>
> >> config PWM_BCM_KONA
> >> tristate "Kona PWM support"
> >> - depends on ARCH_BCM_MOBILE
> >> + depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS
> >> + default ARCH_BCM_CYGNUS
> > Is it possible to build this driver also on other arches? Then you might
> > want to consider
> >
> > depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST
> good idea to add the COMPILE_TEST just to increase compile coverage

Will Do.

Thanks,
Clement

> >
> > Best regards
> > Uwe
> >

2018-11-08 11:00:06

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH 2/2] pwm: bcm-kona: apply pwm settings on enable

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
> <[email protected]> wrote:
> > On Wed, Nov 07, 2018 at 10:36:13AM +0100, Cl?ment P?ron wrote:
> > > From: Suji Velupillai <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Cl?ment P?ron <[email protected]>
> > > ---
> > > 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/ |

2018-11-08 12:23:36

by Thierry Reding

[permalink] [raw]
Subject: Re: [PATCH 1/2] pwm: kconfig: enable kona pwm to be built for cygnus arch

On Thu, Nov 08, 2018 at 11:47:53AM +0100, Clément Péron wrote:
> Hi Uwe, Scott,
>
> On Wed, 7 Nov 2018 at 17:48, Scott Branden <[email protected]> wrote:
> >
> >
> > On 2018-11-07 8:12 a.m., Uwe Kleine-König wrote:
> > > On Wed, Nov 07, 2018 at 10:36:12AM +0100, Clément Péron wrote:
> > >> The Cygnus architecture use a Kona PWM. This is already present
> > >> in the device tree but can't be built actually. Hence, allow the
> > >> Kona PWM to be built for Cygnus arch.
> > >>
> > >> Signed-off-by: Clément Péron <[email protected]>
> > >> ---
> > >> drivers/pwm/Kconfig | 3 ++-
> > >> 1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > >> index 763ee50ea57d..76d56fc8b1b7 100644
> > >> --- a/drivers/pwm/Kconfig
> > >> +++ b/drivers/pwm/Kconfig
> > >> @@ -88,7 +88,8 @@ config PWM_BCM_IPROC
> > >>
> > >> config PWM_BCM_KONA
> > >> tristate "Kona PWM support"
> > >> - depends on ARCH_BCM_MOBILE
> > >> + depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS
> > >> + default ARCH_BCM_CYGNUS
> > > Is it possible to build this driver also on other arches? Then you might
> > > want to consider
> > >
> > > depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST
> > good idea to add the COMPILE_TEST just to increase compile coverage
>
> Will Do.

This is known to potentially break builds on things like UM where we
have !HAS_IOMEM. Please make sure you thoroughly build-test before you
switch on COMPILE_TEST.

Thierry


Attachments:
(No filename) (1.54 kB)
signature.asc (849.00 B)
Download all attachments

2018-11-08 15:33:07

by Tim Kryger

[permalink] [raw]
Subject: Re: [PATCH 2/2] pwm: bcm-kona: apply pwm settings on enable

On Thu, Nov 8, 2018 at 2:59 AM Uwe Kleine-König
<[email protected]> 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
> > <[email protected]> wrote:
> > > On Wed, Nov 07, 2018 at 10:36:13AM +0100, Clément Péron wrote:
> > > > From: Suji Velupillai <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Clément Péron <[email protected]>
> > > > ---
> > > > 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.

2018-11-09 10:02:22

by Clément Péron

[permalink] [raw]
Subject: Re: [PATCH 1/2] pwm: kconfig: enable kona pwm to be built for cygnus arch

Hi Thierry,

On Thu, 8 Nov 2018 at 13:22, Thierry Reding <[email protected]> wrote:
>
> On Thu, Nov 08, 2018 at 11:47:53AM +0100, Clément Péron wrote:
> > Hi Uwe, Scott,
> >
> > On Wed, 7 Nov 2018 at 17:48, Scott Branden <[email protected]> wrote:
> > >
> > >
> > > On 2018-11-07 8:12 a.m., Uwe Kleine-König wrote:
> > > > On Wed, Nov 07, 2018 at 10:36:12AM +0100, Clément Péron wrote:
> > > >> The Cygnus architecture use a Kona PWM. This is already present
> > > >> in the device tree but can't be built actually. Hence, allow the
> > > >> Kona PWM to be built for Cygnus arch.
> > > >>
> > > >> Signed-off-by: Clément Péron <[email protected]>
> > > >> ---
> > > >> drivers/pwm/Kconfig | 3 ++-
> > > >> 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > >> index 763ee50ea57d..76d56fc8b1b7 100644
> > > >> --- a/drivers/pwm/Kconfig
> > > >> +++ b/drivers/pwm/Kconfig
> > > >> @@ -88,7 +88,8 @@ config PWM_BCM_IPROC
> > > >>
> > > >> config PWM_BCM_KONA
> > > >> tristate "Kona PWM support"
> > > >> - depends on ARCH_BCM_MOBILE
> > > >> + depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS
> > > >> + default ARCH_BCM_CYGNUS
> > > > Is it possible to build this driver also on other arches? Then you might
> > > > want to consider
> > > >
> > > > depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST
> > > good idea to add the COMPILE_TEST just to increase compile coverage
> >
> > Will Do.
>
> This is known to potentially break builds on things like UM where we
> have !HAS_IOMEM. Please make sure you thoroughly build-test before you
> switch on COMPILE_TEST.

Thanks for the point, so something like this (but maybe it's too
protective no ?) :

config PWM_BCM_KONA
tristate "Kona PWM support"
depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST
depends on COMMON_CLK && HAS_IOMEM && OF
default ARCH_BCM_CYGNUS

Regards,
Clement


>
> Thierry

2018-11-09 10:56:07

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH 1/2] pwm: kconfig: enable kona pwm to be built for cygnus arch

On Fri, Nov 09, 2018 at 10:58:41AM +0100, Cl?ment P?ron wrote:
> Hi Thierry,
>
> On Thu, 8 Nov 2018 at 13:22, Thierry Reding <[email protected]> wrote:
> >
> > On Thu, Nov 08, 2018 at 11:47:53AM +0100, Cl?ment P?ron wrote:
> > > Hi Uwe, Scott,
> > >
> > > On Wed, 7 Nov 2018 at 17:48, Scott Branden <[email protected]> wrote:
> > > >
> > > >
> > > > On 2018-11-07 8:12 a.m., Uwe Kleine-K?nig wrote:
> > > > > On Wed, Nov 07, 2018 at 10:36:12AM +0100, Cl?ment P?ron wrote:
> > > > >> The Cygnus architecture use a Kona PWM. This is already present
> > > > >> in the device tree but can't be built actually. Hence, allow the
> > > > >> Kona PWM to be built for Cygnus arch.
> > > > >>
> > > > >> Signed-off-by: Cl?ment P?ron <[email protected]>
> > > > >> ---
> > > > >> drivers/pwm/Kconfig | 3 ++-
> > > > >> 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >>
> > > > >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > >> index 763ee50ea57d..76d56fc8b1b7 100644
> > > > >> --- a/drivers/pwm/Kconfig
> > > > >> +++ b/drivers/pwm/Kconfig
> > > > >> @@ -88,7 +88,8 @@ config PWM_BCM_IPROC
> > > > >>
> > > > >> config PWM_BCM_KONA
> > > > >> tristate "Kona PWM support"
> > > > >> - depends on ARCH_BCM_MOBILE
> > > > >> + depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS
> > > > >> + default ARCH_BCM_CYGNUS
> > > > > Is it possible to build this driver also on other arches? Then you might
> > > > > want to consider
> > > > >
> > > > > depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST
> > > > good idea to add the COMPILE_TEST just to increase compile coverage
> > >
> > > Will Do.
> >
> > This is known to potentially break builds on things like UM where we
> > have !HAS_IOMEM. Please make sure you thoroughly build-test before you
> > switch on COMPILE_TEST.
>
> Thanks for the point, so something like this (but maybe it's too
> protective no ?) :
>
> config PWM_BCM_KONA
> tristate "Kona PWM support"
> depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST
> depends on COMMON_CLK && HAS_IOMEM && OF

Instead of COMMON_CLK HAVE_CLK should be good enough. Just for compile
testing the default implementations provided in include/linux/clk.h
should even be good enough in the !HAVE_CLK case.

Also OF might not be necessary, at least the driver compiles fine on
amd64 without OF.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

2018-12-12 11:06:17

by Thierry Reding

[permalink] [raw]
Subject: Re: [PATCH 2/2] pwm: bcm-kona: apply pwm settings on enable

On Wed, Nov 07, 2018 at 10:36:13AM +0100, Clément Péron wrote:
> From: Suji Velupillai <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Clément Péron <[email protected]>
> ---
> 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


Attachments:
(No filename) (708.00 B)
signature.asc (849.00 B)
Download all attachments

2018-12-12 11:10:00

by Clément Péron

[permalink] [raw]
Subject: Re: [PATCH 2/2] pwm: bcm-kona: apply pwm settings on enable

Hi Thierry,

On Wed, 12 Dec 2018 at 12:04, Thierry Reding <[email protected]> wrote:
>
> On Wed, Nov 07, 2018 at 10:36:13AM +0100, Clément Péron wrote:
> > From: Suji Velupillai <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Clément Péron <[email protected]>
> > ---
> > 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