[RFC PATCH] SDHCI: S3C: Add support for retrieving memory and irq resource information from device tree.

[RFC PATCH] SDHCI: S3C: Add support for retrieving memory and irq resource information from device tree.

[thomas.abraham-QSEj5FYQhm4dnm+yROfE0A]

From: Thomas Abraham <thomas.abraham-68IGFXMjmZ7QT0dZR+AlfA@public.gmane.org>

Add support for retrieving memory and irq resource information
from device tree for Samsung's SDHCI controller driver.

Signed-off-by: Thomas Abraham <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---

The modification will be made more generic to support both
DT and non-DT versions of the driver without the #ifdef's.
For now, this patch is for review and to understand if the
approach adopted to obtain resource information from the
device tree is appropriate.

 drivers/mmc/host/sdhci-s3c.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
index 1720358..f536061 100644
--- a/drivers/mmc/host/sdhci-s3c.c
+++ b/drivers/mmc/host/sdhci-s3c.c
@@ -19,6 +19,9 @@
 #include <linux/clk.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
 
 #include <linux/mmc/host.h>
 
@@ -348,23 +351,52 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
 	struct sdhci_s3c *sc;
 	struct resource *res;
 	int ret, irq, ptr, clks;
+	struct device_node *np = NULL;
+#ifdef CONFIG_OF
+	struct resource iores;
+#endif
 
 	if (!pdata) {
 		dev_err(dev, "no device data specified\n");
 		return -ENOENT;
 	}
 
+#ifdef CONFIG_OF
+	for_each_compatible_node(np, NULL, "samsung,sdhci-s3c") {
+		const u32 *id = of_get_property(np, "cell-index", NULL);
+		if (be32_to_cpu(*id) == pdev->id)
+			break;
+	}
+
+	if (!np) {
+		dev_err(dev, "no matching device node specified in device tree\n");
+		return -ENOENT;
+	}
+#endif
+
+#ifndef CONFIG_OF
 	irq = platform_get_irq(pdev, 0);
+#else
+	irq = of_irq_to_resource(np, 0, NULL);
+#endif
 	if (irq < 0) {
 		dev_err(dev, "no irq specified\n");
 		return irq;
 	}
 
+#ifndef CONFIG_OF
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "no memory specified\n");
 		return -ENOENT;
 	}
+#else
+	if (of_address_to_resource(np, 0, &iores)) {
+		dev_err(dev, "no memory specified in device tree\n");
+		return -ENOENT;
+	}
+	res = &iores;
+#endif
 
 	host = sdhci_alloc_host(dev, sizeof(struct sdhci_s3c));
 	if (IS_ERR(host)) {
-- 
1.6.6.rc2

Re: [RFC PATCH] SDHCI: S3C: Add support for retrieving memory and irq resource information from device tree.

[Grant Likely]

On Mon, Jan 31, 2011 at 9:28 AM,  <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> From: Thomas Abraham <thomas.abraham-68IGFXMjmZ7QT0dZR+AlfA@public.gmane.org>
>
> Add support for retrieving memory and irq resource information
> from device tree for Samsung's SDHCI controller driver.
>
> Signed-off-by: Thomas Abraham <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>
> The modification will be made more generic to support both
> DT and non-DT versions of the driver without the #ifdef's.
> For now, this patch is for review and to understand if the
> approach adopted to obtain resource information from the
> device tree is appropriate.
>
>  drivers/mmc/host/sdhci-s3c.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 1720358..f536061 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -19,6 +19,9 @@
>  #include <linux/clk.h>
>  #include <linux/io.h>
>  #include <linux/gpio.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
>
>  #include <linux/mmc/host.h>
>
> @@ -348,23 +351,52 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>        struct sdhci_s3c *sc;
>        struct resource *res;
>        int ret, irq, ptr, clks;
> +       struct device_node *np = NULL;
> +#ifdef CONFIG_OF
> +       struct resource iores;
> +#endif
>
>        if (!pdata) {
>                dev_err(dev, "no device data specified\n");
>                return -ENOENT;
>        }
>
> +#ifdef CONFIG_OF
> +       for_each_compatible_node(np, NULL, "samsung,sdhci-s3c") {
> +               const u32 *id = of_get_property(np, "cell-index", NULL);
> +               if (be32_to_cpu(*id) == pdev->id)
> +                       break;
> +       }
> +
> +       if (!np) {
> +               dev_err(dev, "no matching device node specified in device tree\n");
> +               return -ENOENT;
> +       }

I think I've got a better solution to this which I'll be posting
today.  I've got code that allows the dt support code to 'snoop'
platform bus registrations and set the of_node pointer for matching
nodes.  Then all the normal platform_bus support will work just fine.
Doing it this way prevents building a kernel that supports both dt and
non-dt booting.

Also, relying on cell-index is generally considered a bad idea.  When
using the dt, let the kernel do the device enumeration instead of
specifying it explicitly with cell-index.

> +#endif
> +
> +#ifndef CONFIG_OF
>        irq = platform_get_irq(pdev, 0);
> +#else
> +       irq = of_irq_to_resource(np, 0, NULL);
> +#endif

When using the dt, platform_get_irq() should still work correctly.
This shouldn't be necessary.

>        if (irq < 0) {
>                dev_err(dev, "no irq specified\n");
>                return irq;
>        }
>
> +#ifndef CONFIG_OF
>        res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>        if (!res) {
>                dev_err(dev, "no memory specified\n");
>                return -ENOENT;
>        }
> +#else
> +       if (of_address_to_resource(np, 0, &iores)) {
> +               dev_err(dev, "no memory specified in device tree\n");
> +               return -ENOENT;
> +       }
> +       res = &iores;
> +#endif

Ditto

>
>        host = sdhci_alloc_host(dev, sizeof(struct sdhci_s3c));
>        if (IS_ERR(host)) {
> --
> 1.6.6.rc2
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [RFC PATCH] SDHCI: S3C: Add support for retrieving memory and irq resource information from device tree.

[Rob Herring]

Thomas,

On 01/31/2011 10:28 AM, thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
> From: Thomas Abraham<thomas.abraham-68IGFXMjmZ7QT0dZR+AlfA@public.gmane.org>
>
> Add support for retrieving memory and irq resource information
> from device tree for Samsung's SDHCI controller driver.
>
> Signed-off-by: Thomas Abraham<thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>
> The modification will be made more generic to support both
> DT and non-DT versions of the driver without the #ifdef's.
> For now, this patch is for review and to understand if the
> approach adopted to obtain resource information from the
> device tree is appropriate.
>

There is already an OF SDHCI driver. Some fixes from me for ARM have 
gone into .38.

>   drivers/mmc/host/sdhci-s3c.c |   32 ++++++++++++++++++++++++++++++++
>   1 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-s3c.c b/drivers/mmc/host/sdhci-s3c.c
> index 1720358..f536061 100644
> --- a/drivers/mmc/host/sdhci-s3c.c
> +++ b/drivers/mmc/host/sdhci-s3c.c
> @@ -19,6 +19,9 @@
>   #include<linux/clk.h>
>   #include<linux/io.h>
>   #include<linux/gpio.h>
> +#include<linux/of.h>
> +#include<linux/of_irq.h>
> +#include<linux/of_address.h>
>
>   #include<linux/mmc/host.h>
>
> @@ -348,23 +351,52 @@ static int __devinit sdhci_s3c_probe(struct platform_device *pdev)
>   	struct sdhci_s3c *sc;
>   	struct resource *res;
>   	int ret, irq, ptr, clks;
> +	struct device_node *np = NULL;
> +#ifdef CONFIG_OF
> +	struct resource iores;
> +#endif
>
>   	if (!pdata) {
>   		dev_err(dev, "no device data specified\n");
>   		return -ENOENT;
>   	}
>
> +#ifdef CONFIG_OF
> +	for_each_compatible_node(np, NULL, "samsung,sdhci-s3c") {
> +		const u32 *id = of_get_property(np, "cell-index", NULL);

Per Grant, using cell-index should be avoided.

> +		if (be32_to_cpu(*id) == pdev->id)

Any drivers that depend on pdev->id being an index will break for device 
tree.

> +			break;
> +	}
> +
> +	if (!np) {
> +		dev_err(dev, "no matching device node specified in device tree\n");
> +		return -ENOENT;
> +	}
> +#endif
> +
> +#ifndef CONFIG_OF
>   	irq = platform_get_irq(pdev, 0);

Using platform_get_irq works for OF drivers too.

> +#else
> +	irq = of_irq_to_resource(np, 0, NULL);
> +#endif
>   	if (irq<  0) {
>   		dev_err(dev, "no irq specified\n");
>   		return irq;
>   	}
>
> +#ifndef CONFIG_OF
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Ditto

>   	if (!res) {
>   		dev_err(dev, "no memory specified\n");
>   		return -ENOENT;
>   	}
> +#else
> +	if (of_address_to_resource(np, 0,&iores)) {
> +		dev_err(dev, "no memory specified in device tree\n");
> +		return -ENOENT;
> +	}
> +	res =&iores;
> +#endif
>
>   	host = sdhci_alloc_host(dev, sizeof(struct sdhci_s3c));
>   	if (IS_ERR(host)) {

You are missing an of_match_table. This patch could not work without it.

The primary things you need to do for OF support on a driver are add the 
match table and convert platform data to OF bindings.

Rob

Re: [RFC PATCH] SDHCI: S3C: Add support for retrieving memory and irq resource information from device tree.

[Grant Likely]

On Mon, Jan 31, 2011 at 10:00 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Thomas,
>
> On 01/31/2011 10:28 AM, thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
>>
>> From: Thomas Abraham<thomas.abraham-68IGFXMjmZ7QT0dZR+AlfA@public.gmane.org>
>>
>> Add support for retrieving memory and irq resource information
>> from device tree for Samsung's SDHCI controller driver.
>>
>> Signed-off-by: Thomas Abraham<thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>
>> The modification will be made more generic to support both
>> DT and non-DT versions of the driver without the #ifdef's.
>> For now, this patch is for review and to understand if the
>> approach adopted to obtain resource information from the
>> device tree is appropriate.
>>

> You are missing an of_match_table. This patch could not work without it.

Actually, the way he is implementing it the of_match_table would have
absolutely no effect.  However, you are right, it *should* be
implemented so that it binds via the of_match_table.  I suspect Thomas
did it this way because he doesn't yet have an easy way to get his
platform devices registered from the device tree.

g.