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.has...@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.has...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
