linux-mips
[Top] [All Lists]

Re: [PATCH 4/9] Alchemy: register mmc platform device for db1200/pb1200

To: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Subject: Re: [PATCH 4/9] Alchemy: register mmc platform device for db1200/pb1200 boards
From: Manuel Lauss <mano@roarinelk.homelinux.net>
Date: Wed, 21 May 2008 15:10:53 +0200
Cc: linux-mips@linux-mips.org, linux-kernel@vger.kernel.org, drzeus@drzeus.cx
In-reply-to: <4834166C.40901@ru.mvista.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20080519080339.GA21985@roarinelk.homelinux.net> <20080519080605.GE21985@roarinelk.homelinux.net> <4834166C.40901@ru.mvista.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.16 (2007-06-09)
Hi Sergei,

> Manuel Lauss wrote:
>
>> Add au1xmmc platform data for PB1200/DB1200 boards, and wire up
>> the 2 SD controllers for them.
>
>> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
>
>    OK, this definitely looks better (and shorter :-).

Thanks ;-)


>> +static struct resource au1200_mmc0_resources[] = {
>> +    [0] = {
>> +            .start          = SD0_PHYS_ADDR,
>> +            .end            = SD0_PHYS_ADDR + 0x7ffff,
>> +            .flags          = IORESOURCE_MEM,
>> +    },
>> +    [1] = {
>> +            .start          = AU1200_SD_INT,
>> +            .flags          = IORESOURCE_IRQ,
>> +    },
>> +    [2] = {
>> +            .start          = DSCR_CMD0_SDMS_TX0,
>> +            .flags          = IORESOURCE_DMA,
>> +    },
>> +    [3] = {
>> +            .start          = DSCR_CMD0_SDMS_RX0,
>> +            .flags          = IORESOURCE_DMA,
>> +    }
>> +};
>> +
>> +static struct resource au1200_mmc1_resources[] = {
>> +    [0] = {
>> +            .start          = SD1_PHYS_ADDR,
>> +            .end            = SD1_PHYS_ADDR + 0x7ffff,
>> +            .flags          = IORESOURCE_MEM,
>> +    },
>> +    [1] = {
>> +            .start          = AU1200_SD_INT,
>> +            .flags          = IORESOURCE_IRQ,
>> +    },
>> +    [2] = {
>> +            .start          = DSCR_CMD0_SDMS_TX1,
>> +            .flags          = IORESOURCE_DMA,
>> +    },
>> +    [3] = {
>> +            .start          = DSCR_CMD0_SDMS_RX1,
>> +            .flags          = IORESOURCE_DMA,
>> +    }
>> +};
>> +
>
>    Shouldn't the IRQ/DMA resources also have their 'end' field set?

Not sure, but I'll add them too.


>> +static void pb1200mmc0_set_power(void *mmc_host, int state)
>> +{
>> +    if (state)
>> +            bcsr->board |= BCSR_BOARD_SD0PWR;
>> +    else
>> +            bcsr->board &= ~BCSR_BOARD_SD0PWR;
>> +
>> +    au_sync_delay(1);
>> +}
>> +
>> +static int pb1200mmc0_card_readonly(void *mmc_host)
>> +{
>> +    return (bcsr->status & BCSR_STATUS_SD0WP) ? 1 : 0;
>> +}
>> +
>> +static int pb1200mmc0_card_inserted(void *mmc_host)
>> +{
>> +    return (bcsr->sig_status & BCSR_INT_SD0INSERT) ? 1 : 0;
>> +}
>> +
>> +#ifndef CONFIG_MIPS_DB1200
>> +static void pb1200mmc1_set_power(void *mmc_host, int state)
>> +{
>> +    if (state)
>> +            bcsr->board |= BCSR_BOARD_SD1PWR;
>> +    else
>> +            bcsr->board &= ~BCSR_BOARD_SD1PWR;
>> +
>> +    au_sync_delay(1);
>> +}
>> +
>> +static int pb1200mmc1_card_readonly(void *mmc_host)
>> +{
>> +    return (bcsr->status & BCSR_STATUS_SD1WP) ? 1 : 0;
>> +}
>> +
>> +static int pb1200mmc1_card_inserted(void *mmc_host)
>> +{
>> +    return (bcsr->sig_status & BCSR_INT_SD1INSERT) ? 1 : 0;
>> +}
>> +#endif
>
>    These 2 separate versions could be converted into single one by using 
> the data arrays holding info BCSR bits -- something like the MMC driver has 
> currently.

Well, that's what my initial submission did, but it required access to the
mmc drivers host structure (moved from au1xmmc.h to au1100_mmc.h) to determine
which controller wants the attention, and you weren't fond of that.


>> +
>> +const struct au1xmmc_platform_data au1xmmc_platdata[2] = {
>> +    [0] = {
>> +            .set_power      = pb1200mmc0_set_power,
>> +            .card_inserted  = pb1200mmc0_card_inserted,
>> +            .card_readonly  = pb1200mmc0_card_readonly,
>> +            .cd_setup       = NULL,         /* use poll-timer in driver */
>> +    },
>> +#ifndef CONFIG_MIPS_DB1200
>> +    [1] = {
>> +            .set_power      = pb1200mmc1_set_power,
>> +            .card_inserted  = pb1200mmc1_card_inserted,
>> +            .card_readonly  = pb1200mmc1_card_readonly,
>> +            .cd_setup       = NULL,         /* use poll-timer in driver */
>> +    },
>> +#endif
>> +};
>
>    You don't have to explicitly set 'cd_setup' to NULL...

The comment explains what setting to NULL does, so I'd like to
keep it (for people implementing this for other boards and wondering what
to do with thse members...)


> PS: BTW, I've noticed that include/asm-mips/mach-db1x00/db1x00.h defines 
> macros mmc_card_insterted() and mmc_power_on() which no code seems to be 
> using (might have been intended for use by the MMC driver but why no such 
> macros in other board headers?). Care to remove those while you're working 
> on MMC?

Sure, I'll add another patch to the pile.


Thank you!
        Manuel Lauss

<Prev in Thread] Current Thread [Next in Thread>