Re: [PATCH] of/platform: Support dynamic device tree on AMBA bus

[Frank Rowand]

On 10/31/18 8:32 AM, Jaewon Kim wrote:
> 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().

I was trying to make the commit comment more explanatory.  (And finding it difficult
to remain brief, yet meaningful.)  The original patch header comment did not tell me
why the patch changes added support of dynamic devicetree on AMBA - I had to read the
patch before I understood what it did.  If you can think of better words than I
suggested, please come up with different wording.

My intent with this sentence was that more explanation needed to be added to the
comment, not that the code needed to change in any way other than what I noted
in-line inside the patch.  Does this make things more clear, or am I misunderstanding
your question?

-Frank


> 
>>
>>> The AMBA device must be registered on the AMBA bus, not the platform bus.
>>>
>>> Signed-off-by: Jaewon Kim <jaewon02.kim@xxxxxxxxxxx>
>>> ---
>>> Â 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
> 
>