Hello,
On Friday, July 09, 2010 11:25 PM Andrew Morton wrote:
On Wed, 16 Jun 2010 08:49:56 +0200
> Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote:
>
> > On some Samsung SoCs not all SDHCI controllers have card detect (CD)
> > line. For some embedded designs it is not even needed, because ususally
> > the device (like SDIO flash memory or wifi controller) is permanently
> > wired to the controller. There are also systems which have a card detect
> > line connected to some of the external interrupt lines or the presence
> > of the card depends on some other actions (like enabling a power
> > regulator).
> >
> > This patch adds support for all these cases. The following card
> > detection methods are possible:
> >
> > 1. internal sdhci host card detect line
> > 2. external event
> > 3. external gpio interrupt
> > 4. no card detect line, controller will poll for the card
> > 5. no card detect line, card is permanently wired to the controller
> > (once detected host won't poll it any more)
> >
> > By default, all existing code would use method #1, what is compatible
> > with the previous version of the driver.
> >
> > In case of external event, two callbacks must be provided in platdata:
> > ext_cd_init and ext_cd_cleanup. Both of them get a callback to a
> > function that notifies the s3c-sdhci host contoller as their argument.
> > That callback function should be called from the even dispatcher to let
> > host notice the card insertion/removal.
> >
> > In case of external gpio interrupt, a gpio pin number must be provided
> > in platdata (ext_cd_gpio parameter), as well as the information about
> > the polarity of that gpio pin (ext_cd_gpio_invert). By default
> > (ext_cd_gpio_invert == 0) gpio value 0 means 'card has been removed',
> > but this can be changed to 'card has been removed' when
> > ext_cd_gpio_invert == 1.
> >
> > This patch adds changes to sdhci-s3c driver.
> >
> > ...
> >
> > +static void sdhci_s3c_notify_change(struct platform_device *dev, int
> state)
> > +{
> > + struct sdhci_host *host;
> > + unsigned long flags;
> > +
> > + local_irq_save(flags);
> > + host = platform_get_drvdata(dev);
> > + if (host) {
> > + if (state) {
> > + dev_dbg(&dev->dev, "card inserted.\n");
> > + host->flags &= ~SDHCI_DEVICE_DEAD;
> > + host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > + tasklet_schedule(&host->card_tasklet);
> > + } else {
> > + dev_dbg(&dev->dev, "card removed.\n");
> > + host->flags |= SDHCI_DEVICE_DEAD;
> > + host->quirks &= ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > + tasklet_schedule(&host->card_tasklet);
> > + }
> > + }
> > + local_irq_restore(flags);
> > +}
>
> What's the local_irq_save() there for?
>
> Presumably it is for local-cpu-only protection of some data. But which
> data is it there to protect?
>
> It doesn't provide protection on SMP systems and if I'm guessing
> correctly about why it was added, it would be much better to use
> spin_lock_irq[save]() here. That sets a better example, it means the
> code has a hope of working correctly on SMP systems and will devolve to
> local_irq_save() on UP kernels anyway.
Ok. I will change local_irq_save to spin_lock_irqsave. Thanks for spotting
this!
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
