linux-kernel - Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGS+omAj0=AWsUA+B8mDOBK5kjwhjwqfmxq4bFeGViOWUU0pCg@mail.gmail.com>
Date:	Fri, 17 Jul 2015 01:18:41 +0800
From:	Daniel Kurtz <djkurtz@...omium.org>
To:	YH Huang <yh.huang@...iatek.com>
Cc:	Thierry Reding <thierry.reding@...il.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Mark Rutland <mark.rutland@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>, linux-pwm@...r.kernel.org,
	"open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	srv_heupstream <srv_heupstream@...iatek.com>,
	linux-mediatek@...ts.infradead.org,
	Sascha Hauer <kernel@...gutronix.de>,
	Yingjoe Chen <yingjoe.chen@...iatek.com>
Subject: Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support

On Fri, Jul 17, 2015 at 12:44 AM, YH Huang <yh.huang@...iatek.com> wrote:
> On Thu, 2015-07-16 at 23:21 +0800, Daniel Kurtz wrote:
>> On Thu, Jul 16, 2015 at 3:17 PM, YH Huang <yh.huang@...iatek.com> wrote:
>> > On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote:
>> >> On Thu, Jul 16, 2015 at 1:38 PM, YH Huang <yh.huang@...iatek.com> wrote:
>> >> > On Wed, 2015-07-15 at 23:59 +0800, YH Huang wrote:
>> >> >> On Mon, 2015-07-13 at 18:19 +0800, Daniel Kurtz wrote:
>> >> >> > On Mon, Jul 13, 2015 at 5:04 PM, YH Huang <yh.huang@...iatek.com> wrote:
>> >> >> > > +#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.

Thanks,
-Dan



>
> Thanks for your kindly suggestions. I will update the patch soon.
>
> Regards,
> YH Huang
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/