Re: GPIO switch framework (was: Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update)
On Thursday 18 December 2008, Jani Nikula wrote: On Tue, 2008-12-16 at 11:35 +0530, ext Trilok Soni wrote: OK, I found other guys (android ??) using such home brew frameworks. Time to write a switch framework. To start a discussion on what such a GPIO switch framework should be like if someone were to write it, here's a list of the kind of things I'd like to see in it (mostly from gpio-switch.c): * Based on or integrated in the gpiolib. As noted earlier: such a thing should be a switch framework, with gpio just on implementation. Not every switch will be implemented as a GPIO. * Dynamically changeable notification callbacks in kernel. Not clear what this means. Reconfigure the switch type on the fly? I'm not a big fan of the event namespace which linux/notifier.h defines, I hope that's not what is meant here ... though maybe its callback model is OK, adding and removing callbacks is easy enough. Current OMAP gpio-switch has a single callback, and that might be part of the problem. Linux has a single callback for things like timers and request completions; but these switches have different lifecycle model. Decoupling caller/switch and callee(s)/callbacks() may provide a better model. * Sysfs notifications to userspace. Why sysfs in particular, rather than some /dev/input/* event channel? Note that input devices already decouple the event reporters and event receivers fairly well. * Debouncing for the notifications to filter spurious events. * Dynamically adjustable debounce timeouts. Debouncing might not be a framework issue; at first thought, it seems like something the GPIO glue into that framework might need to handle. * Arbitrary names for the GPIOs in sysfs instead of (or in addition to) the gpiolib style gpioN. This mostly sounds to me like an input device... Except maybe for the notion that in-kernel software needs to be responsive to the events too. I seem to recall push those policies to userspace arguments cropping up in similar cases. (My two cents: kernel is full of policy, sometimes reconfigurable; easy to believe system integrity can require a few more.) Anything else? I didn't see anything in the Android source that couldn't be covered with this. The Android stuff seems already split into a class and GPIO glue. (Hence my CC to Mike Lockwood, the author of that drivers/switch code.) Any reason you couldn't use that as-is, maybe after updating it a bit? Add some GPIO debouncing, use input framework not miscdev in the core, different headset issues, etc. - Dave Jani. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GPIO switch framework (was: Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update)
Hi Jani, On Thu, Dec 18, 2008 at 7:10 PM, Jani Nikula ext-jani.1.nik...@nokia.com wrote: On Thu, 2008-12-18 at 18:30 +0530, ext Trilok Soni wrote: Why limit to GPIO based switches only? GPIOs should be client of switch framework I think, and how h/w implements that switch should be hidden by client specific driver. You're quite right, it's better not to make such unnecessary limitations. All the more reason to discuss before anyone submits a truckload of patches for review. :) Let me CC Brian from Android here. Brian, we are discussing here of making generic switch framework and android-kernel also have near to generic switch fwk implementation, so that we can combine the linux-omap gpio switch fwk ideas and android one to create generic switch framework. For complete thread information, please start from this link: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg07845.html -- ---Trilok Soni http://triloksoni.wordpress.com http://www.linkedin.com/in/triloksoni -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GPIO switch framework (was: Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update)
Definitely worth discussing -- I'm going to be out of the office for two weeks over the holidays, and most of the rest of our team is on vacation as well. I'll pass this on, but I'm not sure if people will have much time to take a look before January. [Trilok Soni soni.tri...@gmail.com] Hi Jani, On Thu, Dec 18, 2008 at 7:10 PM, Jani Nikula ext-jani.1.nik...@nokia.com wrote: On Thu, 2008-12-18 at 18:30 +0530, ext Trilok Soni wrote: Why limit to GPIO based switches only? GPIOs should be client of switch framework I think, and how h/w implements that switch should be hidden by client specific driver. You're quite right, it's better not to make such unnecessary limitations. All the more reason to discuss before anyone submits a truckload of patches for review. :) Let me CC Brian from Android here. Brian, we are discussing here of making generic switch framework and android-kernel also have near to generic switch fwk implementation, so that we can combine the linux-omap gpio switch fwk ideas and android one to create generic switch framework. For complete thread information, please start from this link: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg07845.html -- ---Trilok Soni http://triloksoni.wordpress.com http://www.linkedin.com/in/triloksoni -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
GPIO switch framework (was: Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update)
On Tue, 2008-12-16 at 11:35 +0530, ext Trilok Soni wrote: OK, I found other guys (android ??) using such home brew frameworks. Time to write a switch framework. To start a discussion on what such a GPIO switch framework should be like if someone were to write it, here's a list of the kind of things I'd like to see in it (mostly from gpio-switch.c): * Based on or integrated in the gpiolib. * Dynamically changeable notification callbacks in kernel. * Sysfs notifications to userspace. * Debouncing for the notifications to filter spurious events. * Dynamically adjustable debounce timeouts. * Arbitrary names for the GPIOs in sysfs instead of (or in addition to) the gpiolib style gpioN. Anything else? I didn't see anything in the Android source that couldn't be covered with this. Jani. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: GPIO switch framework (was: Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update)
On Thu, 2008-12-18 at 18:30 +0530, ext Trilok Soni wrote: Why limit to GPIO based switches only? GPIOs should be client of switch framework I think, and how h/w implements that switch should be hidden by client specific driver. You're quite right, it's better not to make such unnecessary limitations. All the more reason to discuss before anyone submits a truckload of patches for review. :) Jani. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update
On Mon, Dec 15, 2008 at 02:08:16PM +0200, Jani Nikula wrote: Add new function omap_update_gpio_switch() to support dynamically changing the GPIO switch notify callback functions and debounce timeouts. Why do they need to be changed? GPIO switches related to the board schematics, which do not hopefully change dynamically. + spin_lock_irqsave(sw-lock, flags); if (state) timeout = sw-debounce_rising; else timeout = sw-debounce_falling; + spin_unlock_irqrestore(sw-lock, flags); What's the point of this? The above read can be consider an atomic operation. + spin_lock_irqsave(sw-lock, flags); + notify = sw-notify; + notify_data = sw-notify_data; + spin_unlock_irqrestore(sw-lock, flags); + + if (notify != NULL) + notify(notify_data, state); What if omap_update_gpio_switch() is called just before the check for notify != NULL from another (hypothetical =)) CPU? Then you end up with the function completing, but still having the old notify callback called with old notify_data. Cheers, Juha -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update
On Mon, 2008-12-15 at 15:40 +0200, ext Juha Yrjölä wrote: On Mon, Dec 15, 2008 at 02:08:16PM +0200, Jani Nikula wrote: Add new function omap_update_gpio_switch() to support dynamically changing the GPIO switch notify callback functions and debounce timeouts. Why do they need to be changed? GPIO switches related to the board schematics, which do not hopefully change dynamically. The switches themselves don't change (nor does this patch support changing them), but different kinds of external devices may be connected to the GPIO swiches. It would be useful to be able to change the notification callbacks and debounce timeouts according to the device. + spin_lock_irqsave(sw-lock, flags); if (state) timeout = sw-debounce_rising; else timeout = sw-debounce_falling; + spin_unlock_irqrestore(sw-lock, flags); What's the point of this? The above read can be consider an atomic operation. Umm, I suppose I was more worried that the write might not be an atomic operation, messing up the read as well. But I'll take your word for it if you say the write is okay. :) + spin_lock_irqsave(sw-lock, flags); + notify = sw-notify; + notify_data = sw-notify_data; + spin_unlock_irqrestore(sw-lock, flags); + + if (notify != NULL) + notify(notify_data, state); What if omap_update_gpio_switch() is called just before the check for notify != NULL from another (hypothetical =)) CPU? Then you end up with the function completing, but still having the old notify callback called with old notify_data. I was, of course, making sure a NULL pointer is not called and there's no mismatch between old/new notify/notify_data. Your scenario might theoretically actually occur on a single processor system as well, don't you think? But is it actually a problem or not? And if yes, do you have any suggestions as to handling the case? If there's no need to lock for reading the debounce timeouts in gpio_sw_irq_handler() as you say above, do you think I could switch to a mutex and call notify callback holding that? BR, Jani. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update
On Mon, Dec 15, 2008 at 05:44:26PM +0530, ext Trilok Soni wrote: Hi Tony On Mon, Dec 15, 2008 at 5:38 PM, Jani Nikula ext-jani.1.nik...@nokia.com wrote: Add new function omap_update_gpio_switch() to support dynamically changing the GPIO switch notify callback functions and debounce timeouts. Signed-off-by: Jani Nikula ext-jani.1.nik...@nokia.com Not related to this patch, but why not we convert this omap specific gpio switch code into generic switch framework and attach gpios to that switch framework, other arm-linux platforms seems to also use such home-brew frameworks for this functionality it seems. Yes, would be cool. Move it to gpiolib, I'm sure there are other platforms that might get interested in this :-) Jani, do you plan to do that ?? -- balbi -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update
Hi Tony On Mon, Dec 15, 2008 at 5:38 PM, Jani Nikula ext-jani.1.nik...@nokia.com wrote: Add new function omap_update_gpio_switch() to support dynamically changing the GPIO switch notify callback functions and debounce timeouts. Signed-off-by: Jani Nikula ext-jani.1.nik...@nokia.com Not related to this patch, but why not we convert this omap specific gpio switch code into generic switch framework and attach gpios to that switch framework, other arm-linux platforms seems to also use such home-brew frameworks for this functionality it seems. -- ---Trilok Soni http://triloksoni.wordpress.com http://www.linkedin.com/in/triloksoni -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update
On Mon, Dec 15, 2008 at 04:52:32PM +0200, Jani Nikula wrote: Why do they need to be changed? GPIO switches related to the board schematics, which do not hopefully change dynamically. The switches themselves don't change (nor does this patch support changing them), but different kinds of external devices may be connected to the GPIO swiches. It would be useful to be able to change the notification callbacks and debounce timeouts according to the device. Could you give an example use-case? If the gpio-switch framework starts getting extended and used more, it might be sensible to convert it to the general GPIO API, as Trilok suggested. Umm, I suppose I was more worried that the write might not be an atomic operation, messing up the read as well. But I'll take your word for it if you say the write is okay. :) Integer reads/writes are atomic, unless the system has some serious cache coherency issues. And in that case, spinlocks won't save you either -- only the HW engineers can. =) What if omap_update_gpio_switch() is called just before the check for notify != NULL from another (hypothetical =)) CPU? Then you end up with the function completing, but still having the old notify callback called with old notify_data. I was, of course, making sure a NULL pointer is not called and there's no mismatch between old/new notify/notify_data. Your scenario might theoretically actually occur on a single processor system as well, don't you think? Ah, yes. I was under the impression that gpio_sw_handler() was called from IRQ context, but since it's a work handler, the function certainly can be preempted. But is it actually a problem or not? And if yes, do you have any suggestions as to handling the case? If there's no need to lock for reading the debounce timeouts in gpio_sw_irq_handler() as you say above, do you think I could switch to a mutex and call notify callback holding that? Especially in the case of frameworks, it's good to make things as correct as possible. A mutex is safe. Cheers, Juha -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update
On Mon, Dec 15, 2008 at 03:44:05PM +0200, Jani Nikula wrote:
That's a nice idea, but gpio_sw_irq_handler() actually supports having 0
debounce timeouts, i.e. no settling time. Of course, I could use -1 for
don't update. However, the semantics above is exactly the same as in
add_board_switches() for the update case. I'm not sure if it would be a
good idea to deviate from this - what do you think?
makes sense to me. But then again, if the uses does something like:
static struct omap_gpio_switch *cfg;
static int __init blabla_probe(struct platform_device *dev)
{
cfg = kmalloc(sizeof(cfg), GFP_KERNEL));
cfg-notify = my_notify;
omap_update_gpio_swtich(cfg);
return 0;
};
that means that if cfg-debounce_rising was different than 0, it'll get
overwritten to 0, right ? So, at least, you should put a big note for
users to initialize all necessary fields. Or, again, you only change if
cfg-whatever is different than 0 (or NULL), but that could a problem
when you really wanna change cfg-debounce_rising from, say, 100 to 0.
That wouldn't happen :-p
--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update
On Mon, 2008-12-15 at 17:29 +0200, ext Juha Yrjölä wrote: On Mon, Dec 15, 2008 at 04:52:32PM +0200, Jani Nikula wrote: The switches themselves don't change (nor does this patch support changing them), but different kinds of external devices may be connected to the GPIO swiches. It would be useful to be able to change the notification callbacks and debounce timeouts according to the device. Could you give an example use-case? For example a jack that supports headsets with a button. The button could function by simply grounding one of the lines (connected to a GPIO, of course) or by generating some sort of an interrupt pulse. The jack might support other kinds of accessories as well. And the required actions and settling times might differ vastly. If the gpio-switch framework starts getting extended and used more, it might be sensible to convert it to the general GPIO API, as Trilok suggested. I agree with this, but I'm afraid I don't have the time to extend the scope of my work that much right now. BR, Jani. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update
On Mon, 2008-12-15 at 15:22 +0200, Felipe Balbi wrote:
Hi,
one comment below
On Mon, Dec 15, 2008 at 02:08:16PM +0200, ext Jani Nikula wrote:
+int omap_update_gpio_switch(const struct omap_gpio_switch *cfg)
+{
+ unsigned long flags;
+ struct gpio_switch *sw = find_switch(cfg-gpio, cfg-name);
+
+ if (!sw)
+ return -EINVAL;
+
+ spin_lock_irqsave(sw-lock, flags);
+ sw-debounce_rising = cfg-debounce_rising;
+ sw-debounce_falling = cfg-debounce_falling;
+ sw-notify = cfg-notify;
+ sw-notify_data = cfg-notify_data;
+ spin_unlock_irqrestore(sw-lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL(omap_update_gpio_switch);
how about you only change what's not null ?? then you could only change
the notify callback and keep the same debounce_rising/falling ??
That's a nice idea, but gpio_sw_irq_handler() actually supports having 0
debounce timeouts, i.e. no settling time. Of course, I could use -1 for
don't update. However, the semantics above is exactly the same as in
add_board_switches() for the update case. I'm not sure if it would be a
good idea to deviate from this - what do you think?
Jani.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: Add support for dynamic GPIO switch update
Hi,
one comment below
On Mon, Dec 15, 2008 at 02:08:16PM +0200, ext Jani Nikula wrote:
+int omap_update_gpio_switch(const struct omap_gpio_switch *cfg)
+{
+ unsigned long flags;
+ struct gpio_switch *sw = find_switch(cfg-gpio, cfg-name);
+
+ if (!sw)
+ return -EINVAL;
+
+ spin_lock_irqsave(sw-lock, flags);
+ sw-debounce_rising = cfg-debounce_rising;
+ sw-debounce_falling = cfg-debounce_falling;
+ sw-notify = cfg-notify;
+ sw-notify_data = cfg-notify_data;
+ spin_unlock_irqrestore(sw-lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL(omap_update_gpio_switch);
how about you only change what's not null ?? then you could only change
the notify callback and keep the same debounce_rising/falling ??
--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
