Re: [PATCH v8 10/17] pwm: crc: Fix period changes not having any effect

2020-08-31 Thread Thierry Reding
On Sun, Aug 30, 2020 at 02:57:46PM +0200, Hans de Goede wrote:
> The pwm-crc code is using 2 different enable bits:
> 1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE)
> 2. bit 0 of the BACKLIGHT_EN register
> 
> The BACKLIGHT_EN register at address 0x51 really controls a separate
> output-only GPIO which is earmarked to be used as output connected to the
> backlight-enable pin for LCD panels, this GPO is part of the PMIC's
> "Display Panel Control Block." . This pin should probably be moved over
> to a GPIO provider driver (and consumers modified accordingly), but that
> is something for an(other) patch.
> 
> Enabling / disabling the actual PWM output is controlled by the
> PWM_OUTPUT_ENABLE bit of the PWM0_CLK_DIV register.
> 
> As the comment in the old code already indicates we must disable the PWM
> before we can change the clock divider. But the crc_pwm_disable() and
> crc_pwm_enable() calls the old code make for this only change the
> BACKLIGHT_EN register; and the value of that register does not matter for
> changing the period / the divider. What does matter is that the
> PWM_OUTPUT_ENABLE bit must be cleared before a new value can be written.
> 
> This commit modifies crc_pwm_config() to clear PWM_OUTPUT_ENABLE instead
> when changing the period, so that period changes actually work.
> 
> Note this fix will cause a significant behavior change on some devices
> using the CRC PWM output to drive their backlight. Before the PWM would
> always run with the output frequency configured by the BIOS at boot, now
> the period time specified by the i915 driver will actually be honored.
> 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Hans de Goede 
> ---
>  drivers/pwm/pwm-crc.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)

Acked-by: Thierry Reding 


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH v8 10/17] pwm: crc: Fix period changes not having any effect

2020-08-30 Thread Hans de Goede
The pwm-crc code is using 2 different enable bits:
1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE)
2. bit 0 of the BACKLIGHT_EN register

The BACKLIGHT_EN register at address 0x51 really controls a separate
output-only GPIO which is earmarked to be used as output connected to the
backlight-enable pin for LCD panels, this GPO is part of the PMIC's
"Display Panel Control Block." . This pin should probably be moved over
to a GPIO provider driver (and consumers modified accordingly), but that
is something for an(other) patch.

Enabling / disabling the actual PWM output is controlled by the
PWM_OUTPUT_ENABLE bit of the PWM0_CLK_DIV register.

As the comment in the old code already indicates we must disable the PWM
before we can change the clock divider. But the crc_pwm_disable() and
crc_pwm_enable() calls the old code make for this only change the
BACKLIGHT_EN register; and the value of that register does not matter for
changing the period / the divider. What does matter is that the
PWM_OUTPUT_ENABLE bit must be cleared before a new value can be written.

This commit modifies crc_pwm_config() to clear PWM_OUTPUT_ENABLE instead
when changing the period, so that period changes actually work.

Note this fix will cause a significant behavior change on some devices
using the CRC PWM output to drive their backlight. Before the PWM would
always run with the output frequency configured by the BIOS at boot, now
the period time specified by the i915 driver will actually be honored.

Reviewed-by: Andy Shevchenko 
Signed-off-by: Hans de Goede 
---
 drivers/pwm/pwm-crc.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
index 44ec7d5b63e1..81232da0c767 100644
--- a/drivers/pwm/pwm-crc.c
+++ b/drivers/pwm/pwm-crc.c
@@ -82,14 +82,11 @@ static int crc_pwm_config(struct pwm_chip *c, struct 
pwm_device *pwm,
if (pwm_get_period(pwm) != period_ns) {
int clk_div = crc_pwm_calc_clk_div(period_ns);
 
-   /* changing the clk divisor, need to disable fisrt */
-   crc_pwm_disable(c, pwm);
+   /* changing the clk divisor, clear PWM_OUTPUT_ENABLE first */
+   regmap_write(crc_pwm->regmap, PWM0_CLK_DIV, 0);
 
regmap_write(crc_pwm->regmap, PWM0_CLK_DIV,
clk_div | PWM_OUTPUT_ENABLE);
-
-   /* enable back */
-   crc_pwm_enable(c, pwm);
}
 
/* change the pwm duty cycle */
-- 
2.28.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel