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

["Sergei Shtylyov" <sshtylyov@ru.mvista.com>]

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