2014-06-26 19:03 GMT+08:00 Mark Rutland <mark.rutland@xxxxxxx>: > On Thu, Jun 26, 2014 at 07:23:30AM +0100, Vincent Yang wrote: >> This patch adds new host controller driver for >> Fujitsu SDHCI controller f_sdh30. >> >> Signed-off-by: Vincent Yang <Vincent.Yang@xxxxxxxxxxxxxx> >> --- >> .../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. > > 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" > >> +- 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. Yes, I'll update it in next version. > >> +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. Yes, I'll update it in next version. > >> +- 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>; Yes, I'll update it in next version. > > [...] > >> + 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. > >> + >> + 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; [...] > >> + >> + 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. Yes, I'll update them as below in next version. - priv->clk_sd4 = of_clk_get(pdev->dev.of_node, 0); + priv->clk_sd4 = clk_get(&pdev->dev, "sd_sd4clk"); - priv->clk_b = of_clk_get(pdev->dev.of_node, 1); + priv->clk_b = clk_get(&pdev->dev, "sd_bclk"); Thanks a lot for your review! Best regards, Vincent Yang > > Thanks, > Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html