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

[Grant Likely]

[cc'ing linux-mmc at vger.kernel.org]

On Thu, Mar 17, 2011 at 02:33:20PM +0800, Shawn Guo wrote:
> 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.

I doubt very much that the sdhci-of-core.c users/developers will have
a problem with this.  There is a strong trend toward unifying DT and
non-DT code, and Linux has a strong pattern of each driver handling
its own registration.

I actually don't have an objection to this patch specifically, but
rather I want to see the overall direction be to eliminate
sdhci-of-core.c and sdhci-pltfm.c entirely instead of using it more..

On another note, this patch changes a number of names, both of 
structures and variables.  Specifically:

{sdhci_pltfm_data,sdhci_of_data} ==> sdhci_data and
pdata ==> data

However, it would be easier to review if the pdata==>data change was
dropped (the name of the local variable doesn't matter that much), and
if sdhci_of_data was renamed to sdhci_pltfm_data.  Doing so would make
the diff much smaller without changing the sanity of the resulting
code.

g.

> 
> -- 
> 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.
> > 
>