On Thu, 19 May 2011, zhangfei gao wrote:

> On Wed, May 18, 2011 at 4:38 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 17 May 2011, zhangfei gao wrote:
> >> >> The code handle several register difference are located at
> >> >> arch/arm/mach-mmp/mmp2.c is for mmp2
> >> >> arch/arm/mach-mmp/pxa910.c is for pxa910 serious,
> >> >> arch/arm/mach-mmp/mmp3.c is for mmp3 serious, considering there may
> >> >> still registers changing.
> >> >>
> >> >> The board difference are directly put in board.c
> >> >> For example ttc_dkb do not use wp pin, so get_ro is provided.
> >> >
> >> > embedding the code in these chip files stops code sharing.  For example,
> >> > mmp3 has same controller as mmp2.
> >>
> >> If register are totally same, they can share.
> >> If there is minor difference with chip upgrading, they can put in
> >> different file.
> >
> > When we talked about similar problems in drivers during the Linaro Developer
> > Summit, the broad consensus was to move code from arch/arm into individual
> > device drivers, and I think this should be done for this driver too.
> >
> > My recommendation here would be to have no callbacks in the platform
> > data at all, but instead have the code for all variants in the
> > sdhci-pxa driver itself. You can still have a structure to describe
> > the differences using function pointers, but it's better if that is
> > part of the driver itself. In the platform data, provide a way
> > to identify the variant of the controller (e.g. using an enum)
> > and point to the base addresses as required.
> >
> >        Arnd
> >
> Hi, Arnd
> 
> Thanks for your suggestion.
> 
> To make sdhci-pxa easy to maintain, we still prefer put platform
> specific difference under platform code.

That's fine to split out platform differences.  However please put those 
differences, maybe in different files, but alongside the driver itself 
in drivers/mmc/host/sdhci-pxa-pxa910.c, sdhci-pxa-mmp2.c, etc. We don't 
want to see driver specific stuff hidden in arch/arm/mach-pxa/ or the 
like.

> More and more workaround comes when more and more platform need to
> support, making the driver bigger and bigger and not easy to maintain.
> Nobody remember what's the workaround purpose several years later.

You really really should spend more time documenting those workarounds 
in the source directly in that case.  Anyone not familiar with the 
issues should be able to understand why a given workaround is there, 
whether it is today or in several years.  And this is why we want driver 
stuff go in the driver directory where more people specifically 
interested in MMC/SD are looking.


Nicolas