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

["Sergei Shtylyov" <sshtylyov@xxxxxxxxxxxxx>]

----- Original Message ----- 
From: "Manuel Lauss" <mano@xxxxxxxxxxxxxxxxxxxxxxx>
To: "Sergei Shtylyov" <sshtylyov@xxxxxxxxxxxxx>
Cc: <linux-mips@xxxxxxxxxxxxxx>; <linux-kernel@xxxxxxxxxxxxxxx>
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