Date:	Thu, 31 Jan 2013 11:23:03 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	"Manjunathappa, Prakash" <prakash.pm@...com>
Cc:	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"mporter@...com" <mporter@...com>,
	"davinci-linux-open-source@...ux.davincidsp.com" 
	<davinci-linux-open-source@...ux.davincidsp.com>,
	"cjb@...top.org" <cjb@...top.org>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"nsekhar@...com" <nsekhar@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	"hs@...x.de" <hs@...x.de>, "ido@...ery.com" <ido@...ery.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/3] mmc: davinci_mmc: add DT support

Hello,

I have a few comments on the devicetree binding and the way it's parsed.

On Thu, Jan 31, 2013 at 10:33:06AM +0000, Manjunathappa, Prakash wrote:
> Adds device tree support for davinci_mmc. Also add
> binding documentation.
> Tested with non-dma PIO mode and without GPIO
> card_detect/write_protect option because of
> dependencies on EDMA and GPIO modules DT support.
> 
> Signed-off-by: Manjunathappa, Prakash <prakash.pm@...com>
> Cc: linux-mmc@...r.kernel.org
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: linux-kernel@...r.kernel.org
> Cc: davinci-linux-open-source@...ux.davincidsp.com
> Cc: devicetree-discuss@...ts.ozlabs.org
> Cc: cjb@...top.org
> Cc: Sekhar Nori <nsekhar@...com>
> Cc: mporter@...com
> ---
>  .../devicetree/bindings/mmc/davinci_mmc.txt        |   23 +++++++
>  drivers/mmc/host/davinci_mmc.c                     |   69 +++++++++++++++++++-
>  2 files changed, 91 insertions(+), 1 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mmc/davinci_mmc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mmc/davinci_mmc.txt b/Documentation/devicetree/bindings/mmc/davinci_mmc.txt
> new file mode 100644
> index 0000000..144bee6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/davinci_mmc.txt
> @@ -0,0 +1,23 @@
> +* TI Highspeed MMC host controller for DaVinci
> +
> +The Highspeed MMC Host Controller on TI DaVinci family
> +provides an interface for MMC, SD and SDIO types of memory cards.
> +
> +This file documents differences between the core properties described
> +by mmc.txt and the properties used by the davinci_mmc driver.
> +
> +Required properties:
> +- compatible:
> + Should be "ti,davinci_mmc", for davinci controllers

"ti,davinci-mmc" (with '-' rather than '_') would be preferable.

> +
> +Optional properties:
> +- bus-width: Number of data lines, can be <1>, <4>, or <8>, default <1>
> +- max-frequency: maximum operating clock frequency.

You said you only described differences from mmc.txt, but this is listed in
mmc.txt.

> +- caps: Check for Host capabilities in <linux/mmc/host.h>

Is this a set of binary flags? Also, is this Linux-specific?

I really don't think this should be in the devicetree binding. Why do you need
this?

> +- version: version of controller.

This should be encoded as part of the compatible string.

> +Example:
> +	mmc1: mmc@...809c000 {
> +		compatible = "ti,omap4-hsmmc";
> +		reg = <0x4809c000 0x400>;
> +		bus-width = <4>;
> +	};

It would be nice if the example referred to this binding rather than another.

> diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
> index 382b79d..ce6730d 100644
> --- a/drivers/mmc/host/davinci_mmc.c
> +++ b/drivers/mmc/host/davinci_mmc.c
> @@ -34,6 +34,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/edma.h>
>  #include <linux/mmc/mmc.h>
> +#include <linux/of.h>
>  
>  #include <linux/platform_data/mmc-davinci.h>
>  
> @@ -1156,16 +1157,75 @@ static void __init init_mmcsd_host(struct mmc_davinci_host *host)
>  
>  	mmc_davinci_reset_ctrl(host, 0);
>  }
> +#ifdef CONFIG_OF
> +static struct davinci_mmc_config
> +	*mmc_of_get_pdata(struct platform_device *pdev)
> +{
> +	struct device_node *np;
> +	struct davinci_mmc_config *pdata = NULL;
> +	u32 data;
> +	int ret;
> +
> +	pdata = pdev->dev.platform_data;
> +	if (!pdata) {
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata) {
> +			dev_err(&pdev->dev, "Failed to allocate memory for struct davinci_mmc_config\n");
> +			goto nodata;
> +		}
> +	}

Why do you need to conditionally allocate this? This only seems to be called
once.

> +
> +	np = pdev->dev.of_node;
> +	if (!np)
> +		goto nodata;

Why not just return immediately here? You do nothing special at nodata.

> +
> +	ret = of_property_read_u32(np, "bus-width", &data);
> +	if (ret)
> +		dev_info(&pdev->dev, "wires not specified, defaulting to 4 bit mode\n");
> +	pdata->wires = data;

That dev_info doesn't seem to match up with what the next line is doing (data
might not have been initialised). Also, unless I've misunderstood, it doesn't
match up with the default of 1 mentioned in the binding doc.

> +
> +	ret = of_property_read_u32(np, "max-frequency", &data);
> +	if (ret)
> +		dev_info(&pdev->dev, "max-frequency not specified, defaulting to 25MHz\n");
> +	pdata->max_freq = data;

Again, the dev_info doesn't match up with the line below it. I notice
the default's not one specified in the binding. Is this frequency chosen
from the hardware docs, or is it an arbitrary choice. If not arbitrary,
it might be worth specifying in the binding.


> +
> +	ret = of_property_read_u32(np, "caps", &data);
> +	if (ret)
> +		dev_info(&pdev->dev, "card capability not specified\n");
> +	pdata->caps = data;

Again, you may be using garbage data.

> +
> +	ret = of_property_read_u32(np, "version", &data);
> +	if (ret)
> +		dev_err(&pdev->dev, "version not specified\n");
> +	pdata->version = data;

And again, though I'd prefer to see this property go entirely.

> +
> +nodata:
> +	return pdata;
> +}
> +
> +#else
> +static struct davinci_mmc_config
> +	*mmc_of_get_pdata(struct platform_device *pdev)
> +{
> +	return pdev->dev.platform_data;
> +}
> +#endif

This is poorly named if it's required for !CONFIG_OF.

Why not change this to something like mmc_parse_pdata, returning an
error code. For !CONFIG_OF, it can simply return 0, which should be less
surprising for anyone else reading this code.

>  
>  static int __init davinci_mmcsd_probe(struct platform_device *pdev)
>  {
> -	struct davinci_mmc_config *pdata = pdev->dev.platform_data;
> +	struct davinci_mmc_config *pdata = NULL;
>  	struct mmc_davinci_host *host = NULL;
>  	struct mmc_host *mmc = NULL;
>  	struct resource *r, *mem = NULL;
>  	int ret = 0, irq = 0;
>  	size_t mem_size;
>  
> +	pdata = mmc_of_get_pdata(pdev);
> +	if (pdata == NULL) {
> +		dev_err(&pdev->dev, "Can not get platform data\n");
> +		return -ENOENT;
> +	}
> +
>  	/* REVISIT:  when we're fully converted, fail if pdata is NULL */
>  
>  	ret = -ENODEV;
> @@ -1403,11 +1463,18 @@ static const struct dev_pm_ops davinci_mmcsd_pm = {
>  #define davinci_mmcsd_pm_ops NULL
>  #endif
>  
> +static const struct of_device_id davinci_mmc_of_match[] = {
> +	{.compatible = "ti,davinci_mmc", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, davinci_mmc_of_match);

For supporting multiple versions, you could either use some auxdata
here, or check each one in davince_mmcsd_probe.

> +
>  static struct platform_driver davinci_mmcsd_driver = {
>  	.driver		= {
>  		.name	= "davinci_mmc",
>  		.owner	= THIS_MODULE,
>  		.pm	= davinci_mmcsd_pm_ops,
> +		.of_match_table = of_match_ptr(davinci_mmc_of_match),
>  	},
>  	.remove		= __exit_p(davinci_mmcsd_remove),
>  };

Where does davinci_mmcsd_probe get called from, and how is the
of_match_table used? Should it not be set as .probe on the driver?

Thanks,
Mark.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists