Re: [PATCH] pwm: add spear pwm driver support

[Shiraz Hashim <shiraz.hashim@st.com>]

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