Re: [PATCH 3/7] au1xmmc: remove db1x00 board-specific functions fromdriver

From: Sergei Shtylyov
Date: Mon May 12 2008 - 07:44:21 EST

Hello.

Manuel Lauss wrote:
From 47ecf116465ed850d2202880f7795fcee4826184 Mon Sep 17 00:00:00 2001
From: Manuel Lauss <mlau@xxxxxxxxxx>
Date: Wed, 7 May 2008 14:57:01 +0200
Subject: [PATCH] au1xmmc: remove db1x00 board-specific functions from driver

Remove the DB1200 board-specific functions (card present, read-only
methods) and instead add platform data which is passed to the driver.
This allows for platforms to implement other carddetect schemes
(e.g. dedicated irq) without having to pollute the driver code.
The poll timer (used for pb1200) is kept for compatibility.

With the board-specific stuff gone, the driver no longer needs to know
how many physical controllers the silicon actually has; every device
can be registered as needed, update the code to reflect that.

Signed-off-by: Manuel Lauss <mano@xxxxxxxxxxxxxxxxxxxxxxx>
[...]
diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
index cc5f7bc..8660f86 100644
--- a/drivers/mmc/host/au1xmmc.c
+++ b/drivers/mmc/host/au1xmmc.c
@@ -49,7 +49,6 @@
#include <asm/mach-au1x00/au1100_mmc.h>
#include <au1xxx.h>
-#include "au1xmmc.h"
I think you should merge the header to the driver in a separate patch.
@@ -174,8 +221,6 @@ static void au1xmmc_finish_request(struct au1xmmc_host *host)
host->status = HOST_S_IDLE;
- bcsr->disk_leds |= (1 << 8);
-
So, the LED support is gone with your patch? You should at least document this...
mmc_request_done(host->mmc, mrq);
}
@@ -663,7 +708,9 @@ static void au1xmmc_request(struct mmc_host* mmc, struct mmc_request* mrq)
host->mrq = mrq;
host->status = HOST_S_CMD;
- bcsr->disk_leds &= ~(1 << 8);
+ au_writel(0, HOST_STATUS(host));
+ au_sync();
+ FLUSH_FIFO(host);
Hm, not an obvious change...
if (mrq->data) {
FLUSH_FIFO(host);
@@ -749,118 +796,87 @@ static void au1xmmc_dma_callback(int irq, void *dev_id)
static irqreturn_t au1xmmc_irq(int irq, void *dev_id)
{
-
+ struct au1xmmc_host *host = dev_id;
u32 status;
- int i, ret = 0;
- disable_irq(AU1100_SD_IRQ);
+ status = au_readl(HOST_STATUS(host));
- for(i = 0; i < AU1XMMC_CONTROLLER_COUNT; i++) {
- struct au1xmmc_host * host = au1xmmc_hosts[i];
- u32 handled = 1;
+ if (!(status & (1 << 15)))
Why not SD_STATUS_I?
+ //IRQ_OFF(host, SD_CONFIG_TH|SD_CONFIG_RA|SD_CONFIG_RF);
No C99 // comments please -- checkpatch.pl would have given you error about them...
+ } else if (status & 0x203FBC70) {
I think the mask should be changed to 0x203F3C70 since you're handling SD_STATUS_I but maybe I'm wrong...
-static void au1xmmc_init_dma(struct au1xmmc_host *host)
+static int au1xmmc_init_dma(struct au1xmmc_host *host)
I'd have called it au1xmmc_init_dbdma() instead since in Au1100 the controller works with its "old-style" DMA... though maybe the difference could be handled within this function via #ifdef...
@@ -878,116 +896,201 @@ static const struct mmc_host_ops au1xmmc_ops = {
.get_ro = au1xmmc_card_readonly,
};
-static int __devinit au1xmmc_probe(struct platform_device *pdev)
+static void au1xmmc_poll_event(unsigned long arg)
{
+ struct au1xmmc_host *host = (struct au1xmmc_host *)arg;
Don't need new line here...
- int i, ret = 0;
+ int card = au1xmmc_card_inserted(host);
+ int controller = (host->flags & HOST_F_ACTIVE) ? 1 : 0;
Remove extra space please. And what does this variable actually mean?
+ host->iobase = (unsigned long)ioremap(r->start, 0xff);
You have the r->end specifying the resource end, why 0xff (well, actually 0x3c is enough)
@@ -1004,21 +1107,32 @@ static struct platform_driver au1xmmc_driver = {
static int __init au1xmmc_init(void)
{
+ if (dma) {
+ /* DSCR_CMD0_ALWAYS has a stride of 32 bits, we need a stride
+ * of 8 bits. And since devices are shared, we need to create
+ * our own to avoid freaking out other devices
Missing period at end of statement.
+ */
+ if (!memid)
Hm, is there a chance that it won't be NULL?
+ memid = au1xxx_ddma_add_device(&au1xmmc_mem_dbdev);
+ if (!memid) {
+ printk(KERN_ERR "au1xmmc: cannot add memory dma dev\n");
+ return -ENODEV;
+ }
+ }
return platform_driver_register(&au1xmmc_driver);
}
[...]
diff --git a/include/asm-mips/mach-au1x00/au1100_mmc.h b/include/asm-mips/mach-au1x00/au1100_mmc.h
index 9e0028f..6474fac 100644
--- a/include/asm-mips/mach-au1x00/au1100_mmc.h
+++ b/include/asm-mips/mach-au1x00/au1100_mmc.h
@@ -38,15 +38,46 @@
#ifndef __ASM_AU1100_MMC_H
#define __ASM_AU1100_MMC_H
[...]
+struct au1xmmc_platdata {
I'd suggest au1xmmc_platform_data.
+ int(*cd_setup)(void *mmc_host, int on);
+ int(*card_inserted)(void *mmc_host);
+ int(*card_readonly)(void *mmc_host);
+ void(*set_power)(void *mmc_host, int state);
+};
+
+struct au1xmmc_host {
+ struct mmc_host *mmc;
+ struct mmc_request *mrq;
+
+ u32 id;
+
+ u32 flags;
+ u32 iobase;
+ u32 clock;
+
+ int status;
+
+ struct {
+ int len;
+ int dir;
+ u32 tx_chan;
+ u32 rx_chan;
+ } dma;
+
+ struct {
+ int index;
+ int offset;
+ int len;
+ } pio;
+
+ struct timer_list timer;
+ struct tasklet_struct finish_task;
+ struct tasklet_struct data_task;
+
+ struct platform_device *pdev;
+ struct au1xmmc_platdata *platdata;
+ int irq;
+};
Hm, do you need the above structure to be visible from the platform code?

WBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/