Prev: of/gpio: Add support for two-stage registration for the of_gpio_chips
Next: twl4030 threaded_irq support
From: David Brownell on 26 Jan 2010 01:50
On Monday 25 January 2010, Anton Vorontsov wrote:
>
> +config GPIOLIB_NOTIFIER
> +�������bool
> +�������help
> +������� �This symbol is selected by subsystems that need to handle GPIO
> +������� �chips addition and removal. E.g., this is used for the
> +������� �OpenFirmware bindings.
> +

I'm no huge fan of notifiers, but I suppose they have their place.

However ... I don't see a lot of win to making this optional. Just
inline the little two blocking_notifier_call_chain() calls directly,
making this a *LOT* simpler.

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Anton Vorontsov on 26 Jan 2010 12:30
On Mon, Jan 25, 2010 at 10:34:29PM -0800, David Brownell wrote:
> On Monday 25 January 2010, Anton Vorontsov wrote:
> >
> > +config GPIOLIB_NOTIFIER
> > +       bool
> > +       help
> > +         This symbol is selected by subsystems that need to handle GPIO
> > +         chips addition and removal. E.g., this is used for the
> > +         OpenFirmware bindings.
> > +
>
> I'm no huge fan of notifiers, but I suppose they have their place.
>
> However ... I don't see a lot of win to making this optional.

OK, will remove it.

> Just
> inline the little two blocking_notifier_call_chain() calls directly,
> making this a *LOT* simpler.

I'd rather stay with gpio_call_chain() helper, it makes the code
a little bit prettier, IMO. Compare this:

--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1103,6 +1107,9 @@ fail:
pr_err("gpiochip_add: gpios %d..%d (%s) not registered\n",
chip->base, chip->base + chip->ngpio - 1,
chip->label ? : "generic");
+ else
+ blocking_notifier_call_chain(&gpio_notifier,
+ GPIO_NOTIFY_CHIP_ADDED, chip);
return status;
}
EXPORT_SYMBOL_GPL(gpiochip_add);
@@ -1119,6 +1126,13 @@ int gpiochip_remove(struct gpio_chip *chip)
int status = 0;
unsigned id;

+ /* Ask external subsystems to release the chip. */
+ status = blocking_notifier_call_chain(&gpio_notifier,
+ GPIO_NOTIFY_CHIP_REMOVE, chip);
+ status = notifier_to_errno(status);
+ if (status)
+ return status;
+
spin_lock_irqsave(&gpio_lock, flags);

for (id = chip->base; id < chip->base + chip->ngpio; id++) {

---

With the call_chain helper:

--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1029,6 +1030,16 @@ static inline void gpiochip_unexport(struct gpio_chip *chip)

#endif /* CONFIG_GPIO_SYSFS */

+static int gpio_call_chain(struct gpio_chip *chip, enum gpio_notify_msg msg)
+{
+ int ret = blocking_notifier_call_chain(&gpio_notifier, msg, chip);
+
+ return notifier_to_errno(ret);
+}
+
/**
* gpiochip_add() - register a gpio_chip
* @chip: the chip to register, with chip->base initialized
@@ -1103,6 +1114,9 @@ fail:
pr_err("gpiochip_add: gpios %d..%d (%s) not registered\n",
chip->base, chip->base + chip->ngpio - 1,
chip->label ? : "generic");
+ else
+ gpio_call_chain(chip, GPIO_NOTIFY_CHIP_ADDED);
+
return status;
}
EXPORT_SYMBOL_GPL(gpiochip_add);
@@ -1119,6 +1133,11 @@ int gpiochip_remove(struct gpio_chip *chip)
int status = 0;
unsigned id;

+ /* Ask external subsystems to release the chip. */
+ status = gpio_call_chain(chip, GPIO_NOTIFY_CHIP_REMOVE);
+ if (status)
+ return status;
+
spin_lock_irqsave(&gpio_lock, flags);

for (id = chip->base; id < chip->base + chip->ngpio; id++) {
---

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Brownell on 26 Jan 2010 16:10
On Tuesday 26 January 2010, Anton Vorontsov wrote:
> > Just
> > inline the little two blocking_notifier_call_chain() calls directly,
> > making this a *LOT* simpler.
>
> I'd rather stay with gpio_call_chain() helper, it makes the code
> a little bit prettier, IMO. Compare this:

The one without the wrapper is IMO more clear, since it doesn't
obfuscate anything. Fewer lines of code, too. :)

Pretty is a good attribute ... but is a distant third to clarity.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Grant Likely on 9 Feb 2010 12:20
On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
<avorontsov(a)ru.mvista.com> wrote:
> Some platforms (e.g. OpenFirmware) want to know when a particular chip
> added or removed, so that the platforms could add their specifics for
> non-platform devices, like I2C or SPI GPIO chips.
>
> This patch implements the notifier for chip addition and removal events.
>
> Signed-off-by: Anton Vorontsov <avorontsov(a)ru.mvista.com>
> ---
> �drivers/gpio/gpiolib.c � � | � 14 ++++++++++++++
> �include/asm-generic/gpio.h | � �8 ++++++++
> �2 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 350842a..375c03a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -9,6 +9,7 @@
> �#include <linux/seq_file.h>
> �#include <linux/gpio.h>
> �#include <linux/idr.h>
> +#include <linux/notifier.h>
>
>
> �/* Optional implementation infrastructure for GPIO interfaces.
> @@ -1029,6 +1030,9 @@ static inline void gpiochip_unexport(struct gpio_chip *chip)
>
> �#endif /* CONFIG_GPIO_SYSFS */
>
> +BLOCKING_NOTIFIER_HEAD(gpio_notifier);
> +EXPORT_SYMBOL_GPL(gpio_notifier);
> +
> �/**
> �* gpiochip_add() - register a gpio_chip
> �* @chip: the chip to register, with chip->base initialized
> @@ -1103,6 +1107,9 @@ fail:
> � � � � � � � �pr_err("gpiochip_add: gpios %d..%d (%s) not registered\n",
> � � � � � � � � � � � �chip->base, chip->base + chip->ngpio - 1,
> � � � � � � � � � � � �chip->label ? : "generic");
> + � � � else
> + � � � � � � � blocking_notifier_call_chain(&gpio_notifier,
> + � � � � � � � � � � � � � � � � � � � � � �GPIO_NOTIFY_CHIP_ADDED, chip);

Rather than doing an else block which will need to be reworked if/when
any additional code is added to the bottom of this routine, please
rework the if() block to bail on failure instead of implicitly falling
through to the return statement.

Otherwise, this patch looks okay to me, so you can go ahead and add my:

Acked-by: Grant Likely <grant.likely(a)secretlab.ca>

*however* (and don't kill me for saying this because I know I
suggested the notifier approach in the first place). Looking at the
whole patch series, the notifier call adds a lot of code for very
little gain. If you dropped just the notifier bits (but left the rest
of the series the same), then the the of gpio bits would be
considerably simpler, and the only impact on the core gpiolib would be
the addition of an of_gpiochip_register_simple() and
of_gpiochip_unregister() hooks that will be conditionally compiled.

And to address one of my previous concerns, I've got no problem with
the automatic registration of GPIO devices for OF usage, as long as
of-aware drivers have the option of overriding the simple defaults
when needed.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 5 Mar 2010 15:00
On Tue, 9 Feb 2010 10:16:44 -0700
Grant Likely <grant.likely(a)secretlab.ca> wrote:

> On Fri, Feb 5, 2010 at 1:32 PM, Anton Vorontsov
> <avorontsov(a)ru.mvista.com> wrote:
> > Some platforms (e.g. OpenFirmware) want to know when a particular chip
> > added or removed, so that the platforms could add their specifics for
> > non-platform devices, like I2C or SPI GPIO chips.
> >
> > This patch implements the notifier for chip addition and removal events.
> >
> > Signed-off-by: Anton Vorontsov <avorontsov(a)ru.mvista.com>
> > ---
> > drivers/gpio/gpiolib.c | 14 ++++++++++++++
> > include/asm-generic/gpio.h | 8 ++++++++
> > 2 files changed, 22 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 350842a..375c03a 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -9,6 +9,7 @@
> > #include <linux/seq_file.h>
> > #include <linux/gpio.h>
> > #include <linux/idr.h>
> > +#include <linux/notifier.h>
> >
> >
> > /* Optional implementation infrastructure for GPIO interfaces.
> > @@ -1029,6 +1030,9 @@ static inline void gpiochip_unexport(struct gpio_chip *chip)
> >
> > #endif /* CONFIG_GPIO_SYSFS */
> >
> > +BLOCKING_NOTIFIER_HEAD(gpio_notifier);
> > +EXPORT_SYMBOL_GPL(gpio_notifier);
> > +
> > /**
> > * gpiochip_add() - register a gpio_chip
> > * @chip: the chip to register, with chip->base initialized
> > @@ -1103,6 +1107,9 @@ fail:
> > pr_err("gpiochip_add: gpios %d..%d (%s) not registered\n",
> > chip->base, chip->base + chip->ngpio - 1,
> > chip->label ? : "generic");
> > + else
> > + blocking_notifier_call_chain(&gpio_notifier,
> > + GPIO_NOTIFY_CHIP_ADDED, chip);
>
> Rather than doing an else block which will need to be reworked if/when
> any additional code is added to the bottom of this routine, please
> rework the if() block to bail on failure instead of implicitly falling
> through to the return statement.

This still hasn't happened.

> Otherwise, this patch looks okay to me, so you can go ahead and add my:
>
> Acked-by: Grant Likely <grant.likely(a)secretlab.ca>
>

I'll merge it anyway and will ask you guys to keep track of this issue,
thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: of/gpio: Add support for two-stage registration for the of_gpio_chips
Next: twl4030 threaded_irq support