[PATCH v2 0/3] add CNS3xxx AHCI support

2011/1/7 Anton Vorontsov [off-list ref]:

On Thu, Jan 06, 2011 at 06:17:56PM -0500, Jeff Garzik wrote:

quoted

quoted

[...]

quoted

quoted

It is overkill to rename the entirety of ahci_platform just for one override
function.
This sort of thing I would have expected to be added directly to
ahci_platform.c.

It might be overkill for only one controller. but it is more clean and
readable to have different SoC specific changes in separate files,
especially when more SoCs need to make similar changes.

I think that renaming the file is not necessary. You can just
rename the module in the makefile.

Personally I like the current approach more than putting
controller-specific fixups directly into ahci_platform.

My main objection is the renaming. ?If ahci_platform wants to export
some symbols for SoC modules like ahci_platform_cns, that's ok.

In general, follow the library approach rather than linking modules
together in strange ways.

In ahci_platform case there's almost nothing to factor out to a
library. Each platform just needs its own small "fixups" that we
normally pass via platform data.

We would happily pass them via platform data, and initially Mac
did exactly this, see:

http://ns3.spinics.net/lists/linux-ide/msg39554.html

You may notice that all platform-specific quirks are in the
platform code, which is neat. But there is a problem: once
the platform code needs to use libata, then we have to
build libata into the kernel (no modules possible), i.e.

http://ns3.spinics.net/lists/linux-ide/msg39656.html

It's not something new, we have the same "problem" with SDHCI,
USB and plenty other drivers. But the solution is simple, for
example see drivers/mmc/host/sdhci-pltfm.c and sdhci-cns3xxx.c
(and sdhci-esdhc-imx.c as another example of platform-specific
hooks for sdhci-pltfm).

Of course, we could make full-fledged platform driver just for
CNS3xxx, but that's kind of overkill. IMO, it's better to make
ahci_platform flexible and suitable for generic use.

Thanks for explaining all this.

As for renaming, I agree that ahci_platform.c file does not
need to be renamed, I still think that we could just rename
the module (so far it is used only on CNS3xxx platforms, so
no big deal even if anybody rely on the module name, which
I doubt.)

Yes, you're right.
cns3xxx seems like the only platform that uses ahci_platform driver.
Keeping the same module name is not necessary.

We could rename the module ahci_platform to ahci_platforms(?), into
which link ahci_platform and SoC specific changes.
Is it acceptable?

Best Regards,
Mac Lin