2014-06-27 18:12 GMT+08:00 Mark Rutland [off-list ref]:

On Fri, Jun 27, 2014 at 04:32:21AM +0100, Vincent Yang wrote:

quoted

2014-06-26 19:03 GMT+08:00 Mark Rutland [off-list ref]:

quoted

On Thu, Jun 26, 2014 at 07:23:30AM +0100, Vincent Yang wrote:

quoted

This patch adds new host controller driver for
Fujitsu SDHCI controller f_sdh30.

Signed-off-by: Vincent Yang <redacted>
---
 .../devicetree/bindings/mmc/sdhci-fujitsu.txt      |  35 +++
 drivers/mmc/host/Kconfig                           |   7 +
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/sdhci_f_sdh30.c                   | 346 +++++++++++++++++++++
 drivers/mmc/host/sdhci_f_sdh30.h                   |  40 +++
 5 files changed, 429 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
 create mode 100644 drivers/mmc/host/sdhci_f_sdh30.c
 create mode 100644 drivers/mmc/host/sdhci_f_sdh30.h

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
new file mode 100644
index 0000000..40add438
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt

@@ -0,0 +1,35 @@
+* Fujitsu SDHCI controller
+
+This file documents differences between the core properties in mmc.txt
+and the properties used by the sdhci_f_sdh30 driver.
+
+Required properties:
+- compatible: "fujitsu,f_sdh30"

Please use '-' rather than '_' in compatible strings.

Hi Mark,
Yes, I'll update it to '-' in next version.

quoted

This seems to be the name of the driver. What is the precise name of the
IP block?

The name of the IP block is "F_SDH30".
That's why it uses "fujitsu,f_sdh30"

Hmm. I'd still be tempted to use "fujitsu,f-sdh30".

Hi Mark,
Sure, I'll update it to "fujitsu,f-sdh30" in next version.

quoted

quoted

[...]

quoted

+       if (!of_property_read_u32(pdev->dev.of_node, "vendor-hs200",
+                                 &priv->vendor_hs200))
+               dev_info(dev, "Applying vendor-hs200 setting\n");
+       else
+               priv->vendor_hs200 = 0;

This wasn't in the binding document, and a grep for "vendor-hs200" in a
v3.16-rc2 tree found me nothing.

Please document this.

Yes, it is a setting for a vendor specific register.
I'll update it in next version.

It would be nice to know exactly what this is. We usually shy clear of
placing register values in dt. I can wait until the next posting if
you're goin to document that.

I'm thinking about removing this register value in dt.
I'll update it in next version.

quoted

quoted

quoted

+       if (!of_property_read_u32(pdev->dev.of_node, "bus-width", &bus_width)) {
+               if (bus_width == 8) {
+                       dev_info(dev, "Applying 8 bit bus width\n");
+                       host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+               }
+       }

What if bus-width is not 8, or is not present?

In both cases, it will not touch host->mmc->caps at all. Then sdhci_add_host()
will handle it and set MMC_CAP_4_BIT_DATA as default:

[...]
/*
* A controller may support 8-bit width, but the board itself
* might not have the pins brought out.  Boards that support
* 8-bit width must set "mmc->caps |= MMC_CAP_8_BIT_DATA;" in
* their platform code before calling sdhci_add_host(), and we
* won't assume 8-bit width for hosts without that CAP.
*/
if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
mmc->caps |= MMC_CAP_4_BIT_DATA;

Ok, but does it make sense for a dts to have:

        bus-width = <1>;

If so, we should presumably do something.

If not, we should at least print a warning that the dtb doesn't make
sense.

I'll print a warning for invalid values in next version.
Thanks a lot for your review!


Best regards,
Vincent Yang

Cheers,
Mark.