On Tue, Oct 07, 2014 at 11:14:11AM +0200, Boris Brezillon wrote:
> On Tue, 7 Oct 2014 10:45:16 +0200 Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> > On Mon, Oct 06, 2014 at 04:07:02PM +0200, Boris Brezillon wrote:
[...]
> > > +static int atmel_hlcdc_pwm_remove(struct platform_device *pdev)
> > > +{
> > > + struct atmel_hlcdc_pwm *chip = platform_get_drvdata(pdev);
> > > + int ret;
> > > +
> > > + ret = pwmchip_remove(&chip->chip);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + clk_disable_unprepare(chip->hlcdc->periph_clk);
> >
> > You might want to call clk_disable_unprepare() regardless of whether or
> > not pwmchip_remove() failed. You could simply leave out the above check
> > for ret and instead...
>
> Are you sure of this one, if pwmchip_remove fails, then the PWM chip
> might still be used. And if we disable the clock the PWM chip won't work
> anymore.
The return value of .remove() isn't really used, so if built as a module
the PWM chip will disappear anyway. Even if not built as a module the
managed resources are going to go away anyway, so for all intents and
purposes the PWM chip will be defunct.
A long time ago there were some discussions about adding reference
counting to the PWM chip and PWM devices to better handle this situation
but it has never really proven to be an issue thus far.
Perhaps a better way to solve this would be to make pwmchip_remove() not
return an error and instead WARN in the single case where it can fail
(if one of the PWM devices it exposes is still in use). That way users
will still get an appropriate warning that something's awry and it would
play more nicely with an advanced mechanism to keep PWM devices alive
beyond the lifetime of a driver to cope with removal.
Irrespective of the above it's probably fine either way, since if
pwmchip_remove() fails you're in a pretty bad place anyway.
Thierry
Attachment:
pgpuaBrgchmrV.pgp
Description: PGP signature
