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

[Thierry Reding]

On Mon, Oct 22, 2012 at 02:20:26PM +0200, Lars-Peter Clausen wrote:
> On 10/22/2012 09:55 AM, Thierry Reding wrote:
> > 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.
> 
> In my opinion it would make sense to put this into the PWM core. Even if the
> device is still physically connected, e.g. because it is a on-SoC device, it
> should be stopped if the device is removed. You do not want the PWM device
> to continue to provide it's service (which is the PWM signal) after the
> device has been removed. This means this is something that needs to be
> implemented by every PWM driver.

Alright. Let's keep it in the driver for now and I'll remove it once I
get around to implementing the functionality in the core.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature