Date: Fri, 19 Oct 2012 15:13:09 +0530
From: Shiraz Hashim <shiraz.hashim@st.com>
To: viresh kumar <viresh.kumar@linaro.org>
Cc: <thierry.reding@avionic-design.de>,
        <spear--sw-devel@lists.codex.cro.st.com>,
        <linux-kernel@vger.kernel.org>, <spear-devel@list.st.com>
Subject: Re: [PATCH] pwm: add spear pwm driver support
Message-ID: <20121019094308.GC4185@localhost.localdomain>
References: <1350559712-8514-1-git-send-email-shiraz.hashim@st.com>
 <CAOh2x=mdtkW=a-xv1BeHxoGsgbF5s+4B8QmZBdvqnWHr30Ap2A@mail.gmail.com>
 <20121019055943.GA4185@localhost.localdomain>
 <CAOh2x=kXu2EG4GFaW8HpSi=VFLvGEMEyeN_3L74wU2sfe1Z=uQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAOh2x=kXu2EG4GFaW8HpSi=VFLvGEMEyeN_3L74wU2sfe1Z=uQ@mail.gmail.com>
User-Agent: Mutt/1.5.20 (2009-06-14)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3427
Lines: 107

Hi Viresh,

On Fri, Oct 19, 2012 at 12:23:08PM +0530, viresh kumar wrote:
> On Fri, Oct 19, 2012 at 11:29 AM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> > On Thu, Oct 18, 2012 at 11:11:06PM +0530, viresh kumar wrote:
> >> On Thu, Oct 18, 2012 at 4:58 PM, Shiraz Hashim <shiraz.hashim@st.com> wrote:
> 
> >> > +static int __devexit spear_pwm_remove(struct platform_device *pdev)
> >> > +{
> >> > +       struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> >> > +       int i;
> >> > +
> >> > +       if (WARN_ON(!pc))
> >> > +               return -ENODEV;
> >> > +
> >> > +       for (i = 0; i < NUM_PWM; i++) {
> >> > +               struct pwm_device *pwmd = &pc->chip.pwms[i];
> >> > +
> >> > +               if (!test_bit(PWMF_ENABLED, &pwmd->flags))
> 
> One point here: If i am not wrong you want to disable pwmd if it is enabled.
> Shouldn't you check for if (test_bit(PWMF_ENABLED, &pwmd->flags)) instead?
> 

You are right. The code is un-necessarily disabling all pwms while trying to
enable the clock for those which are not enabled also.


> >> > +                       if (clk_prepare_enable(pc->clk) < 0)
> >> > +                               continue;
> >> > +
> >> > +               spear_pwm_writel(pc, i, PWMCR, 0);
> >> > +               clk_disable_unprepare(pc->clk);
> >> > +       }
> >>
> >> You are enabling/disabling clock N times here and each of these will
> >> write to an register. Do something better.
> >>
> >
> > I need to shut down all active pwms, how else would you suggest that ?
> 
> Sorry, i misread the code on the second go :(
> 
> I am proposing something like:
> 
> static int __devexit spear_pwm_remove(struct platform_device *pdev)
> {
>        struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
>        int i, clk_enabled = 0;
> 
>        if (WARN_ON(!pc))
>                return -ENODEV;
> 
>        for (i = 0; i < NUM_PWM; i++) {
>                struct pwm_device *pwmd = &pc->chip.pwms[i];
> 
>                if (!test_bit(PWMF_ENABLED, &pwmd->flags) && !clk_enabled)
>                        if (clk_prepare_enable(pc->clk) < 0)
>                                continue;
> 		       else
> 			       clk_enabled++;
> 
>                spear_pwm_writel(pc, i, PWMCR, 0);
>        }
> 
>        if (clk_enabled)
> 	       clk_disable_unprepare(pc->clk);
>        ...
> }


It may not be required as pwms which are not enabled do not have
their clocks enabled. Hence, perhaps we can do following,

8<---------------------------
static int spear_pwm_remove(struct platform_device *pdev)
{
        struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
        int i;

        if (WARN_ON(!pc))
                return -ENODEV;

        for (i = 0; i < NUM_PWM; i++) {
                struct pwm_device *pwm = &pc->chip.pwms[i];

                if (test_bit(PWMF_ENABLED, &pwm->flags)) {
                        spear_pwm_writel(pc, i, PWMCR, 0);
                        clk_disable(pc->clk);
                }
        }

        /* clk was prepared in probe, hence unprepare it here */
        clk_unprepare(pc->clk);
        return pwmchip_remove(&pc->chip);
}

---------------------------->8

--
regards
Shiraz 
--
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/