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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080521131053.GA8459@roarinelk.homelinux.net>
Date:	Wed, 21 May 2008 15:10:53 +0200
From:	Manuel Lauss <mano@...rinelk.homelinux.net>
To:	Sergei Shtylyov <sshtylyov@...mvista.com>
Cc:	linux-mips@...ux-mips.org, linux-kernel@...r.kernel.org,
	drzeus@...eus.cx
Subject: Re: [PATCH 4/9] Alchemy: register mmc platform device for
	db1200/pb1200 boards

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@...rinelk.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/