Re: [PATCH 4/7] Alchemy: db1200/pb1200: register mmc platform deviceand board specific functions

From: Sergei Shtylyov
Date: Thu May 08 2008 - 15:31:42 EST

Manuel Lauss wrote:

diff --git a/arch/mips/au1000/common/platform.c b/arch/mips/au1000/common/platform.c
index 31d2a22..08a5900 100644
--- a/arch/mips/au1000/common/platform.c
+++ b/arch/mips/au1000/common/platform.c
-
-static struct platform_device au1xxx_mmc_device = {
- .name = "au1xxx-mmc",
- .id = 0,
- .dev = {
- .dma_mask = &au1xxx_mmc_dmamask,
- .coherent_dma_mask = 0xffffffff,
- },
- .num_resources = ARRAY_SIZE(au1xxx_mmc_resources),
- .resource = au1xxx_mmc_resources,
-};
#endif /* #ifdef CONFIG_SOC_AU1200 */

What board-specific was here?

Nothing in here per se, but a) I don't like this file, it registers
stuff some boards don't need/want, b) this part is only interesting
for pb1200 board anyway.

Sigh. Do you know that Au1100 also has the same MMC controllers? The platform device is not registered in this case though and the driver (however small it now actually uses the platform device per se) is therefore unable to control it (well, I'm not sure it can do that since it seems to be Au1200 centered now (using DBDMA), however it was initially written for Au1100 as it seems.

I moved it to the pb1200 platform.c because
of the function pointers for au1xmmc platdata.

I'm sure that can be handled without moving the device itself...

diff --git a/arch/mips/au1000/pb1200/platform.c b/arch/mips/au1000/pb1200/platform.c
index 5930110..bee2bf7 100644
--- a/arch/mips/au1000/pb1200/platform.c
+++ b/arch/mips/au1000/pb1200/platform.c
@@ -20,8 +20,17 @@
#include <linux/init.h>
#include <linux/platform_device.h>
+#include <linux/mmc/host.h>
#include <asm/mach-au1x00/au1xxx.h>
+#include <asm/mach-au1x00/au1xxx_dbdma.h>
+#include <asm/mach-au1x00/au1100_mmc.h>
+
+#if defined(CONFIG_MIPS_PB1200)
+#include <asm/mach-pb1x00/pb1200.h>
+#elif defined(CONFIG_MIPS_DB1200)
+#include <asm/mach-db1x00/db1200.h>
+#endif

#include <asm/mach-au1x00/au1xxx.h> does all that already -- you don't need to include the board specific headers one more time. Drop this part please.

Done. I originally put it in because of compile errors wrt. BCSR register.

Hm, why would there be any if au1xxx.h resolves the right board #include?

+ return (bcsr->sig_status & au1xmmc_card_table[host->id].bcsrstatus)
+ ? 1 : 0;
+}
+
+static struct au1xmmc_platdata db1xmmcpd = {
+ .set_power = pb1200mmc_set_power,
+ .card_inserted = pb1200mmc_card_inserted,
+ .card_readonly = pb1200mmc_card_readonly,
+ .cd_setup = NULL, /* use poll-timer in driver */

Function ptrs in the platform data? That's something new -- though why not? :-)

Is this an accepted way of doing things in the kernel? If not, I'm open to
suggestions!

I really don't know -- never seen such trick before.

(I prefer this to globally-visible methods called by the
driver. I like it when related things are neatly grouped together).

Yes, this indeed looks better.

+};
+
+static u64 au1xxx_mmc_dmamask = ~(u32)0;
+
+struct resource au1200sd0_res[] = {
+ [0] = {
+ .start = CPHYSADDR(SD0_BASE),

Why not just use SD0_PHYS_ADDR?

+ .end = CPHYSADDR(SD0_BASE) + 0x40,

You've missed "- 1" here. Though it's alos not clear why 0x40 and not 0x3c (there's not register at 0x3c) or 0x80000 -- the range used according to the memory map...

I'll add the whole 0x80000 range ;-)

I've just posted a patch to fix this in the current code. Well, not that it matters much but I'm kind of pedantic. :-)

+ [3] = {
+ .start = DSCR_CMD0_SDMS_TX0,
+ .flags = IORESOURCE_DMA,
+ },
+ [4] = {
+ .start = DSCR_CMD0_SDMS_RX0,
+ .flags = IORESOURCE_DMA,
+ },
+};

Humm. The other devices (like IDE) should also claim DMA resources...

It was a convenient way to pass DDMA IDs without having to use a lookup table in the driver ;-)

Humm, the DBDMA code seems to mean by "DMA channel" something different, see au1xxx_dbdma_chan_alloc()...

+static struct platform_device au1200_sd0_device = {
+ .name = "au1xxx-mmc",
+ .id = 0, /* index into au1xmmc_card_table[] */
+ .dev = {
+ .dma_mask = &au1xxx_mmc_dmamask,
+ .coherent_dma_mask = 0xffffffff,
+ .platform_data = &db1xmmcpd,

Can't we leave the MMC platform device where it is but define the platform data structure per board with some starndard name? Since IMO it doesn't make sense to move the platform device itself.

I like my device setup data in one file (preferably living in the board
subdir), but for mainline inclusion I can move it back to its original
place if you and others prefer so.

I'd definitely prefer to leave the SOC device where they were...

+ },
+ .num_resources = ARRAY_SIZE(au1200sd0_res),
+ .resource = au1200sd0_res,
+};
+
+#ifndef CONFIG_MIPS_DB1200

Wait, SD controller 1 is there regardless of the board, so should be registerred regardless. If however the board doesn't have the necessary resources to support the driver functionality, I think it can be indicated by the board-level platform data, so that the driver could decide whether it wants to support that controller or not.

Won't this cause problems if e.g. you are using PCMCIA (since SD1 pins are
muxed with pcmcia signals)?

Good point. I think that the code in arch/mips/au1000/common/platform.c should check the sys_pinfunc register, and not blindly register all devices.

In any case, I just followed the original code, which also disabled the 2nd
controller for the db1200.

Indeed...

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/