Re: [PATCH 2/5] MMC: mmci: Seperate ARM variants from generic code

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 15 March 2012, Russell King - ARM Linux wrote:
> On Thu, Mar 15, 2012 at 06:23:32PM +0000, Arnd Bergmann wrote:
> > 
> > * we want to get rid of arch/arm/mach-ux500/board-mop500-sdi.c
> > * the sdi0_configure() and mop500_sdi0_vdd_handler() functions
> >   need to be moved somewhere in order to do that, so they should
> >   be with the driver.
> 
> I don't believe they should.  MMCI _is_ the peripheral itself.  All the
> stuff that's in board-mop500-sdi.c seems to be doing is configuration
> type stuff for the SoC that its been integrated into.  Are you seriously
> advocating having multiple drivers for essentially the same hardware?

It's basically the same we have for sdhci or ohci, or even the various
8250 versions. There is usually an identical core used in a number of
chips with slight variantions. In some cases it's better to describe
them as quirks of one driver, in other cases it's better abstract
them as separate driver files. The debate is just over which of these
two cases mmci fits into.

> > * if we want to have a separate probe function, we also need to
> >   have a separate amba_driver structure
> > * the variant_data in mmci.c belongs with the amba_id, so that
> >   also gets moved to the ux500 file.
> > 
> > If you have a better solution for one or more of these, I'm
> > all ears.
> 
> Are you really advocating having mmci_foo.c where foo is an ever
> increasing number of SoCs with the MMCI integrated because you want
> to integrate SoC specific details into the MMCI peripheral driver?

Only when they are significantly different from the common one.
According to your arguments above, the ux500 one might not be
different enough, but if something comes along that needs something
wildly different while still sharing a lot of the other code,
I would certainly use the same model.

> > The alternatives that I can see are:
> > a) keep using auxdata to supply a platform_data pointer and
> >    do everything in the main driver. Problem: we want to
> >    avoid auxdata if possible
> > b) move the code from patch 4 into mmci.c using #ifdef.
> >    Problem: it's ugly code that has nothing to do with mmci
> >    in general.
> > c) use the regulator framework to do the voltage selection
> >    here, and have only generic code in mmci.c. This may
> >    be the best solution, but I have no idea if this is
> >    actually possible, or how to do it.
> 
> Let's ignore the DMA stuff for the time being - that's a separate problem
> which needs to be addressed once we work out how to deal with how DMA
> engine stuff gets resolved with DT based systems.

Agreed.

> The remainder of it (mmci_platform_data) looks like it can be easily
> represented as DT properties, the exception to that is the ios_handler.
> I think the right solution for that is to move to the regulator framework,
> but allow the driver to continue having the ios callback if there is
> no attached regulator.  It needs that because of the four integrated
> regulator control bits (which are entirely undefined.)

Yes, that's what I was not sure about whether it's worth it. If we do this,
I absolutely agree that there is no need to split mmci.c.

> We _do_ need a solution for GPIO-controlled voltage regulators, as these
> are actually quite common - see the various soc-common using PCMCIA
> drivers.
> 
> So I think actually a bit more thought is required here, and there's a
> need to use some of the facilities such as regulator which are now present
> that weren't when the driver was originally written some 9 years ago.

Ok. Let's see what Mark has to say about this.

	Arnd