| Message ID | 1341997995-14020-3-git-send-email-tarun.kanti@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 11, 2012 at 11:13 AM, Tarun Kanti DebBarma <tarun.kanti@ti.com> wrote: > Add *remove* callback so that necessary cleanup operations are > performed when device is unregistered. The device is deleted > from the list and associated clock handle is released by > calling clk_put() and irq descriptor is released using the > irq_free_desc() api. > > Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> Looks good, applied. Yours, Linus Walleij
Tarun Kanti DebBarma <tarun.kanti@ti.com> writes: > Add *remove* callback so that necessary cleanup operations are > performed when device is unregistered. How was this tested? on what platforms? > The device is deleted > from the list and associated clock handle is released by > calling clk_put() and irq descriptor is released using the > irq_free_desc() api. There is quite a bit of other things to do in remove to properly cleanup what is done in probe. Also, what happens when a 'remove' is triwhen there are GPIOs that are still requested and in use, especially if they are GPIO IRQs. Also, what about runtime PM? In short, this seems very premature and I suspect untested. Kevin > Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com> > Reported-by: Paul Walmsley <paul@pwsan.com> > Reviewed-by: Jon Hunter <jon-hunter@ti.com> > Cc: Kevin Hilman <khilman@ti.com> > Cc: Rajendra Nayak <rnayak@ti.com> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Cousson, Benoit <b-cousson@ti.com> > Cc: Paul Walmsley <paul@pwsan.com> > --- > drivers/gpio/gpio-omap.c | 30 ++++++++++++++++++++++++++++++ > 1 files changed, 30 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index afecdcc..08929d5 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -1140,6 +1140,35 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) > return ret; > } > > +/** > + * omap_gpio_remove - cleanup a registered gpio device > + * @pdev: pointer to current gpio platform device > + * > + * Called by driver framework whenever a gpio device is unregistered. > + * GPIO is deleted from the list and associated clock handle freed. > + */ > +static int __devexit omap_gpio_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct gpio_bank *bank; > + unsigned long flags; > + int ret = -EINVAL; > + > + list_for_each_entry(bank, &omap_gpio_list, node) { > + spin_lock_irqsave(&bank->lock, flags); > + if (bank->dev == dev) { > + list_del(&bank->node); > + clk_put(bank->dbck); > + irq_free_desc(bank->irq_base); > + ret = 0; > + spin_unlock_irqrestore(&bank->lock, flags); > + break; > + } > + spin_unlock_irqrestore(&bank->lock, flags); > + } > + return ret; > +} > + > #ifdef CONFIG_ARCH_OMAP2PLUS > > #if defined(CONFIG_PM_RUNTIME) > @@ -1466,6 +1495,7 @@ MODULE_DEVICE_TABLE(of, omap_gpio_match); > > static struct platform_driver omap_gpio_driver = { > .probe = omap_gpio_probe, > + .remove = __devexit_p(omap_gpio_remove), > .driver = { > .name = "omap_gpio", > .pm = &gpio_pm_ops,
On Thu, Jul 12, 2012 at 1:25 AM, Kevin Hilman <khilman@ti.com> wrote: > There is quite a bit of other things to do in remove to properly cleanup > what is done in probe. OK I'm dropping this patch for now... Yours, Linus Walleij
Hi Linus, Linus Walleij <linus.walleij@linaro.org> writes: > On Thu, Jul 12, 2012 at 1:25 AM, Kevin Hilman <khilman@ti.com> wrote: > >> There is quite a bit of other things to do in remove to properly cleanup >> what is done in probe. > > OK I'm dropping this patch for now... > Thanks. For future reference... as one of the OMAP maintainers, I request that you not pull/merge OMAP GPIO patches unless they've had a bit of review and exposure. Unfortunately, we have a history with this driver where regressions have been introduced and the maintainers end up having to find and fix them. In the case of OMAP GPIO, unless it's an obvious fix, I would recommend you wait at least until you see some acks/tested tags from any of - Santosh Shilimkar <santosh.shilimkar@ti.com> - Rajendra Nayak <rnayak@ti.com> - Benoit Cousson <b-cousson@ti.com> or Tony, Paul or myself. For major series, I have been collecting/queueing them for Grant after ensuring they have been well reviewed and well tested (although I am eagerly hoping to hand off this role to someone else.) Thanks, Kevin
On Thu, Jul 12, 2012 at 7:48 PM, Kevin Hilman <khilman@ti.com> wrote: > In the case of OMAP GPIO, unless it's an obvious fix, I would recommend > you wait at least until you see some acks/tested tags from any of > > - Santosh Shilimkar <santosh.shilimkar@ti.com> > - Rajendra Nayak <rnayak@ti.com> > - Benoit Cousson <b-cousson@ti.com> > > or Tony, Paul or myself. Instead of trying to store this information in my and Grants brains and us forgetting it, what about patching MAINTAINERS to reflect the fact instead? That's better I think, plus I use that file a lot. > For major series, I have been collecting/queueing them for Grant after > ensuring they have been well reviewed and well tested (although I am > eagerly hoping to hand off this role to someone else.) Listing it under your GIT tree in MAINTAINERS for this driver will make this work better I think. One path for OMAP GPIO patches, simple. It's obviously the ambiguity that cause the trouble. Then you can also decide on each cycle whether to send these to GPIO or ARM SoC etc. Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index afecdcc..08929d5 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -1140,6 +1140,35 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) return ret; } +/** + * omap_gpio_remove - cleanup a registered gpio device + * @pdev: pointer to current gpio platform device + * + * Called by driver framework whenever a gpio device is unregistered. + * GPIO is deleted from the list and associated clock handle freed. + */ +static int __devexit omap_gpio_remove(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct gpio_bank *bank; + unsigned long flags; + int ret = -EINVAL; + + list_for_each_entry(bank, &omap_gpio_list, node) { + spin_lock_irqsave(&bank->lock, flags); + if (bank->dev == dev) { + list_del(&bank->node); + clk_put(bank->dbck); + irq_free_desc(bank->irq_base); + ret = 0; + spin_unlock_irqrestore(&bank->lock, flags); + break; + } + spin_unlock_irqrestore(&bank->lock, flags); + } + return ret; +} + #ifdef CONFIG_ARCH_OMAP2PLUS #if defined(CONFIG_PM_RUNTIME) @@ -1466,6 +1495,7 @@ MODULE_DEVICE_TABLE(of, omap_gpio_match); static struct platform_driver omap_gpio_driver = { .probe = omap_gpio_probe, + .remove = __devexit_p(omap_gpio_remove), .driver = { .name = "omap_gpio", .pm = &gpio_pm_ops,