[cc'ing linux-mmc@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@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:
- 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.