Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] of/platform: Support dynamic device tree on AMBA bus
To:     Frank Rowand <frowand.list@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Jaewon Kim <jaewon02.kim@samsung.com>
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <1540485597-12240-1-git-send-email-jaewon02.kim@samsung.com>
 <25f61a9b-af37-18b1-9975-cb4ccbbb3aa7@gmail.com>
From:   Jaewon Kim <jaewon02.kim@gmail.com>
Message-ID: <6946c801-0ea3-d980-ebe0-12eb5a9d2186@gmail.com>
Date:   Thu, 1 Nov 2018 00:32:31 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <25f61a9b-af37-18b1-9975-cb4ccbbb3aa7@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Frank,


Thanks to review my patch.

On 18. 10. 31. 오전 8:04, Frank Rowand wrote:
> Hi Jaewon,
>
> On 10/25/18 9:39 AM, Jaewon Kim wrote:
>> This patch supports dynamic device-tree for AMBA device.
> Add AMBA devices and buses to of_platform_notify() so that dynamic device-tree
> will support AMBA.

What is meaning of this comment?

I already adds AMBA to of_platform_notify().

>
>> The AMBA device must be registered on the AMBA bus, not the platform bus.
>>
>> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
>> ---
>>   drivers/of/platform.c | 93 ++++++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 73 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 04ad312..b9ac105 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -286,6 +286,22 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>>   	of_node_clear_flag(node, OF_POPULATED);
>>   	return NULL;
>>   }
>> +
>> +/**
>> + * of_find_amba_device_by_node - Find the amba_device associated with a node
>> + * @np: Pointer to device tree node
>> + *
>> + * Returns amba_device pointer, or NULL if not found
>> + */
>> +struct amba_device *of_find_amba_device_by_node(struct device_node *np)
>> +{
>> +	struct device *dev;
>> +
>> +	dev = bus_find_device(&amba_bustype, NULL, np, of_dev_node_match);
>> +	return dev ? to_amba_device(dev) : NULL;
>> +}
>> +EXPORT_SYMBOL(of_find_amba_device_by_node);
>> +
>>   #else /* CONFIG_ARM_AMBA */
>>   static struct amba_device *of_amba_device_create(struct device_node *node,
>>   						 const char *bus_id,
>> @@ -294,6 +310,12 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>>   {
>>   	return NULL;
>>   }
>> +
>> +struct amba_device *of_find_amba_device_by_node(struct device_node *np)
>> +{
>> +	return NULL;
>> +}
>> +EXPORT_SYMBOL(of_find_amba_device_by_node);
>>   #endif /* CONFIG_ARM_AMBA */
>>   
>>   /**
>> @@ -665,6 +687,7 @@ static int of_platform_notify(struct notifier_block *nb,
>>   {
>>   	struct of_reconfig_data *rd = arg;
>>   	struct platform_device *pdev_parent, *pdev;
>> +	struct amba_device *adev_p, *adev;
>>   	bool children_left;
>>   
>>   	switch (of_reconfig_get_state_change(action, rd)) {
>> @@ -677,17 +700,34 @@ static int of_platform_notify(struct notifier_block *nb,
>>   		if (of_node_check_flag(rd->dn, OF_POPULATED))
>>   			return NOTIFY_OK;
>>   
>> -		/* pdev_parent may be NULL when no bus platform device */
>> -		pdev_parent = of_find_device_by_node(rd->dn->parent);
>> -		pdev = of_platform_device_create(rd->dn, NULL,
>> -				pdev_parent ? &pdev_parent->dev : NULL);
>> -		of_dev_put(pdev_parent);
>> -
>> -		if (pdev == NULL) {
>> -			pr_err("%s: failed to create for '%pOF'\n",
>> -					__func__, rd->dn);
>> -			/* of_platform_device_create tosses the error code */
>> -			return notifier_from_errno(-EINVAL);
>> +		if (of_device_is_compatible(rd->dn, "arm,primecell")) {
>> +			/* adev_p may be NULL when no bus amba device */
>> +			adev_p = of_find_amba_device_by_node(rd->dn->parent);
>> +			adev = of_amba_device_create(rd->dn, NULL, NULL,
>> +					adev_p ? &adev_p->dev : NULL);
>> +
>> +			if (adev_p)
>> +				put_device(&adev_p->dev);
> Please follow the same model as of_dev_put() here, create a new function
> of_amba_dev_put() to implement these two lines.
Okay, I will create of_amba_dev_put() function.
>
>
>> +
>> +			if (adev == NULL) {
>> +				pr_err("%s: failed to create for '%s'\n",
>> +						__func__, rd->dn->full_name);
>> +				/* of_amba_device_create tosses the error */
>> +				return notifier_from_errno(-EINVAL);
>> +			}
>> +		} else {
>> +			/* pdev_parent may be NULL when no bus platform device*/
>> +			pdev_parent = of_find_device_by_node(rd->dn->parent);
>> +			pdev = of_platform_device_create(rd->dn, NULL,
>> +					pdev_parent ? &pdev_parent->dev : NULL);
>> +			of_dev_put(pdev_parent);
>> +
>> +			if (pdev == NULL) {
>> +				pr_err("%s: failed to create for '%pOF'\n",
>> +						__func__, rd->dn);
>> +				/* of_platform_device_create tosses the error */
>> +				return notifier_from_errno(-EINVAL);
>> +			}
>>   		}
>>   		break;
>>   
>> @@ -698,15 +738,28 @@ static int of_platform_notify(struct notifier_block *nb,
>>   			return NOTIFY_OK;
>>   
>>   		/* find our device by node */
>> -		pdev = of_find_device_by_node(rd->dn);
>> -		if (pdev == NULL)
>> -			return NOTIFY_OK;	/* no? not meant for us */
>> -
>> -		/* unregister takes one ref away */
>> -		of_platform_device_destroy(&pdev->dev, &children_left);
>> -
>> -		/* and put the reference of the find */
>> -		of_dev_put(pdev);
>> +		if (of_device_is_compatible(rd->dn, "arm,primecell")) {
>> +			adev = of_find_amba_device_by_node(rd->dn);
>> +			if (adev == NULL)
>> +				return NOTIFY_OK; /* no? not meant for us */
>> +
>> +			/* unregister takes one ref away */
>> +			of_platform_device_destroy(&adev->dev, &children_left);
>> +
>> +			/* and put the reference of the find */
>> +			if (adev)
>> +				put_device(&adev->dev);
> of_amba_dev_put() here also
>
> -Frank
Okay, I will create it.
>
>> +		} else {
>> +			pdev = of_find_device_by_node(rd->dn);
>> +			if (pdev == NULL)
>> +				return NOTIFY_OK; /* no? not meant for us */
>> +
>> +			/* unregister takes one ref away */
>> +			of_platform_device_destroy(&pdev->dev, &children_left);
>> +
>> +			/* and put the reference of the find */
>> +			of_dev_put(pdev);
>> +		}
>>   		break;
>>   	}
>>   
>>
Thanks

Jaewon Kim