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

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

[Clément Péron]

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 <peron.clem@gmail.com>
---
 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

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

[Clément Péron]

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) {
 		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

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

[Uwe Kleine-König]

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 <peron.clem@gmail.com>
> ---
>  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/  |

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

[Uwe Kleine-König]

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

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

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

[Scott Branden]

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 <peron.clem@gmail.com>
>> ---
>>   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
>

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

[Clément Péron]

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

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

[Clément Péron]

Hi Uwe, Scott,

On Wed, 7 Nov 2018 at 17:48, Scott Branden <scott.branden@broadcom.com> 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 <peron.clem@gmail.com>
> >> ---
> >>   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
> >

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

[Uwe Kleine-König]

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

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

[Thierry Reding]

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

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 <scott.branden@broadcom.com> 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 <peron.clem@gmail.com>
> > >> ---
> > >>   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

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

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

[Tim Kryger]

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.

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

[Clément Péron]

Hi Thierry,

On Thu, 8 Nov 2018 at 13:22, Thierry Reding <thierry.reding@gmail.com> 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 <scott.branden@broadcom.com> 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 <peron.clem@gmail.com>
> > > >> ---
> > > >>   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

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

[Uwe Kleine-König]

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 <thierry.reding@gmail.com> 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 <scott.branden@broadcom.com> 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 <peron.clem@gmail.com>
> > > > >> ---
> > > > >>   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/  |

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

[Thierry Reding]

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

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

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

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

[Clément Péron]

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