Hi Magnus Thanks for your review(s)!
On Fri, 6 Jan 2012, Magnus Damm wrote: > On Wed, Jan 4, 2012 at 11:17 PM, Guennadi Liakhovetski > <g.liakhovet...@gmx.de> wrote: > > If the platform specified a GPIO number and IRQ trigger polarity flags, use > > the generic MMC GPIO card hotplug helper. > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovet...@gmx.de> > > --- > > drivers/mmc/host/tmio_mmc.h | 4 -- > > drivers/mmc/host/tmio_mmc_pio.c | 69 > > +++++++++++++++++---------------------- > > include/linux/mfd/tmio.h | 19 +++++++--- > > 3 files changed, 43 insertions(+), 49 deletions(-) > > diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h > > index 0dc9804..4ef9307 100644 > > --- a/include/linux/mfd/tmio.h > > +++ b/include/linux/mfd/tmio.h > > @@ -1,8 +1,10 @@ > > #ifndef MFD_TMIO_H > > #define MFD_TMIO_H > > > > +#include <linux/device.h> > > #include <linux/fb.h> > > #include <linux/io.h> > > +#include <linux/jiffies.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > > > @@ -64,8 +66,8 @@ > > #define TMIO_MMC_SDIO_IRQ (1 << 2) > > /* > > * Some platforms can detect card insertion events with controller powered > > - * down, in which case they have to call tmio_mmc_cd_wakeup() to power up > > the > > - * controller and report the event to the driver. > > + * down, using a GPIO IRQ, in which case they have to fill in cd_irq, > > cd_gpio, > > + * and cd_flags fields of struct tmio_mmc_data. > > */ > > #define TMIO_MMC_HAS_COLD_CD (1 << 3) > > /* > > @@ -98,18 +100,23 @@ struct tmio_mmc_data { > > struct tmio_mmc_dma *dma; > > struct device *dev; > > bool power; > > + unsigned int cd_gpio; > > + unsigned int cd_irq; > > + unsigned long cd_flags; > > void (*set_pwr)(struct platform_device *host, int state); > > void (*set_clk_div)(struct platform_device *host, int state); > > int (*get_cd)(struct platform_device *host); > > int (*write16_hook)(struct tmio_mmc_host *host, int addr); > > }; > > This looks much easier than callbacks and notifiers. Thanks! > > But why do you need to pass three values here? I expected it to be > sufficient with GPIO and perhaps also with trigger flags. Shouldn't > gpio_to_irq() give you the IRQ from the GPIO? Well, it could:-) What about sh7372? sh73a0 does implement it, whereas sh7372 only returns an error. And - yes, we can fix that one. But this tells me - there can be other platforms like that, and they might not get fixed quickly enough. Should we really refuse to work with them? We could make this parameter optional: set to to an error code and the cd-gpio will try to use gpio_to_irq(). If it fails - no luck. > And why on earth would > we want to trigger on something else than both edges? Both edges seems like a logical choice to me too. But - remember the time, when one of sh-mobile platforms didn't implement triggering on both edges? Until we've implemented a software workaround, whereby we switch to the opposite edge after each interrupt? Are you sure there are no such platforms around left? > The SoC code may me rather limited in terms of supporting > gpio_to_irq() but that needs to be fixed then. On SH-Mobile the PFC > code has been updated to allow this, so it's only SoC-specific code > missing. Anyone with access to hardware and data sheet can do this, > and if there are some issues with this then I'd be happy to help > myself. > > On the cosmetic side, wouldn't it make sense to have a structure of > these values and just pass that along? Otherwise people will mix and > match short and ints for the GPIO and it may become rather messy over > time. Short for GPIO? Why would anyone use that and how can that become messy?:-) In principle - I'm basically fine either way. If you really prefer a struct here - we can do that too, yes. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html