[PATCH 2/7] mmc: sdhi: enchance OF support for flags and cd-gpios
From: Guennadi Liakhovetski <hidden>
Date: 2012-11-26 10:16:15
Also in:
linux-sh
Hi Simon Thanks for the patch, please, see some comments inline. On Mon, 26 Nov 2012, Simon Horman wrote:
Enhance the OF support for SDHI by adding the following bindings: * renesas,shmobile-sdhi-has-idle-wait: Equivalent to setting TMIO_MMC_HAS_IDLE_WAIT in the flags of platform data * renesas,shmobile-sdhi-use-gpio-cd: Equivalent to setting TMIO_MMC_USE_GPIO_CD in the flags of platform data * renesas,shmobile-sdhi-wrprotect-disable: Equivalent to setting TMIO_MMC_WRPROTECT_DISABLE in the flags of platform data * cd-gpios: Equivalent to setting cd_gpio in platform data This patch also requests the clock based on the device tree node name if the platform data is absent.
I'm not sure this is a good idea. IIUC, the idea is, that you either only
have one clock, then you don't use a name, or you use several clocks, then
your driver should know the names and what each of them is used for. See,
e.g. drivers/mtd/nand/gpmi-nand/gpmi-nand.c::gpmi_get_clks()
static char *extra_clks_for_mx6q[GPMI_CLK_MAX] = {
"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
};
static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
{
...
/* The main clock is stored in the first. */
r->clock[0] = clk_get(this->dev, "gpmi_io");
etc., and clock declaration in arch/arm/boot/dts/imx6q.dtsi:
gpmi-nand at 00112000 {
compatible = "fsl,imx6q-gpmi-nand";
...
clocks = <&clks 152>, <&clks 153>, <&clks 151>,
<&clks 150>, <&clks 149>;
clock-names = "gpmi_io", "gpmi_apb", "gpmi_bch",
"gpmi_bch_apb", "per1_bch";
quoted hunk ↗ jump to hunk
The motivation for this is to allow registration of the SDHI device of the KZM-A9-GT board through device tree. The following device tree snippet illustrates the use of the bindings described above. sdhi0 at 0xee100000 { compatible = "renesas,shmobile-sdhi"; reg = <0xee100000 0x100>; interrupt-parent = <&gic>; interrupts = <0 83 0x4 0 84 0x4 0 85 0x4>; interrupt-names = "card_detect", "sdcard", "sdio"; reg-io-width = <2>; vmmc-supply = <&fixedregulator2v8>; vqmmc-supply = <&fixedregulator2v8>; renesas,shmobile-sdhi-has-idle-wait; }; sdhi2 at 0xee140000 { compatible = "renesas,shmobile-sdhi"; reg = <0xee140000 0x100>; interrupt-parent = <&gic>; interrupts = <0 103 0x4 0 104 0x4 0 105 0x4>; interrupt-names = "card_detect", "sdcard", "sdio"; reg-io-width = <2>; vmmc-supply = <&fixedregulator2v8>; vqmmc-supply = <&fixedregulator2v8>; renesas,shmobile-sdhi-has-idle-wait; renesas,shmobile-sdhi-use-gpio-cd; renesas,shmobile-sdhi-wrprotect-disable; cd-gpios = <&gpio 13 1>; }; Cc: Guennadi Liakhovetski <redacted> Signed-off-by: Simon Horman <horms@verge.net.au> --- drivers/mmc/host/sh_mobile_sdhi.c | 76 +++++++++++++++++++++++++++++++------ 1 file changed, 65 insertions(+), 11 deletions(-)diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index 0bdc146..8c7e658 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c@@ -29,6 +29,7 @@ #include <linux/mfd/tmio.h> #include <linux/sh_dma.h> #include <linux/delay.h> +#include <linux/of_gpio.h> #include "tmio_mmc.h"@@ -117,13 +118,64 @@ static const struct sh_mobile_sdhi_ops sdhi_ops = { .cd_wakeup = sh_mobile_sdhi_cd_wakeup, }; +static int __devinit
__devinit is being removed: http://thread.gmane.org/gmane.linux.ports.arm.omap/89686 please, drop it from this patch.
+sh_mobile_sdhi_probe_clk(struct platform_device *pdev,
+ struct sh_mobile_sdhi *priv,
+ const char *clk_name)
+{Once you resolve the doubt about the clock name, maybe this function could be dropped and you could just call clk_get() directly? Preferably from sh_mobile_sdhi_probe() directly (also see below)?
+ priv->clk = clk_get(&pdev->dev, clk_name);
+ if (IS_ERR(priv->clk)) {
+ dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
+ return PTR_ERR(priv->clk);
+ }
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static int __devinit
+sh_mobile_sdhi_probe_config_dt(struct platform_device *pdev,
+ struct sh_mobile_sdhi *priv)
+{
+ struct tmio_mmc_data *mmc_data = &priv->mmc_data;
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+
+ BUG_ON(!np);Hm, does this mean, that if you define CONFIG_OF in your kernel and don't provide platform data the driver will just Oops?...
quoted hunk ↗ jump to hunk
+ + ret = sh_mobile_sdhi_probe_clk(pdev, priv, np->name); + if (ret) + return ret; + + ret = of_get_named_gpio(np, "cd-gpios", 0); + if (gpio_is_valid(ret)) + mmc_data->cd_gpio = ret; + + if (of_get_property(np, "renesas,shmobile-sdhi-has-idle-wait", NULL)) + mmc_data->flags |= TMIO_MMC_HAS_IDLE_WAIT; + if (of_get_property(np, "renesas,shmobile-sdhi-use-gpio-cd", NULL)) + mmc_data->flags |= TMIO_MMC_USE_GPIO_CD; + if (of_get_property(np, "renesas,shmobile-sdhi-wrprotect-disable", + NULL)) + mmc_data->flags |= TMIO_MMC_WRPROTECT_DISABLE; + + return 0; +} +#else +static int __devinit +sh_mobile_sdhi_probe_config_dt(struct platform_device *pdev, + struct sh_mobile_sdhi *priv) +{ + return 0; +} +#endif + static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) { struct sh_mobile_sdhi *priv; struct tmio_mmc_data *mmc_data; struct sh_mobile_sdhi_info *p = pdev->dev.platform_data; struct tmio_mmc_host *host; - char clk_name[8]; int irq, ret, i = 0; bool multiplexed_isr = true;@@ -144,21 +196,17 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) } } - snprintf(clk_name, sizeof(clk_name), "sdhi%d", pdev->id); - priv->clk = clk_get(&pdev->dev, clk_name); - if (IS_ERR(priv->clk)) { - dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name); - ret = PTR_ERR(priv->clk); - goto eclkget; - } - mmc_data->clk_enable = sh_mobile_sdhi_clk_enable; mmc_data->clk_disable = sh_mobile_sdhi_clk_disable; mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED; if (p) { + char clk_name[8]; + snprintf(clk_name, sizeof(clk_name), "sdhi%d", pdev->id); + ret = sh_mobile_sdhi_probe_clk(pdev, priv, clk_name); + if (ret) + goto eclkget; +
This breaks the no-DT and no-platform-data case. With this your patch the clock wouldn't be obtain then.
quoted hunk ↗ jump to hunk
mmc_data->flags = p->tmio_flags; - if (mmc_data->flags & TMIO_MMC_HAS_IDLE_WAIT) - mmc_data->write16_hook = sh_mobile_sdhi_write16_hook; mmc_data->ocr_mask = p->tmio_ocr_mask; mmc_data->capabilities |= p->tmio_caps; mmc_data->capabilities2 |= p->tmio_caps2;@@ -176,7 +224,13 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) priv->dma_priv.alignment_shift = 1; /* 2-byte alignment */ mmc_data->dma = &priv->dma_priv; } + } else {
Please, remember, that the driver should be able to work with no platform data, no DT and CONFIG_OF either defined or not.
+ ret = sh_mobile_sdhi_probe_config_dt(pdev, priv); + if (ret) + goto eclkget; } + if (mmc_data->flags & TMIO_MMC_HAS_IDLE_WAIT) + mmc_data->write16_hook = sh_mobile_sdhi_write16_hook; /* * All SDHI blocks support 2-byte and larger block sizes in 4-bit -- 1.7.10.4
Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/