pwm: kconfig: enable kona pwm to be built for cygnus arch

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

drivers/pwm/pwm-bcm-kona.c +2 -2

index 09a95aeb3a70..d991d53c4b38 100644
@@ -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;

Hello,

Quoted 27 lines · Clément Péron

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

drivers/pwm/pwm-bcm-kona.c +2 -2

index 09a95aeb3a70..d991d53c4b38 100644
@@ -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;

drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 158

Quoted 5 lines

drivers/pwm/pwm-bcm-kona.c +1 -1

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

Quoted 38 lines · Uwe Kleine-König

Quoted 27 lines · Clément Péron

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

drivers/pwm/pwm-bcm-kona.c +2 -2

index 09a95aeb3a70..d991d53c4b38 100644
@@ -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;

drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 158

Quoted 5 lines

drivers/pwm/pwm-bcm-kona.c +1 -1

@@ -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:

Quoted 5 lines

        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.

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?

Quoted 46 lines · Clément Péron

Quoted 38 lines · Uwe Kleine-König

Quoted 27 lines · Clément Péron

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

drivers/pwm/pwm-bcm-kona.c +2 -2

index 09a95aeb3a70..d991d53c4b38 100644
@@ -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;

drivers/pwm/pwm-bcm-kona.c static int kona_pwmc_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 158

Quoted 5 lines

drivers/pwm/pwm-bcm-kona.c +1 -1

@@ -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:

Quoted 5 lines

        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?

Quoted 7 lines

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

(quoted diff) +1 -1

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

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

Quoted 10 lines · Clément Péron

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

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

drivers/pwm/Kconfig +0 -0

index 763ee50ea57d..76d56fc8b1b7 100644

Quoted 14 lines · 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 <[email protected]>

---
  drivers/pwm/Kconfig | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pwm/Kconfig +0 -0

index 763ee50ea57d..76d56fc8b1b7 100644

drivers/pwm/Kconfig config PWM_BCM_IPROC · line 92

Quoted 6 lines

drivers/pwm/Kconfig +2 -1

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

Quoted 24 lines · Uwe Kleine-König

Quoted 14 lines · 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 <[email protected]>

---
  drivers/pwm/Kconfig | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pwm/Kconfig +0 -0

index 763ee50ea57d..76d56fc8b1b7 100644

drivers/pwm/Kconfig config PWM_BCM_IPROC · line 92

Quoted 6 lines

drivers/pwm/Kconfig +2 -1

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

Quoted 1 line

      depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST

good idea to add the COMPILE_TEST just to increase compile coverage

Quoted 4 lines

Best regards
Uwe

Hi Uwe, Scott,

Quoted 27 lines · Scott Branden

Quoted 24 lines · Uwe Kleine-König

Quoted 14 lines · 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 <[email protected]>

---
  drivers/pwm/Kconfig | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pwm/Kconfig +0 -0

index 763ee50ea57d..76d56fc8b1b7 100644

drivers/pwm/Kconfig config PWM_BCM_IPROC · line 92

Quoted 6 lines

drivers/pwm/Kconfig +2 -1

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

Quoted 2 lines

Quoted 1 line

      depends on ARCH_BCM_MOBILE || ARCH_BCM_CYGNUS || COMPILE_TEST

good idea to add the COMPILE_TEST just to increase compile coverage

Will Do.

Quoted 30 lines · Clément Péron

Hi Uwe, Scott,

Quoted 27 lines · Scott Branden

Quoted 24 lines · Uwe Kleine-König

Quoted 14 lines · 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 <[email protected]>

---
  drivers/pwm/Kconfig | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pwm/Kconfig +0 -0

index 763ee50ea57d..76d56fc8b1b7 100644

drivers/pwm/Kconfig config PWM_BCM_IPROC · line 92

Quoted 6 lines

drivers/pwm/Kconfig +2 -1

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

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

      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

Hi Thierry,

Quoted 32 lines · Thierry Reding

Quoted 30 lines · Clément Péron

Hi Uwe, Scott,

Quoted 27 lines · Scott Branden

Quoted 24 lines · Uwe Kleine-König

Quoted 14 lines · 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 <[email protected]>

---
  drivers/pwm/Kconfig | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pwm/Kconfig +0 -0

index 763ee50ea57d..76d56fc8b1b7 100644

drivers/pwm/Kconfig config PWM_BCM_IPROC · line 92

Quoted 6 lines

drivers/pwm/Kconfig +2 -1

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

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

      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