[PATCH 0/7] au1xmmc updates

[PATCH 0/7] au1xmmc updates

[Manuel Lauss]

Hello,

The following patches update the au1xmmc driver with new
features and a few cleanups.

Testers (especially with db1200 and pb1200 boards) and comments
 welcome!

Thanks!
	Manuel Lauss

[PATCH 1/7] Alchemy: export get_au1x00_speed for modules

[Manuel Lauss]

[PATCH 2/7] Alchemy: dbdma: add api to delete custom ddma devices

[Manuel Lauss]

[PATCH 3/7] au1xmmc: remove db1x00 specific functions and platform device conversion

[Manuel Lauss]

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

[Manuel Lauss]

[PATCH 5/7] au1xmmc: 4 bit transfer mode

[Manuel Lauss]

[PATCH 6/7] au1xmmc: wire up SDIO interrupt

[Manuel Lauss]

[PATCH 7/7] au1xmmc: codingstyle tidying

[Manuel Lauss]

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

[Sergei Shtylyov]

Hello.

Manuel Lauss wrote:

> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>

> 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

    Hm, I don't see anything board-specific in
arch/mips/au1000/common/platform.c anymore -- after I removed IDE and SMC
91C111 from there (these were devices on the static bus).

> @@ -162,24 +162,6 @@ static struct resource au1xxx_usb_gdt_resources[] = {
>  	},
>  };
>  
> -static struct resource au1xxx_mmc_resources[] = {
> -	[0] = {
> -		.start          = SD0_PHYS_ADDR,
> -		.end            = SD0_PHYS_ADDR + 0x40,
> -		.flags          = IORESOURCE_MEM,
> -	},
> -	[1] = {
> -		.start		= SD1_PHYS_ADDR,
> -		.end 		= SD1_PHYS_ADDR + 0x40,

    Hm, I haven't noticed that these are off-by-one... :-/

> -		.flags		= IORESOURCE_MEM,
> -	},
> -	[2] = {
> -		.start          = AU1200_SD_INT,
> -		.end            = AU1200_SD_INT,
> -		.flags          = IORESOURCE_IRQ,
> -	}
> -};
> -
>  static u64 udc_dmamask = ~(u32)0;
>  
>  static struct platform_device au1xxx_usb_gdt_device = {
> @@ -245,19 +227,6 @@ static struct platform_device au1200_lcd_device = {
>  	.num_resources  = ARRAY_SIZE(au1200_lcd_resources),
>  	.resource       = au1200_lcd_resources,
>  };
> -
> -static u64 au1xxx_mmc_dmamask =  ~(u32)0;
> -
> -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?

>  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.

>  static struct resource ide_resources[] = {
>  	[0] = {
> @@ -70,9 +79,125 @@ static struct platform_device smc91c111_device = {
>  	.resource	= smc91c111_resources
>  };
>  
> +
> +static const struct {
> +	u16 bcsrpwr;
> +	u16 bcsrstatus;
> +	u16 wpstatus;
> +} au1xmmc_card_table[] = {
> +	{ BCSR_BOARD_SD0PWR, BCSR_INT_SD0INSERT, BCSR_STATUS_SD0WP },
> +#ifndef CONFIG_MIPS_DB1200
> +	{ BCSR_BOARD_SD1PWR, BCSR_INT_SD1INSERT, BCSR_STATUS_SD1WP }
> +#endif
> +};
> +
> +static void pb1200mmc_set_power(void *mmc_host, int state)
> +{
> +	struct au1xmmc_host *host = mmc_priv((struct mmc_host *)mmc_host);
> +	u32 val = au1xmmc_card_table[host->id].bcsrpwr;
> +
> +	bcsr->board &= ~val;
> +	if (state)
> +		bcsr->board |= val;
> +
> +	au_sync_delay(1);
> +}
> +
> +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.

> +	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? :-)

> +};
> +
> +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...

> +		.flags	= IORESOURCE_MEM,
> +	},
> +	[2] = {
> +		.start	= AU1200_SD_INT,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +	[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...

> +
> +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.

> +	},
> +	.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.

> +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 ;-)...

WBR, Sergei

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

[Manuel Lauss]

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

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

[Sergei Shtylyov]

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

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

[Manuel Lauss]

On Thu, May 08, 2008 at 11:30:24PM +0400, Sergei Shtylyov wrote:
> 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 gathered that from the driver source...

I assume the PIO paths in the driver are intended for the Au1100, correct?
Should not be too hard to force PIO paths when no DDMA IDs are passed
through the platform device's resources.
Someone should test it on Au1100 though.


>> 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...

Okay...


>>>> +	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.
>

Unless someone else speaks up against it, I'll leave it the way it is.


>>>> +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...

Okay.


>>>> +	},
>>>> +	.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.

Hm, sounds ugly, but I'll add something.

Thanks!
	Manuel Lauss

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

[Sergei Shtylyov]

----- Original Message ----- 
From: "Manuel Lauss" <mano@roarinelk.homelinux.net>
To: "Sergei Shtylyov" <sshtylyov@ru.mvista.com>
Cc: <linux-mips@linux-mips.org>; <linux-kernel@vger.kernel.org>
Sent: Friday, May 09, 2008 9:40 AM
Subject: Re: [PATCH 4/7] Alchemy: db1200/pb1200: register mmc platformdevice 
and board specific functions


> On Thu, May 08, 2008 at 11:30:24PM +0400, Sergei Shtylyov wrote:
>> 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 gathered that from the driver source...

> I assume the PIO paths in the driver are intended for the Au1100, correct?
> Should not be too hard to force PIO paths when no DDMA IDs are passed
> through the platform device's resources.


   Yes but it should've probably also supported DMA via Au1100 DMA 
controller (not DBDMA).

>>>>> + 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.

> Unless someone else speaks up against it, I'll leave it the way it is.

   I assume just parametrizing the board-level functions using the data 
about BCSR (like it was done in the driver before) just won't work?

>>>>> + },
>>>>> + .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.

> Hm, sounds ugly, but I'll add something.

   Why? I think it's smarter than register SoC devices in every instance of 
board setup code. This way, the board setup code has already programmed 
sys_pinfunc as it sees fit, and the common code accomodates to this need 
registering those devices that the coard code have selected.

> Thanks!
> Manuel Lauss

WBR, Sergei

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

[Sergei Shtylyov]

----- Original Message ----- 
From: "Manuel Lauss" <mano@roarinelk.homelinux.net>
To: "Sergei Shtylyov" <sshtylyov@ru.mvista.com>
Cc: <linux-mips@linux-mips.org>; <linux-kernel@vger.kernel.org>
Sent: Friday, May 09, 2008 9:40 AM
Subject: Re: [PATCH 4/7] Alchemy: db1200/pb1200: register mmc platformdevice 
and board specific functions


> On Thu, May 08, 2008 at 11:30:24PM +0400, Sergei Shtylyov wrote:
>> 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 gathered that from the driver source...

> I assume the PIO paths in the driver are intended for the Au1100, correct?
> Should not be too hard to force PIO paths when no DDMA IDs are passed
> through the platform device's resources.


   Yes but it should've probably also supported DMA via Au1100 DMA 
controller (not DBDMA).

>>>>> + 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.

> Unless someone else speaks up against it, I'll leave it the way it is.

   I assume just parametrizing the board-level functions using the data 
about BCSR (like it was done in the driver before) just won't work?

>>>>> + },
>>>>> + .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.

> Hm, sounds ugly, but I'll add something.

   Why? I think it's smarter than register SoC devices in every instance of 
board setup code. This way, the board setup code has already programmed 
sys_pinfunc as it sees fit, and the common code accomodates to this need 
registering those devices that the coard code have selected.

> Thanks!
> Manuel Lauss

WBR, Sergei