[PATCH 4/7] mmc: consolidate sdhci_pltfm_data and sdhci_of_data into one

[Shawn Guo]

On Tue, Mar 15, 2011 at 01:55:13PM -0600, Grant Likely wrote:
> On Mon, Mar 14, 2011 at 10:25:56PM +0800, Shawn Guo wrote:
> > This patch is motivated by the work of supporting sdhci-esdhc-imx as
> > an OF device.  The sdhci-esdhc-imx driver was well designed to be
> > able to work with either platform or OF bus, with a little effort to
> > decouple the driver from sdhci_pltfm_data.
> > 
> > Like sdhci_ops works for both platform and OF sdhci driver, the patch
> > demonstrates that sdhci_pltfm_data and sdhci_of_data can be
> > consolidated into sdhci_data, which should work for both.  As one of
> > the results, header linux/mmc/sdhci-pltfm.h can be deleted.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo at linaro.org>
> 
> I like the push to unify DT and non-DT usage with the SDHCI drivers,
> but I'm not convinced on the approach.  Actually, that's more a
> comment on the existing code than it is on the this patch.
> 
Yes, this patch is not supposed to mean that much.  It only intends
to unify one data structure so that sdhci-esdhc-imx.c can work for
both cases.

The topic you raised here is a bigger one which may involve debate
with authors of both sdhci-pltfm.c and sdhci-of-core.c.  We can take
it as the final goal, and this patch could be a first step to that
goal.

-- 
Regards,
Shawn

> I don't like the way that sdhci-pltfm.c and sdhci-of-core.c each take
> responsibility for the .probe hook on each of the implementations.
> Doing it that way means that each implementation needs to add a set of
> hooks into those core files protected by #ifdefs based on whether or
> not the driver is enabled.  It also means that loading one driver
> means that all the configured sdhci drivers get loaded into the
> kernel.  This seems backwards.
> 
> From what I can see, sdhci-pltfm.c and sdhci-of-core.c both provide
> useful common functions that would be used by all sdhci host drivers.
> The interface would be a lot cleaner if those routines were exported
> for use by other modules, and each driver registered its own probe
> hook.  It would keep all the driver specific stuff out of
> sdhci_pltfm_ids and I think it would result in a cleaner interface
> overall.
> 
> Here is how I would do it:
> 
> 1) break the bulk of the sdhci_pltfm.c and sdhci_of_core.c .probe and
> .remove hooks out into separate functions; callable by other modules
> 2) for each driver, add its own probe and remove hooks which calls
> into the new functions created in step 1.  This would eliminate the
> need for the ->exit and ->init callbacks since they would get rolled
> into the new .probe and .remove callbacks
> 3) finally, when all drivers are removed; eliminate the driver
> registration from sdhci_pltfm.c and sdhci_of_core.c because there
> wouldn't be any users left.
> 
> drivers/mmc/host/sdhci-pxa.c appears to be a good example of how I
> think sdhci platform_drivers should be structured.
> 
> At the same time, the functionality from sdhci_of_core.c could easily
> be migrated into sdhci_pltfm.c.
>