On Mon, Oct 22, 2012 at 11:51:11AM +0530, Viresh Kumar wrote:
> On 22 October 2012 11:36, Shiraz Hashim <shiraz.has...@st.com> 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.has...@st.com>
> >> 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
pgpkaAgYKI1cJ.pgp
Description: PGP signature
