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

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

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

+- voltage-ranges : two cells are required, first cell specifies minimum
+  slot voltage (mV), second cell specifies maximum slot voltage (mV).
+  Several ranges could be specified.

Describe this as a list of pairs, it's confusing otherwise.

I'm not sure I follow what having multiple pairs implies.

+Optional properties:
+- gpios: This is one optional gpio for controlling a power mux which
+  switches between two power supplies. 3.3V is selected when gpio is high,
+  and 1.8V is selected when gpio is low. This voltage is used for signal
+  level.

Give this a more descriptive name, like power-mux-gpios. That will match
up with the style of cd-gpios and wp-gpios.

+- clocks: Must contain an entry for each entry in clock-names. It is a
+  list of phandles and clock-specifier pairs.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names: Should contain the following two entries:
+       "sd_sd4clk" - clock primarily used for tuning process
+       "sd_bclk"   - base clock for sdhci controller
+
+Example:
+
+       sdhci1: sdio@36600000 {
+               compatible = "fujitsu,f_sdh30";
+               reg = <0 0x36600000 0x1000>;
+               interrupts = <0 172 0x4>,
+                            <0 173 0x4>;
+               voltage-ranges = <1800 1800 3300 3300>;

Place brackets around each pair to make this clearer:

	voltage-ranges = <1800 1800>, <3300 3300>;

[...]

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

+
+       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?

+
+       ret = mmc_of_parse_voltage(pdev->dev.of_node, &host->ocr_mask);
+       if (ret) {
+               dev_err(dev, "%s: parse voltage error\n", __func__);
+               goto err_voltage;
+       }
+
+       host->hw_name = DRIVER_NAME;
+       host->ops = &sdhci_f_sdh30_ops;
+       host->irq = irq;
+
+       host->ioaddr = of_iomap(pdev->dev.of_node, 0);
+       if (!host->ioaddr) {
+               dev_err(dev, "%s: failed to remap registers\n", __func__);
+               ret = -ENXIO;
+               goto err_remap;
+       }
+
+       priv->clk_sd4 = of_clk_get(pdev->dev.of_node, 0);
+       if (!IS_ERR(priv->clk_sd4)) {
+               ret = clk_prepare_enable(priv->clk_sd4);
+               if (ret < 0) {
+                       dev_err(dev, "Failed to enable sd4 clock: %d\n", ret);
+                       goto err_clk1;
+               }
+       }
+       priv->clk_b = of_clk_get(pdev->dev.of_node, 1);
+       if (!IS_ERR(priv->clk_b)) {
+               ret = clk_prepare_enable(priv->clk_b);
+               if (ret < 0) {
+                       dev_err(dev, "Failed to enable clk_b clock: %d\n", ret);
+                       goto err_clk2;
+               }
+       }

Given you gave these names, get these by name rather than index. It's
less surprising and more flexible.

Thanks,
Mark.