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

[Srinivas KANDAGATLA <srinivas.kandagatla@st.com>]

On 15/11/12 13:11, Grant Likely wrote:
> 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().
I agree, I don't like the duplication too.
> Does something like this work for you instead? 
Yes, it works for me and I have tested your patch.

Tested-by: Srinivas Kandagatla<srinivas.kandagatla@st.com>

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