Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755528AbbGPRTG (ORCPT ); Thu, 16 Jul 2015 13:19:06 -0400 Received: from mail-yk0-f169.google.com ([209.85.160.169]:36045 "EHLO mail-yk0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753858AbbGPRTB (ORCPT ); Thu, 16 Jul 2015 13:19:01 -0400 MIME-Version: 1.0 In-Reply-To: <1437065050.11063.26.camel@mtksdaap41> References: <1436778294-47635-1-git-send-email-yh.huang@mediatek.com> <1436778294-47635-3-git-send-email-yh.huang@mediatek.com> <1436975995.15774.27.camel@mtksdaap41> <1437025108.18175.13.camel@mtksdaap41> <1437031079.8107.13.camel@mtksdaap41> <1437065050.11063.26.camel@mtksdaap41> From: Daniel Kurtz Date: Fri, 17 Jul 2015 01:18:41 +0800 X-Google-Sender-Auth: 5XGvO5PtZj0noq3VyM1TaA35pzY Message-ID: Subject: Re: [PATCH v5 2/3] pwm: add MediaTek display PWM driver support To: YH Huang Cc: Thierry Reding , Matthias Brugger , Mark Rutland , Rob Herring , Pawel Moll , linux-pwm@vger.kernel.org, "open list:OPEN FIRMWARE AND..." , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , srv_heupstream , linux-mediatek@lists.infradead.org, Sascha Hauer , Yingjoe Chen Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4378 Lines: 121 On Fri, Jul 17, 2015 at 12:44 AM, YH Huang wrote: > On Thu, 2015-07-16 at 23:21 +0800, Daniel Kurtz wrote: >> On Thu, Jul 16, 2015 at 3:17 PM, YH Huang wrote: >> > On Thu, 2015-07-16 at 14:54 +0800, Daniel Kurtz wrote: >> >> On Thu, Jul 16, 2015 at 1:38 PM, YH Huang 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 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/