Hi Grant,

On Fri, Jun 03, 2011 at 02:03:16PM -0600, Grant Likely wrote:

On Thu, Jun 02, 2011 at 10:57:50AM +0800, Shawn Guo wrote:

quoted

From: Shawn Guo <redacted>

There are a couple of problems left from the sdhci pltfm and OF
consolidation changes.

* When building more than one sdhci-pltfm based drivers in the same
  image, linker will give multiple definition error on the sdhci-pltfm
  helper functions.  For example right now, building sdhci-of-esdhc
  and sdhci-of-hlwd together is a valid combination from Kconfig view.

* With the current build method, there is error with building the
  drivers as module, but module installation fails with modprobe.

The patch fixes above problems by changing sdhci-pltfm into a module.
To avoid EXPORT_SYMBOL on so many big endian IO accessors, it moves
these accessors into sdhci-pltfm.h as the 'static inline' functions.
As a result, sdhci.h needs to be included in sdhci-pltfm.h, and in
turn can be removed from individual drivers which already include
sdhci-pltfm.h.

Mostly looks good.  One comment below about a static inline, but
otherwise you can add my:

Acked-by: Grant Likely <redacted>

quoted

+static inline void sdhci_be32bs_writew(struct sdhci_host *host,
+				       u16 val, int reg)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	int base = reg & ~0x3;
+	int shift = (reg & 0x2) * 8;
+
+	switch (reg) {
+	case SDHCI_TRANSFER_MODE:
+		/*
+		 * Postpone this write, we must do it together with a
+		 * command write that is down below.
+		 */
+		pltfm_host->xfer_mode_shadow = val;
+		return;
+	case SDHCI_COMMAND:
+		sdhci_be32bs_writel(host,
+				    val << 16 | pltfm_host->xfer_mode_shadow,
+				    SDHCI_TRANSFER_MODE);
+		return;
+	}
+	clrsetbits_be32(host->ioaddr + base, 0xffff << shift, val << shift);
+}

This is really too big to be a static inline.  Go ahead and make it an
exported symbol.  Personally, I wouldn't worry about it and just exporting all
of these accessors.  Making them inline doesn't buy much anyway since
they are used to initialize an ops table, and making them inline
forces each driver to instantiate its own copy.

Thanks for the comment.  Since Chris had picked the patch up, I would
not send another update for this, unless Chris asks me to do.  I
actually like to see all these accessors implemented in the same way
and in the same place.

-- 
Regards,
Shawn