[PATCH 2/7] mmc: sdhi: enchance OF support for flags and cd-gpios

Guennadi Liakhovetski g.liakhovetski at gmx.de
Mon Nov 26 05:16:15 EST 2012


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";

> 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 <g.liakhovetski at gmx.de>
> Signed-off-by: Simon Horman <horms at 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?...

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

>  		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/



More information about the linux-arm-kernel mailing list