Re: [PATCH v5 09/16] pwm: crc: Fix period changes not having any effect

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Jul 17, 2020 at 03:37: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
> 
> I strongly suspect that the BACKLIGHT_EN register at address 0x51 really
> controls a separate output-only GPIO which is connected to the LCD panels
> backlight-enable input. Like how the PANEL_EN register at address 0x52
> controls an output-only GPIO which is earmarked for the LCD panel's
> enable pin. If this is correct then this GPIO should really be added to
> the gpio-crystalcove.c driver and the PWM driver should stop poking it.
> But I've been unable to come up with a definitive answer here, so I'm
> keeping this as is for now.
> 
> 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.

We have a confirmation now that those two bits are real GPOs.

So, with corrected commit message

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  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.26.2
> 

-- 
With Best Regards,
Andy Shevchenko