On Fri, Jul 17, 2015 at 2:35 PM, YH Huang [off-list ref] wrote:

On Fri, 2015-07-17 at 01:18 +0800, Daniel Kurtz wrote:

quoted

On Fri, Jul 17, 2015 at 12:44 AM, YH Huang [off-list ref] wrote:

quoted

On Thu, 2015-07-16 at 23:21 +0800, Daniel Kurtz wrote:

quoted

On Thu, Jul 16, 2015 at 3:17 PM, YH Huang [off-list ref] wrote:

quoted

On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote:

quoted

On Thu, Jul 16, 2015 at 1:38 PM, YH Huang [off-list ref] wrote:

quoted

On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:

quoted

On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:

quoted

On Mon, Jul 13, 2015 at 5:04 PM, YH Huang [off-list ref] wrote:

quoted

+#ifdef CONFIG_PM_SLEEP
+static int mtk_disp_pwm_suspend(struct device *dev)
+{
+       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
+
+       clk_disable_unprepare(mdp->clk_main);
+       clk_disable_unprepare(mdp->clk_mm);
+
+       return 0;
+}
+
+static int mtk_disp_pwm_resume(struct device *dev)
+{
+       struct mtk_disp_pwm *mdp = dev_get_drvdata(dev);
+       int ret;
+
+       ret = clk_prepare_enable(mdp->clk_main);
+       if (ret < 0)
+               return ret;
+
+       ret = clk_prepare_enable(mdp->clk_mm);
+       if (ret < 0) {
+               clk_disable_unprepare(mdp->clk_main);
+               return ret;
+       }
+

Don't you also have to restore the PWM rate and frequency?

Is it possible to save power at runtime by leaving mdp->clk_mm enabled
(to generate the PWM signal), but disable mdp->clk_main (clock
required to access PWM registers)?

The pwm-backlight driver will restore the data.

After I try to disable anyone of the two clocks at runtime, the
backlight doesn't work well(no immediate update or losing backlight).
So we need to keep both clock enabled.

Do you mean you see backlight glitch because the clocks / backlight
were *already on* during the first config (Perhaps left on by the
bootloader)?
I don't know how to solve that problem.
Maybe Thierry does.

In any case, this is a minor issue; we really shouldn't hold up
landing the driver to optimize when the clocks are enabled/disabled
:-). I'm happy enough with what you have in this patch.

Sorry for my terrible expression. Let me try again.
1. We want to disable unnecessary clock at runtime.
But, I get backlight glitch when I disable clk_main or clk_mm in
mtk_disp_pwm_config(). So both clocks are necessary and we don't disable
them at runtime.

2. Because pwm-backlight driver calls mtk_disp_pwm_config() before
mtk_disp_pwm_enable(), we will lose the first config if clocks are
enabled in mtk_disp_pwm_enable(). I prefer to enable clocks in probe
function. Samsung did the same way in their pwm driver.

I don't understand why you will "lose the first config if clocks are
enabled in mtk_disp_pwm_enable().  I don't believe registers will lose
their values just because you turn enable/disable clocks.

Perhaps I wasn't clear with what I was proposing which is something like this:

mtk_disp_pwm_config()
{
  clk_enable();
  /* write registers */
  clk_disable();
}

mtk_disp_enable()
{
  clk_enable();
  /* write enable bit */
}

mtk_disp_disable()
{
  /* clear enable bit */
  clk_disable();
}


In this way, if mtk_disp_pwm_config() is called when the pwm is
disabled, we will temporarily enable the clocks long enough to update
the register values.  These values should take effect the next time
the PWM is enabled.  We then disable the clocks and wait for the PWM
to be enabled.

If mtk_disp_pwm_config() is called when the pwm is already enabled, we
will increment the enable count on the clocks, but then we decrement
it again immediately.

I think there is something wrong about mdp->clk_mm(generate the PWM
signal) and mdp->clk_main(access PWM registers).
If we want to write the register, we should enable both clocks before.
I try it in this way.

mtk_disp_pwm_config()
{
   clk_enable(mdp->clk_main);
   clk_enable(mdp->clk_mm);
   /* write registers */
   clk_disable(mdp->clk_mm);
   clk_disable(mdp->clk_main);
}
with
A:
mtk_disp_enable()
{
   clk_enable(mdp->clk_main);
   clk_enable(mdp->clk_mm);
   /* write enable bit */
   clk_disable(mdp->clk_mm);
}
or
B:
mtk_disp_enable()
{
   clk_enable(mdp->clk_main);
   clk_enable(mdp->clk_mm);
   /* write enable bit */
   clk_disable(mdp->clk_main);
}

I both get backlight glitch with "A" or "B".
So I think we should keep clocks enabled.

Ok, but what about this:

mtk_disp_pwm_config()
{
   clk_enable(mdp->clk_main);
   /* write registers */
   clk_disable(mdp->clk_main);
}

mtk_disp_enable()
{
   clk_enable(mdp->clk_main);
   clk_enable(mdp->clk_mm);
   /* write enable bit */
}

Regards,
YH Huang