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.liakhovetski@xxxxxx> 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.liakhovetski@xxxxxx>
> > ---
> > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
