* sdhci: Best Practice Question
@ 2010-09-25 20:08 Philip Rakity
2010-09-26 0:43 ` [PATCH] sdhci: sepearate get_ro into code that calls platform and helper Philip Rakity
2010-09-26 16:40 ` sdhci: Best Practice Question Chris Ball
0 siblings, 2 replies; 5+ messages in thread
From: Philip Rakity @ 2010-09-25 20:08 UTC (permalink / raw)
To: linux-mmc@vger.kernel.org
and a
host->ops-> get_max_clock()
function that is used to set the max clock. The code snippet is below
host->max_clk =
(caps & SDHCI_CLOCK_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
host->max_clk *= 1000000;
if (host->max_clk == 0 || host->quirks &
SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN) {
if (!host->ops->get_max_clock) {
printk(KERN_ERR
"%s: Hardware doesn't specify base clock "
"frequency.\n", mmc_hostname(mmc));
return -ENODEV;
}
host->max_clk = host->ops->get_max_clock(host);
}
The quirk seems redundant since this could be coded as follows to remove the need for the quirk
> + if (host->ops->get_max_clock)
> host->max_clk = host->ops->get_max_clock(host);
> + else {
> + host->max_clk =
> + (caps & SDHCI_CLOCK_BASE_MASK)
> + >> SDHCI_CLOCK_BASE_SHIFT;
> + host->max_clk *= 1000000;
> + }
> +
> + if (host->max_clk == 0) {
> + printk(KERN_ERR
> + "%s: Hardware doesn't specify base clock "
> + "frequency.\n", mmc_hostname(mmc));
> + return -ENODEV;
> }
>
This approach was not used since it was felt a quirk was needed.
http://kerneltrap.org/mailarchive/linux-kernel/2010/2/19/4540115
but the quirk redundant and it requires two items be set; the quirk and the callback.
Defining the quirk and not the callback is meaningless.
The change proposed by Richard Zhu for handling write protect uses only a callback.
<snip>
static int sdhci_get_ro(struct mmc_host *mmc)
{
struct sdhci_host *host;
unsigned long flags;
int present;
host = mmc_priv(mmc);
if (host->ops->get_ro)
return host->ops->get_ro(host);
spin_lock_irqsave(&host->lock, flags);
<end snip>
What is the correct practice?
Philip
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] sdhci: sepearate get_ro into code that calls platform and helper 2010-09-25 20:08 sdhci: Best Practice Question Philip Rakity @ 2010-09-26 0:43 ` Philip Rakity 2010-09-26 0:44 ` [PATCH] sdhci: seperate out reset so if platform specific helper exists it is called Philip Rakity 2010-09-26 16:40 ` sdhci: Best Practice Question Chris Ball 1 sibling, 1 reply; 5+ messages in thread From: Philip Rakity @ 2010-09-26 0:43 UTC (permalink / raw) To: linux-mmc@vger.kernel.org >From 0037e6513afc3fc39dd39010f994278c84623b29 Mon Sep 17 00:00:00 2001 From: Philip Rakity <prakity@marvell.com> Date: Sat, 25 Sep 2010 17:23:59 -0700 Subject: [PATCH] sdhci: sepearate get_ro into code that calls platform and helper If a helper function is supplied allow it to make use of the helper. This allows specific behavior to be added when needed and reuse the standard code Signed-off-by: Philip Rakity <prakity@marvell.com> --- drivers/mmc/host/sdhci.c | 14 +++++++++++++- drivers/mmc/host/sdhci.h | 1 + 2 files changed, 14 insertions(+), 1 deletions(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 0774dad..6f2bc4b 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -1204,7 +1204,7 @@ out: spin_unlock_irqrestore(&host->lock, flags); } -static int sdhci_get_ro(struct mmc_host *mmc) +int sdhci_get_ro_helper(struct mmc_host *mmc) { struct sdhci_host *host; unsigned long flags; @@ -1226,6 +1226,18 @@ static int sdhci_get_ro(struct mmc_host *mmc) return !(present & SDHCI_WRITE_PROTECT); } +int sdhci_get_ro(struct mmc_host *mmc) +{ + struct sdhci_host *host; + + host = mmc_priv(mmc); + + if (host->ops->get_ro) + return host->ops->get_ro(mmc); + else + return sdhci_get_ro_helper(mmc); +} + static void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable) { struct sdhci_host *host; diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index f68af47..bd1fa15 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -324,6 +324,7 @@ struct sdhci_ops { unsigned int (*get_min_clock)(struct sdhci_host *host); unsigned int (*get_timeout_clock)(struct sdhci_host *host); void (*platform_specific_reset)(struct sdhci_host *host, u8 mask); + int (*get_ro)(struct mmc_host *mmc); }; #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] sdhci: seperate out reset so if platform specific helper exists it is called 2010-09-26 0:43 ` [PATCH] sdhci: sepearate get_ro into code that calls platform and helper Philip Rakity @ 2010-09-26 0:44 ` Philip Rakity 2010-09-26 16:45 ` [RFC] sdhci: SDHCI_QUIRK_BROKEN_CARD_DETECTION Philip Rakity 0 siblings, 1 reply; 5+ messages in thread From: Philip Rakity @ 2010-09-26 0:44 UTC (permalink / raw) To: linux-mmc@vger.kernel.org >From ae9c828941b7d170881ef62190b6d5d90d122ece Mon Sep 17 00:00:00 2001 From: Philip Rakity <prakity@marvell.com> Date: Sat, 25 Sep 2010 17:36:42 -0700 Subject: [PATCH] sdhci: seperate out reset so if platform specific helper exists it is called if it exists otherwise standard reset function is called. This way the platform specific code and use the standard code if needed. for example in our driver we need to reset some platform specific registers depending on the type of reset a) if not reset_all -> just call the helper b) if reset_all --> call the helper then do our private stuff Signed-off-by: Philip Rakity <prakity@marvell.com> --- drivers/mmc/host/sdhci.c | 10 +++++++++- drivers/mmc/host/sdhci.h | 1 + 2 files changed, 10 insertions(+), 1 deletions(-) diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 17f1554..451fad3 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -138,7 +138,7 @@ static void sdhci_disable_card_detection(struct sdhci_host *host) sdhci_set_card_detection(host, false); } -static void sdhci_reset(struct sdhci_host *host, u8 mask) +void sdhci_reset_helper(struct sdhci_host *host, u8 mask) { unsigned long timeout; u32 uninitialized_var(ier); @@ -176,6 +176,14 @@ static void sdhci_reset(struct sdhci_host *host, u8 mask) sdhci_clear_set_irqs(host, SDHCI_INT_ALL_MASK, ier); } +static void sdhci_reset(struct sdhci_host *host, u8 mask) +{ + if (host->ops->sdhci_reset) + return host->ops->sdhci_reset(host, mask); + else + return sdhci_reset_helper(host, mask); +} + static void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios); static void sdhci_init(struct sdhci_host *host, int soft) diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index a41ad8c..f8c18cd 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -323,6 +323,7 @@ struct sdhci_ops { unsigned int (*get_max_clock)(struct sdhci_host *host); unsigned int (*get_min_clock)(struct sdhci_host *host); unsigned int (*get_timeout_clock)(struct sdhci_host *host); + void (*sdhci_reset)(struct sdhci_host *host, u8 mask); int (*get_ro)(struct mmc_host *mmc); }; -- 1.6.0.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [RFC] sdhci: SDHCI_QUIRK_BROKEN_CARD_DETECTION 2010-09-26 0:44 ` [PATCH] sdhci: seperate out reset so if platform specific helper exists it is called Philip Rakity @ 2010-09-26 16:45 ` Philip Rakity 0 siblings, 0 replies; 5+ messages in thread From: Philip Rakity @ 2010-09-26 16:45 UTC (permalink / raw) To: linux-mmc@vger.kernel.org The quirk SDHCI_QUIRK_BROKEN_CARD_DETECTION is not quite what may needed for embedded systems. Sometimes the card is hard wired onto the board and is always present -- usually the signal into the controller is not used by the board designed -- saves a pin Sometimes the card is plugged into a slot and Card Detect REALLY is broken. In this case we need to poll by setting MMC_CAP_NEEDS_POLL so mmc_detect_change() will find the card. if (host->caps & MMC_CAP_NEEDS_POLL) mmc_schedule_delayed_work(&host->detect, HZ); Since the behavior is a board issue I think it makes more sense to define a new struct sdhci_ops { } that gets called and the adaption layer can decide to set or not set MMC_CAP_NEEDS_POLL. if this is okay will generate the patch. Sample code snippets below struct sdhci_ops { ...... void (*card_detect)(struct sdhci_host *host); } ==== replace if (caps & SDHCI_CAN_DO_HISPD) mmc->caps |= MMC_CAP_SD_HIGHSPEED; if (host->quirks & SDHCI_QUIRK_BROKEN_CARD_DETECTION) mmc->caps |= MMC_CAP_NEEDS_POLL; with if (caps & SDHCI_CAN_DO_HISPD) mmc->caps |= MMC_CAP_SD_HIGHSPEED; if (host->ops->card_detect) host->ops->card_detect(host) ==== platform code void card_detect (struct sdhci_host *host) { if (card_hard_wired || sd_slot_has_no_card_detect_signal) { host->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION; if (sd_slot_has_no_card_detect_signal) host->mmc->caps |= MMC_CAP_NEEDS_POLL } } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: sdhci: Best Practice Question 2010-09-25 20:08 sdhci: Best Practice Question Philip Rakity 2010-09-26 0:43 ` [PATCH] sdhci: sepearate get_ro into code that calls platform and helper Philip Rakity @ 2010-09-26 16:40 ` Chris Ball 1 sibling, 0 replies; 5+ messages in thread From: Chris Ball @ 2010-09-26 16:40 UTC (permalink / raw) To: Philip Rakity; +Cc: linux-mmc@vger.kernel.org Hi Philip, On Sat, Sep 25, 2010 at 01:08:20PM -0700, Philip Rakity wrote: > The change proposed by Richard Zhu for handling write protect uses > only a callback. > > <snip> > static int sdhci_get_ro(struct mmc_host *mmc) > { > struct sdhci_host *host; > unsigned long flags; > int present; > > host = mmc_priv(mmc); > > if (host->ops->get_ro) > return host->ops->get_ro(host); > > spin_lock_irqsave(&host->lock, flags); > > <end snip> > > What is the correct practice? I think that the get_ro hook is reasonable in this case -- we're saying that the host has a sufficiently weird WP setup that sdhci doesn't know what we're supposed to do (unlike SDHCI_QUIRK_INVERTED_WRITE_PROTECT). I'd be curious to hear what others think, though. Should we be simply moving away from adding new quirks, or just limiting them to cases where a full hook isn't warranted? -- Chris Ball <cjb@laptop.org> <http://printf.net/> One Laptop Per Child ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-09-26 16:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-25 20:08 sdhci: Best Practice Question Philip Rakity 2010-09-26 0:43 ` [PATCH] sdhci: sepearate get_ro into code that calls platform and helper Philip Rakity 2010-09-26 0:44 ` [PATCH] sdhci: seperate out reset so if platform specific helper exists it is called Philip Rakity 2010-09-26 16:45 ` [RFC] sdhci: SDHCI_QUIRK_BROKEN_CARD_DETECTION Philip Rakity 2010-09-26 16:40 ` sdhci: Best Practice Question Chris Ball
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.