Re: [PATCH V3] PWM: Add SPEAr PWM chip driver support

[Thierry Reding]

On Mon, Oct 22, 2012 at 11:51:11AM +0530, Viresh Kumar wrote:
> On 22 October 2012 11:36, Shiraz Hashim <shiraz.hashim@xxxxxx> wrote:
> > On Mon, Oct 22, 2012 at 09:39:21AM +0530, viresh kumar wrote:
> >> On Mon, Oct 22, 2012 at 9:21 AM, Shiraz Hashim <shiraz.hashim@xxxxxx> wrote:
> 
> >> > +static int spear_pwm_remove(struct platform_device *pdev)
> >> > +{
> >> > +       struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> >> > +       int i;
> >> > +
> >> > +       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);
> >>
> >> I believe you need to remove the chip first and then do above to
> >> avoid any race conditions, that might occur.
> >
> > I am afraid, I would loose all chips and their related information
> > (PWMF_ENABLED) then.
> 
> I have just checked core's code, and yes you are correct.
> Now i have another doubt :)
> 
> Why shouldn't you do this instead:
> 
>        for (i = 0; i < NUM_PWM; i++)
>               pwm_diable(&pc->chip.pwms[i]);
> 
> And, why should we put above code in pwmchip_remove() instead, so that
> pwm drivers don't need to do all this?
> 
> @Thierry: Your inputs are required here :)

We could probably do that in the core. I've had some discussions about
this with Lars-Peter (Cc'ed) who also had doubts about how this is
currently handled.

The problem is that the core driver code ignores errors from the
driver's .remove() callback, so actually returning the error of
pwmchip_remove() here isn't terribly useful. I had actually assumed
(without checking the code) that the device wouldn't be removed if an
error was returned, but that isn't true.

IIRC Lars-Peter suggested that we do reference counting on PWM devices
so that they could stay around after the module is unloaded but return
errors (-ENODEV?) on all operations to make sure users are aware of them
disappearing.

What you're proposing is different, however. If we put that code in the
core it will mean that once the module is unloaded, all PWM devices will
be disabled. There is currently code in the core that prevents the chip
from being removed if one or more PWM devices are busy. But as explained
above, with the current core code this return value isn't useful at all.

This needs to be addressed, but I'm not quite sure how yet. Obviously it
cannot be solved in the core, because the PWM devices may be provided by
real hotpluggable devices, so just preventing the driver from being
removed won't help.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature