Re: [PATCH 3/4] [MMC] mmc_spi: add polling support for the card detect line
On Thu, May 22, 2008 at 09:34:32PM +0200, Pierre Ossman wrote: [...] Ok. How about this version? Perfect. Unless there is some way you can pass flags in platform data? Using the lack of an init() to determine the need for polling is a bit circumstantial. Aha, that's an idea, thanks. Implemented in the updated patches. p.s. I think, in case of capabilities, it's users' responsibility to not issue bogus flags, so mmc_spi doesn't mask anything, it just passes platform's caps. This also avoids drivers update when (if) new caps will be implemented. -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/4] [MMC] mmc_spi: add polling support for the card detect line
On Wed, May 21, 2008 at 09:28:31PM +0200, Pierre Ossman wrote:
On Wed, 21 May 2008 22:47:13 +0400
Anton Vorontsov [EMAIL PROTECTED] wrote:
Calling get_cd() for every request smells like overhead, especially given
that that get_cd() could ask for GPIO status via relatively slow bus (like
I2C GPIO expanders). So, polling seems most reasonable solution here, no
need to call it very often.
Fair enough. You should probably add a comment about this somewhere so
that people do not call get_cd() in the core request function and
similar places. Place it so that both get_cd() and get_ro() are covered
though, as it should be relevant for both.
I think this is applicable to the .set_ios() too.
[...]
+ if (host-ops-get_cd) {
+ int old_cd_status = host-cd_status;
+
+ host-cd_status = !!host-ops-get_cd(host);
+ if (!(old_cd_status ^ host-cd_status)) {
+ mmc_bus_put(host);
+ goto out;
+ }
+ }
+
This should only be done when there is no bus handler. Since we are
polling, we might actually miss the user removing and reinserting the
card. The only way to check for that is to poke the card and see if it
is still alive. This also means you won't need that state variable.
Yeah, this makes sense. Indeed pretty easy to trigger [if poll interval
increased to 3 seconds, for example].
Also, that second if clause seems fit for an obfuscation contest. ;)
cd_status was a bitfield, so I thought that bit operations would be
appropriate. :-)
p.s. Since mmc_host_ops no longer the same for every instance of
mmc_spi, struct mmc_host_ops can't be const and should be allocated
dynamically.
This can be solved by allowing get_cd() to return an error that will be
treated as if get_cd() wasn't defined. -ENODEV seems suitable.
-ENOSYS (not implemented) sounds better for this purpose...
(get_ro() needs the same treatment, but I haven't gotten around to
that)
Ok. How about this version?
--
Anton Vorontsov
email: [EMAIL PROTECTED]
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/4] [MMC] mmc_spi: add polling support for the card detect line
On Sat, May 17, 2008 at 01:36:33PM +0200, Pierre Ossman wrote: On Fri, 16 May 2008 20:50:57 +0400 Anton Vorontsov [EMAIL PROTECTED] wrote: Some boards do not use interrupts on the CD line, so we want to poll the CD and see if there was a change. 1 second poll interval seems resonable. The idea isn't bad, but I'm not sure about the mechanism. To poll a MMC slot, you do not really need a card detect at all. The MMC layer can just shoot off some requests and see if anything responds. The PXA driver (if my memory serves me right) already does this. So the best idea there would be to add this feature to the MMC core and let the host indicate that it needs it via MMC_CAP_NEEDS_POLL or something like that. The card detection pin then becomes an optimisation, something that is also useful in other ways. Have the host driver check the card detection pin at the start of every request, and quickly fail it if there is no card present. Calling get_cd() for every request smells like overhead, especially given that that get_cd() could ask for GPIO status via relatively slow bus (like I2C GPIO expanders). So, polling seems most reasonable solution here, no need to call it very often. How about these patches? Tested with and without get_cd() optimization. -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 3/4] [MMC] mmc_spi: add polling support for the card detect line
On Fri, 16 May 2008 20:50:57 +0400 Anton Vorontsov [EMAIL PROTECTED] wrote: Some boards do not use interrupts on the CD line, so we want to poll the CD and see if there was a change. 1 second poll interval seems resonable. The idea isn't bad, but I'm not sure about the mechanism. To poll a MMC slot, you do not really need a card detect at all. The MMC layer can just shoot off some requests and see if anything responds. The PXA driver (if my memory serves me right) already does this. So the best idea there would be to add this feature to the MMC core and let the host indicate that it needs it via MMC_CAP_NEEDS_POLL or something like that. The card detection pin then becomes an optimisation, something that is also useful in other ways. Have the host driver check the card detection pin at the start of every request, and quickly fail it if there is no card present. That should give you what you want with much more flexibility for other uses as well. Rgds -- -- Pierre Ossman Linux kernel, MMC maintainerhttp://www.kernel.org rdesktop, core developer http://www.rdesktop.org ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH 3/4] [MMC] mmc_spi: add polling support for the card detect line
Some boards do not use interrupts on the CD line, so we want to poll
the CD and see if there was a change. 1 second poll interval seems
resonable.
Signed-off-by: Anton Vorontsov [EMAIL PROTECTED]
---
drivers/mmc/host/mmc_spi.c | 51 +-
include/linux/spi/mmc_spi.h | 10
2 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 3550858..a3b46b1 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -31,6 +31,7 @@
#include linux/crc7.h
#include linux/crc-itu-t.h
#include linux/scatterlist.h
+#include linux/workqueue.h
#include linux/mmc/host.h
#include linux/mmc/mmc.h /* for R1_SPI_* bit values */
@@ -122,6 +123,11 @@ struct mmc_spi_host {
struct mmc_spi_platform_data*pdata;
+ /* stores last Card-Detect status (when polling) */
+ int cd_status;
+ struct workqueue_struct *cd_poll_wqueue;
+ struct delayed_work cd_poll_work;
+
/* for bulk data transfers */
struct spi_transfer token, t, crc, early_status;
struct spi_message m;
@@ -1155,6 +1161,26 @@ mmc_spi_detect_irq(int irq, void *mmc)
return IRQ_HANDLED;
}
+static void mmc_spi_cd_poll_work(struct work_struct *work)
+{
+ struct mmc_spi_host *host = container_of(work, struct mmc_spi_host,
+cd_poll_work.work);
+ struct mmc_host *mmc = host-mmc;
+ int old_cd;
+
+ dev_dbg(host-spi-dev, polling for card detect...\n);
+
+ old_cd = host-cd_status;
+ host-cd_status = host-pdata-get_cd(mmc-parent);
+ if (old_cd != host-cd_status) {
+ /* ugh... this is ugly, but better than code duplication */
+ mmc_spi_detect_irq(NO_IRQ, mmc);
+ }
+
+ queue_delayed_work(host-cd_poll_wqueue, host-cd_poll_work,
+ MMC_SPI_POLL_INT);
+}
+
struct count_children {
unsignedn;
struct bus_type *bus;
@@ -1323,13 +1349,28 @@ static int mmc_spi_probe(struct spi_device *spi)
if (status != 0)
goto fail_add_host;
- dev_info(spi-dev, SD/MMC host %s%s%s%s\n,
+ if (host-pdata host-pdata-get_cd) {
+ host-cd_status = host-pdata-get_cd(mmc-parent);
+ INIT_DELAYED_WORK(host-cd_poll_work, mmc_spi_cd_poll_work);
+ host-cd_poll_wqueue = create_singlethread_workqueue(
+ mmc-class_dev.bus_id);
+ if (!host-cd_poll_wqueue) {
+ status = -ENOMEM;
+ goto fail_add_host;
+ }
+ queue_delayed_work(host-cd_poll_wqueue, host-cd_poll_work,
+ MMC_SPI_POLL_INT);
+ }
+
+ dev_info(spi-dev, SD/MMC host %s%s%s%s%s\n,
mmc-class_dev.bus_id,
host-dma_dev ? : , no DMA,
(host-pdata host-pdata-get_ro)
? : , no WP,
(host-pdata host-pdata-setpower)
- ? : , no poweroff);
+ ? : , no poweroff,
+ (host-pdata host-pdata-get_cd)
+ ? , cd polling : );
return 0;
fail_add_host:
@@ -1362,6 +1403,12 @@ static int __devexit mmc_spi_remove(struct spi_device
*spi)
if (host-pdata host-pdata-exit)
host-pdata-exit(spi-dev, mmc);
+ if (host-pdata host-pdata-get_cd) {
+ cancel_rearming_delayed_workqueue(
+ host-cd_poll_wqueue, host-cd_poll_work);
+ destroy_workqueue(host-cd_poll_wqueue);
+ }
+
mmc_remove_host(mmc);
if (host-dma_dev) {
diff --git a/include/linux/spi/mmc_spi.h b/include/linux/spi/mmc_spi.h
index e9bbe3e..6ed6ee9 100644
--- a/include/linux/spi/mmc_spi.h
+++ b/include/linux/spi/mmc_spi.h
@@ -1,6 +1,10 @@
#ifndef __LINUX_SPI_MMC_SPI_H
#define __LINUX_SPI_MMC_SPI_H
+#include asm/param.h /* for HZ */
+
+#define MMC_SPI_POLL_INT HZ
+
struct device;
struct mmc_host;
@@ -21,6 +25,12 @@ struct mmc_spi_platform_data {
/* sense switch on sd cards */
int (*get_ro)(struct device *);
+ /*
+* if board does not use CD interrupts, driver can poll the CD
+* line using this function.
+*/
+ int (*get_cd)(struct device *);
+
/* how long to debounce card detect, in msecs */
u16 detect_delay;
--
1.5.5.1
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
