[2/2] gpio/omap: add *remove* callback in platform_driver
diff mbox

Message ID 1341997995-14020-3-git-send-email-tarun.kanti@ti.com
State New, archived
Headers show

Commit Message

Tarun Kanti DebBarma July 11, 2012, 9:13 a.m. UTC
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>
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(-)

Comments

Linus Walleij July 11, 2012, 9:54 p.m. UTC | #1
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
Kevin Hilman July 11, 2012, 11:25 p.m. UTC | #2
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,
Linus Walleij July 12, 2012, 11:03 a.m. UTC | #3
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
Kevin Hilman July 12, 2012, 5:48 p.m. UTC | #4
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
Linus Walleij July 14, 2012, 8:51 p.m. UTC | #5
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

Patch
diff mbox

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,