Hi Sergei,
On Thu, May 08, 2008 at 04:41:32PM +0400, Sergei Shtylyov 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. I moved it to the pb1200 platform.c because
of the function pointers for au1xmmc platdata.
>> static struct platform_device au1x00_pcmcia_device = {
>> 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.
>> +static int pb1200mmc_card_readonly(void *mmc_host)
>> +{
>> + struct au1xmmc_host *host = mmc_priv((struct mmc_host *)mmc_host);
>
> Insert new line after a declaration please.
>
>> + return (bcsr->status & au1xmmc_card_table[host->id].wpstatus) ? 1 : 0;
>> +}
>> +
>> +static int pb1200mmc_card_inserted(void *mmc_host)
>> +{
>> + struct au1xmmc_host *host = mmc_priv((struct mmc_host *)mmc_host);
>
> The same here.
Done and done,
>> + 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 prefer this to globally-visible methods called by the
driver. I like it when related things are neatly grouped together).
>> +};
>> +
>> +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 ;-)
>> + [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 ;-)
>> +
>> +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.
>> + },
>> + .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)?
In any case, I just followed the original code, which also disabled the 2nd
controller for the db1200.
>> +struct resource au1200sd1_res[] = {
>> + [0] = {
>> + .start = CPHYSADDR(SD1_BASE),
>> + .end = CPHYSADDR(SD1_BASE) + 0xff,
>
> Why not 0x40 if the controllers are symmetric I wonder? I'd say it
> should be 0x80000 even (and don't forget to sutract one ;-)...
I corrected this (and wrong resource numbering) locally.
Thanks for looking at it!
Manuel Lauss
