Re: [RFC PATCH 3.7.0-rc2] dt/platform: insert resources correctly into resource tree

[Grant Likely <grant.likely@secretlab.ca>]

On Fri,  2 Nov 2012 10:46:19 +0000, Srinivas KANDAGATLA <srinivas.kandagatla@st.com> wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> 
> This patch add new code to correctly add resources into platform device.
> Issue with the existing code was the resources are added as flat entry
> without creating any tree, this is very much different to what non-dt
> platform code does.
> 
> With this patch the resources appear correctly as tree in /proc/iomem,
> without this patch the resources in /proc/iomem appear as single entry.
> 
> i2c example in /proc/iomem:
> 
> With-patch:
> 
> fed41000-fed4110f : /soc/i2c-stm@fed41000
>   fed41000-fed4110f : i2c
> 
> Without patch:
> fed41000-fed4110f : i2c
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>

Yes, that is a problem that should be fixed.
Although it seems to me that some powerpc platforms will break due to
nodes with overlapping ranges. That will need to be tested.

I also don't like the duplication of code from platform_device_add().
Does something like this work for you instead? I might need to add an
exception to fallback onto of_device_add if the resource regions
overlay. Or modify platform_device_add() to complain about overlaps, but
not fail when on PowerPC.

g.

---

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 72c776f..2edf831 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -283,7 +283,7 @@ int platform_device_add(struct platform_device *pdev)
 	if (!pdev)
 		return -EINVAL;
 
-	if (!pdev->dev.parent)
+	if (!pdev->dev.parent && !pdev->dev.of_node)
 		pdev->dev.parent = &platform_bus;
 
 	pdev->dev.bus = &platform_bus_type;
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b80891b..fb9cbad 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -214,16 +214,22 @@ struct platform_device *of_platform_device_create_pdata(
 #if defined(CONFIG_MICROBLAZE)
 	dev->archdata.dma_mask = 0xffffffffUL;
 #endif
+	dev->name = dev_name(&dev->dev);
 	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
-	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
+	dev->dev.id = PLATFORM_DEVID_NONE;
+	/* device_add will assume that this device is on the same node as
+	 * the parent. If there is no parent defined, set the node
+	 * explicitly */
+	if (!parent)
+		set_dev_node(&dev->dev, of_node_to_nid(np));
 
 	/* We do not fill the DMA ops for platform devices by default.
 	 * This is currently the responsibility of the platform code
 	 * to do such, possibly using a device notifier
 	 */
 
-	if (of_device_add(dev) != 0) {
+	if (platform_device_add(dev)) {
 		platform_device_put(dev);
 		return NULL;
 	}


> ---
> Hi All, 
> Recently I noticed that appearance of /proc/iomem ouput changed 
> when I started using device trees and the reason for this was 
> the of-plaform code was not adding resources in same way as 
> non-dt platform code does.
> 
> Do you have any reason for not doing it the same way as non-dt platform code?
> 
> This patch is a fixup to that issue.
> 
> Comment?
> thanks,
> srini
> 
>  drivers/of/platform.c |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b80891b..f43922c 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -203,6 +203,7 @@ struct platform_device *of_platform_device_create_pdata(
>  					struct device *parent)
>  {
>  	struct platform_device *dev;
> +	int i;
>  
>  	if (!of_device_is_available(np))
>  		return NULL;
> @@ -218,6 +219,28 @@ struct platform_device *of_platform_device_create_pdata(
>  	dev->dev.bus = &platform_bus_type;
>  	dev->dev.platform_data = platform_data;
>  
> +	for (i = 0; i < dev->num_resources; i++) {
> +		struct resource *p, *r = &dev->resource[i];
> +
> +		if (r->name == NULL)
> +			r->name = dev_name(&dev->dev);
> +
> +		p = r->parent;
> +		if (!p) {
> +			if (resource_type(r) == IORESOURCE_MEM)
> +				p = &iomem_resource;
> +			else if (resource_type(r) == IORESOURCE_IO)
> +				p = &ioport_resource;
> +		}
> +
> +		if (p && insert_resource(p, r)) {
> +			pr_err("%s: failed to claim resource %d\n",
> +			       dev_name(&dev->dev), i);
> +			platform_device_put(dev);
> +			return NULL;
> +		}
> +	}
> +
>  	/* We do not fill the DMA ops for platform devices by default.
>  	 * This is currently the responsibility of the platform code
>  	 * to do such, possibly using a device notifier
> -- 
> 1.7.0.4
> 

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