[PATCH] of/platform: dynamic: Use of_platform_bus_create() to create devices

[PATCH] of/platform: dynamic: Use of_platform_bus_create() to create devices

[Alexander Sverdlin]

of_platform_notify() requires parents to have OF_POPULATED_BUS to be set
(which is correct way to verify healthy platform bus), but uses
of_platform_device_create() to create devices, which never sets the above
flag. Therefore it's not possible to add platform buses via overlays.

Use of_platform_bus_create(), which is used in non-overlay case to populate
both buses and devices.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

---
 drivers/of/platform.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index af98343..d2d6e74 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -521,6 +521,7 @@ static int of_platform_notify(struct notifier_block *nb,
 	struct of_reconfig_data *rd = arg;
 	struct platform_device *pdev_parent, *pdev;
 	bool children_left;
+	int ret;
 
 	switch (of_reconfig_get_state_change(action, rd)) {
 	case OF_RECONFIG_CHANGE_ADD:
@@ -534,15 +535,15 @@ static int of_platform_notify(struct notifier_block *nb,
 
 		/* 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);
+		ret = of_platform_bus_create(rd->dn, of_default_bus_match_table,
+					     NULL, pdev_parent ?
+					     &pdev_parent->dev : NULL, true);
 		of_dev_put(pdev_parent);
 
-		if (pdev == NULL) {
+		if (ret) {
 			pr_err("%s: failed to create for '%s'\n",
 					__func__, rd->dn->full_name);
-			/* of_platform_device_create tosses the error code */
-			return notifier_from_errno(-EINVAL);
+			return notifier_from_errno(ret);
 		}
 		break;
 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/platform: dynamic: Use of_platform_bus_create() to create devices

[Rob Herring]

On Thu, Oct 20, 2016 at 6:02 AM, Alexander Sverdlin
<alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:
> of_platform_notify() requires parents to have OF_POPULATED_BUS to be set
> (which is correct way to verify healthy platform bus), but uses
> of_platform_device_create() to create devices, which never sets the above
> flag. Therefore it's not possible to add platform buses via overlays.

Can you create a unit test case that has the problem.

> Use of_platform_bus_create(), which is used in non-overlay case to populate
> both buses and devices.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> ---
>  drivers/of/platform.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index af98343..d2d6e74 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -521,6 +521,7 @@ static int of_platform_notify(struct notifier_block *nb,
>         struct of_reconfig_data *rd = arg;
>         struct platform_device *pdev_parent, *pdev;
>         bool children_left;
> +       int ret;
>
>         switch (of_reconfig_get_state_change(action, rd)) {
>         case OF_RECONFIG_CHANGE_ADD:
> @@ -534,15 +535,15 @@ static int of_platform_notify(struct notifier_block *nb,
>
>                 /* 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);
> +               ret = of_platform_bus_create(rd->dn, of_default_bus_match_table,
> +                                            NULL, pdev_parent ?
> +                                            &pdev_parent->dev : NULL, true);

I think this should be of_platform_default_populate() instead.

>                 of_dev_put(pdev_parent);
>
> -               if (pdev == NULL) {
> +               if (ret) {
>                         pr_err("%s: failed to create for '%s'\n",
>                                         __func__, rd->dn->full_name);
> -                       /* of_platform_device_create tosses the error code */
> -                       return notifier_from_errno(-EINVAL);
> +                       return notifier_from_errno(ret);
>                 }
>                 break;
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/platform: dynamic: Use of_platform_bus_create() to create devices

[Alexander Sverdlin]

Hello Rob,

On 27/10/16 15:00, Rob Herring wrote:
>> @@ -534,15 +535,15 @@ static int of_platform_notify(struct notifier_block *nb,
>>
>>                 /* 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);
>> +               ret = of_platform_bus_create(rd->dn, of_default_bus_match_table,
>> +                                            NULL, pdev_parent ?
>> +                                            &pdev_parent->dev : NULL, true);
> I think this should be of_platform_default_populate() instead.

I've just tested: it cannot be. It calls of_platform_populate(), which ignores "status" property.
This not only brakes half of the unit tests, but also would change the existing behavior of
overlays (they will completely ignore "status" too).

-- 
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/platform: dynamic: Use of_platform_bus_create() to create devices

[Rob Herring]

On Fri, Jan 20, 2017 at 11:38 AM, Alexander Sverdlin
<alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:
> Hello Rob,
>
> On 27/10/16 15:00, Rob Herring wrote:
>>> @@ -534,15 +535,15 @@ static int of_platform_notify(struct notifier_block *nb,
>>>
>>>                 /* 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);
>>> +               ret = of_platform_bus_create(rd->dn, of_default_bus_match_table,
>>> +                                            NULL, pdev_parent ?
>>> +                                            &pdev_parent->dev : NULL, true);
>> I think this should be of_platform_default_populate() instead.
>
> I've just tested: it cannot be. It calls of_platform_populate(), which ignores "status" property.
> This not only brakes half of the unit tests, but also would change the existing behavior of
> overlays (they will completely ignore "status" too).

That's a bug. I assume that's only at the 1st level or things would be
completely broken.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] of/platform: Ignore disabled devices in of_platform_populate()

[Alexander Sverdlin]

Check the status property and give up early enough. Otherwise
OF_POPULATED_BUS might be set for disabled bus which will prevent
it from being populated later (if enabled).

Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/of/platform.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 35de1fc59fa6..5ad8c13f754a 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -473,9 +473,13 @@ int of_platform_populate(struct device_node *root,
 	struct device_node *child;
 	int rc = 0;
 
-	root = root ? of_node_get(root) : of_find_node_by_path("/");
+	if (!root)
+		root = of_find_node_by_path("/");
 	if (!root)
 		return -EINVAL;
+	if (!of_device_is_available(root))
+		return 0;
+	of_node_get(root);
 
 	pr_debug("%s()\n", __func__);
 	pr_debug(" starting at: %s\n", root->full_name);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/platform: Ignore disabled devices in of_platform_populate()

[Alexander Sverdlin]

Hello all,

On 23/01/17 13:21, Alexander Sverdlin wrote:
> Check the status property and give up early enough. Otherwise
> OF_POPULATED_BUS might be set for disabled bus which will prevent
> it from being populated later (if enabled).
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/of/platform.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 35de1fc59fa6..5ad8c13f754a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -473,9 +473,13 @@ int of_platform_populate(struct device_node *root,
>  	struct device_node *child;
>  	int rc = 0;
>  
> -	root = root ? of_node_get(root) : of_find_node_by_path("/");
> +	if (!root)
> +		root = of_find_node_by_path("/");
>  	if (!root)
>  		return -EINVAL;
> +	if (!of_device_is_available(root))
> +		return 0;
> +	of_node_get(root);

Auch, I see unbalanced reference counting here, please ignore this patch... 

>  	pr_debug("%s()\n", __func__);
>  	pr_debug(" starting at: %s\n", root->full_name);

-- 
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] of/platform: Ignore disabled devices in of_platform_populate()

[Alexander Sverdlin]

Check the status property and give up early enough. Otherwise
OF_POPULATED_BUS might be set for disabled bus which will prevent
it from being populated later (if enabled).

Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/of/platform.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 35de1fc59fa6..060621d94f1a 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -476,6 +476,10 @@ int of_platform_populate(struct device_node *root,
 	root = root ? of_node_get(root) : of_find_node_by_path("/");
 	if (!root)
 		return -EINVAL;
+	if (!of_device_is_available(root)) {
+		of_node_put(root);
+		return 0;
+	}
 
 	pr_debug("%s()\n", __func__);
 	pr_debug(" starting at: %s\n", root->full_name);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] of/platform: Ignore disabled devices in of_platform_populate()

[Alexander Sverdlin]

Hello all,

On 23/01/17 13:52, Alexander Sverdlin wrote:
> Check the status property and give up early enough. Otherwise
> OF_POPULATED_BUS might be set for disabled bus which will prevent
> it from being populated later (if enabled).
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/of/platform.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 35de1fc59fa6..060621d94f1a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -476,6 +476,10 @@ int of_platform_populate(struct device_node *root,
>  	root = root ? of_node_get(root) : of_find_node_by_path("/");
>  	if (!root)
>  		return -EINVAL;
> +	if (!of_device_is_available(root)) {
> +		of_node_put(root);
> +		return 0;
> +	}
>  
>  	pr_debug("%s()\n", __func__);
>  	pr_debug(" starting at: %s\n", root->full_name);

no, this one also doesn't allow to switch to of_platform_default_populate(),
please ignore the patch.


-- 
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] of/platform: Ignore disabled devices in of_platform_populate()

[Alexander Sverdlin]

On 23/01/17 15:33, Alexander Sverdlin wrote:
>> Check the status property and give up early enough. Otherwise
>> OF_POPULATED_BUS might be set for disabled bus which will prevent
>> it from being populated later (if enabled).
>>
>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> ---
>>  drivers/of/platform.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 35de1fc59fa6..060621d94f1a 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -476,6 +476,10 @@ int of_platform_populate(struct device_node *root,
>>  	root = root ? of_node_get(root) : of_find_node_by_path("/");
>>  	if (!root)
>>  		return -EINVAL;
>> +	if (!of_device_is_available(root)) {
>> +		of_node_put(root);
>> +		return 0;
>> +	}
>>  
>>  	pr_debug("%s()\n", __func__);
>>  	pr_debug(" starting at: %s\n", root->full_name);
> no, this one also doesn't allow to switch to of_platform_default_populate(),
> please ignore the patch.

Well, the patch seems to fix a real problem because OF_POPULATED_BUS would be
set for disabled buses, which will prevent their enabling later. Even if there
is no in-tree call site stumbles upon this issue. 

-- 
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] of/platform: Ignore disabled devices in of_platform_populate()

[Alexander Sverdlin]

PING?

On 23/01/17 13:52, Alexander Sverdlin wrote:
> Check the status property and give up early enough. Otherwise
> OF_POPULATED_BUS might be set for disabled bus which will prevent
> it from being populated later (if enabled).
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/of/platform.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 35de1fc59fa6..060621d94f1a 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -476,6 +476,10 @@ int of_platform_populate(struct device_node *root,
>  	root = root ? of_node_get(root) : of_find_node_by_path("/");
>  	if (!root)
>  		return -EINVAL;
> +	if (!of_device_is_available(root)) {
> +		of_node_put(root);
> +		return 0;
> +	}
>  
>  	pr_debug("%s()\n", __func__);
>  	pr_debug(" starting at: %s\n", root->full_name);

-- 
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/platform: dynamic: Use of_platform_bus_create() to create devices

[Alexander Sverdlin]

Hi Rob,

On 21/01/17 20:51, Rob Herring wrote:
> On Fri, Jan 20, 2017 at 11:38 AM, Alexander Sverdlin
> <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:
>> Hello Rob,
>>
>> On 27/10/16 15:00, Rob Herring wrote:
>>>> @@ -534,15 +535,15 @@ static int of_platform_notify(struct notifier_block *nb,
>>>>
>>>>                 /* 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);
>>>> +               ret = of_platform_bus_create(rd->dn, of_default_bus_match_table,
>>>> +                                            NULL, pdev_parent ?
>>>> +                                            &pdev_parent->dev : NULL, true);
>>> I think this should be of_platform_default_populate() instead.
>> I've just tested: it cannot be. It calls of_platform_populate(), which ignores "status" property.
>> This not only brakes half of the unit tests, but also would change the existing behavior of
>> overlays (they will completely ignore "status" too).
> That's a bug. I assume that's only at the 1st level or things would be
> completely broken.

well, after two unsuccessful attempts to use of_platform_default_populate() in of_platform_notify()
I see what the actual problem is: of_platform_*populate() do not populate the root node itself,
therefore they are not suitable for of_platform_notify().

-- 
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] Ensure that platform buses from overlays are functional

[Alexander Sverdlin]

This series addresses an issue, which prevents one to add child devices to
a platform bus from an overlay if such a bus was in turn also created from
an overlay.

The root cause is missing OF_POPULATED_BUS flag for such buses and the
difference, in the way, how static and dynamic platform devices are
populated.

Second patch adds a couple of test cases which demonstrate the problem.

This series relies on the correct functioning of
of_unittest_apply_overlay() (previously sent "of/unittest: Swap arguments
of of_unittest_apply_overlay()").

Alexander Sverdlin (2):
  of/platform: dynamic: Use of_platform_bus_create() to create devices
  of/unittest: Test platform bus added from overlay

 drivers/of/platform.c                       | 11 ++++----
 drivers/of/unittest-data/tests-overlay.dtsi | 41 +++++++++++++++++++++++++++++
 drivers/of/unittest.c                       | 31 ++++++++++++++++++++++
 3 files changed, 78 insertions(+), 5 deletions(-)

Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] of/platform: dynamic: Use of_platform_bus_create() to create devices

[Alexander Sverdlin]

of_platform_notify() requires parents to have OF_POPULATED_BUS to be set
(which is correct way to verify healthy platform bus), but uses
of_platform_device_create() to create devices, which never sets the above
flag. Therefore it's not possible to add platform buses via overlays.

Use of_platform_bus_create(), which is used in non-overlay case to populate
both buses and devices.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 drivers/of/platform.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b8064bc2b6eb..35de1fc59fa6 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -578,6 +578,7 @@ static int of_platform_notify(struct notifier_block *nb,
 	struct of_reconfig_data *rd = arg;
 	struct platform_device *pdev_parent, *pdev;
 	bool children_left;
+	int ret;
 
 	switch (of_reconfig_get_state_change(action, rd)) {
 	case OF_RECONFIG_CHANGE_ADD:
@@ -591,15 +592,15 @@ static int of_platform_notify(struct notifier_block *nb,
 
 		/* 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);
+		ret = of_platform_bus_create(rd->dn, of_default_bus_match_table,
+					     NULL, pdev_parent ?
+					     &pdev_parent->dev : NULL, true);
 		of_dev_put(pdev_parent);
 
-		if (pdev == NULL) {
+		if (ret) {
 			pr_err("%s: failed to create for '%s'\n",
 					__func__, rd->dn->full_name);
-			/* of_platform_device_create tosses the error code */
-			return notifier_from_errno(-EINVAL);
+			return notifier_from_errno(ret);
 		}
 		break;
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] of/unittest: Test platform bus added from overlay

[Alexander Sverdlin]

Check that it's possible to:
- add children to platform buses originating from overlays
- use overlay to enable a platform bus originating from overlay

Explicitly avoid unittest_probe(), because this test case checks the correct
creation of the devices only. Driver probing might be delayed (say, modules)
and correct device creation must not depend on it.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 drivers/of/unittest-data/tests-overlay.dtsi | 83 +++++++++++++++++++++++++++++
 drivers/of/unittest.c                       | 60 +++++++++++++++++++++
 2 files changed, 143 insertions(+)

diff --git a/drivers/of/unittest-data/tests-overlay.dtsi b/drivers/of/unittest-data/tests-overlay.dtsi
index 02ba56c20fe1..6925a992f9e2 100644
--- a/drivers/of/unittest-data/tests-overlay.dtsi
+++ b/drivers/of/unittest-data/tests-overlay.dtsi
@@ -325,5 +325,88 @@
 			};
 		};
 
+		/* test that platform buses from overlays are functional */
+		overlay16 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus";
+				__overlay__ {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					test-unittest16 {
+						/*
+						 * testing just device creation,
+						 * no unittest_probe() should
+						 * be called
+						 */
+						compatible = "simple-bus";
+						reg = <16>;
+					};
+				};
+			};
+		};
+
+		overlay17 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus";
+				__overlay__ {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					test-unittest16 {
+						#address-cells = <1>;
+						#size-cells = <0>;
+
+						test-unittest161 {
+							compatible = "unittest";
+							reg = <1>;
+						};
+					};
+				};
+			};
+		};
+
+		overlay18 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus";
+				__overlay__ {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					test-unittest18 {
+						#address-cells = <1>;
+						#size-cells = <0>;
+						/*
+						 * testing just device creation,
+						 * no unittest_probe() should
+						 * be called
+						 */
+						compatible = "simple-bus";
+						reg = <18>;
+						status = "disabled";
+
+						test-unittest181 {
+							compatible = "unittest";
+							reg = <1>;
+						};
+					};
+				};
+			};
+		};
+
+		overlay19 {
+			fragment@0 {
+				target-path = "/testcase-data/overlay-node/test-bus";
+				__overlay__ {
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					test-unittest18 {
+						status = "okay";
+					};
+				};
+			};
+		};
+
 	};
 };
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 66bfa8d674ce..2c533709bb0e 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1854,6 +1854,63 @@ static inline void of_unittest_overlay_i2c_15(void) { }
 
 #endif
 
+/* test that platform buses populated by overlays are functional */
+static void of_unittest_overlay_16(void)
+{
+	int ret;
+	char *child_path;
+
+	/* add an empty bus */
+	ret = of_unittest_apply_overlay_check(16, 16, 0, 1, PDEV_OVERLAY);
+	if (unittest(ret == 0,
+			"overlay test %d failed; overlay application\n", 16))
+		return;
+
+	/* add a device to the bus */
+	ret = of_unittest_apply_overlay(17, 16, PDEV_OVERLAY);
+	if (unittest(ret == 0,
+			"overlay test %d failed; overlay application\n", 16))
+		return;
+
+	child_path = kasprintf(GFP_KERNEL, "%s/test-unittest161",
+			unittest_path(16, PDEV_OVERLAY));
+	if (unittest(child_path, "overlay test %d failed; kasprintf\n", 16))
+		return;
+
+	ret = of_path_device_type_exists(child_path, PDEV_OVERLAY);
+	kfree(child_path);
+	if (unittest(ret, "overlay test %d failed; no child device\n", 16))
+		return;
+}
+
+static void of_unittest_overlay_18(void)
+{
+	int ret;
+	char *child_path;
+
+	/* add a disabled bus */
+	ret = of_unittest_apply_overlay_check(18, 18, 0, 0, PDEV_OVERLAY);
+	if (unittest(ret == 0,
+			"overlay test %d failed; overlay application\n", 18))
+		return;
+
+	/* enable the bus */
+	ret = of_unittest_apply_overlay_check(19, 18, 0, 1, PDEV_OVERLAY);
+	if (unittest(ret == 0,
+			"overlay test %d failed; overlay application\n", 18))
+		return;
+
+	child_path = kasprintf(GFP_KERNEL, "%s/test-unittest181",
+			unittest_path(18, PDEV_OVERLAY));
+	if (unittest(child_path, "overlay test %d failed; kasprintf\n", 18))
+		return;
+
+	ret = of_path_device_type_exists(child_path, PDEV_OVERLAY);
+	kfree(child_path);
+	if (unittest(ret, "overlay test %d failed; no child device\n", 18))
+		return;
+}
+
 static void __init of_unittest_overlay(void)
 {
 	struct device_node *bus_np = NULL;
@@ -1916,6 +1973,9 @@ static void __init of_unittest_overlay(void)
 	of_unittest_overlay_i2c_cleanup();
 #endif
 
+	of_unittest_overlay_16();
+	of_unittest_overlay_18();
+
 	of_unittest_destroy_tracked_overlays();
 
 out:
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/platform: dynamic: Use of_platform_bus_create() to create devices

[Alexander Sverdlin]

Hello Rob,

On 27/10/16 15:00, Rob Herring wrote:
> On Thu, Oct 20, 2016 at 6:02 AM, Alexander Sverdlin
> <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:
>> of_platform_notify() requires parents to have OF_POPULATED_BUS to be set
>> (which is correct way to verify healthy platform bus), but uses
>> of_platform_device_create() to create devices, which never sets the above
>> flag. Therefore it's not possible to add platform buses via overlays.
> Can you create a unit test case that has the problem.

PING
The tests for the highlighted problem were presented here:
http://www.spinics.net/lists/devicetree/msg160016.html
 
>> Use of_platform_bus_create(), which is used in non-overlay case to populate
>> both buses and devices.

>> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> ---
>>  drivers/of/platform.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index af98343..d2d6e74 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -521,6 +521,7 @@ static int of_platform_notify(struct notifier_block *nb,
>>         struct of_reconfig_data *rd = arg;
>>         struct platform_device *pdev_parent, *pdev;
>>         bool children_left;
>> +       int ret;
>>
>>         switch (of_reconfig_get_state_change(action, rd)) {
>>         case OF_RECONFIG_CHANGE_ADD:
>> @@ -534,15 +535,15 @@ static int of_platform_notify(struct notifier_block *nb,
>>
>>                 /* 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);
>> +               ret = of_platform_bus_create(rd->dn, of_default_bus_match_table,
>> +                                            NULL, pdev_parent ?
>> +                                            &pdev_parent->dev : NULL, true);
> I think this should be of_platform_default_populate() instead.
> 
>>                 of_dev_put(pdev_parent);
>>
>> -               if (pdev == NULL) {
>> +               if (ret) {
>>                         pr_err("%s: failed to create for '%s'\n",
>>                                         __func__, rd->dn->full_name);
>> -                       /* of_platform_device_create tosses the error code */
>> -                       return notifier_from_errno(-EINVAL);
>> +                       return notifier_from_errno(ret);
>>                 }
>>                 break;

-- 
Best regards,
Alexander Sverdlin.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/platform: dynamic: Use of_platform_bus_create() to create devices

[Rob Herring]

On Tue, Sep 19, 2017 at 10:13 AM, Alexander Sverdlin
<alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:
> Hello Rob,
>
> On 27/10/16 15:00, Rob Herring wrote:
>> On Thu, Oct 20, 2016 at 6:02 AM, Alexander Sverdlin
>> <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote:
>>> of_platform_notify() requires parents to have OF_POPULATED_BUS to be set
>>> (which is correct way to verify healthy platform bus), but uses
>>> of_platform_device_create() to create devices, which never sets the above
>>> flag. Therefore it's not possible to add platform buses via overlays.
>> Can you create a unit test case that has the problem.
>
> PING
> The tests for the highlighted problem were presented here:
> http://www.spinics.net/lists/devicetree/msg160016.html

Can you please rebase and resend these.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html