linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] firmware: coreboot: Let OF core populate platform device
@ 2018-08-06 17:10 Stephen Boyd
  2018-08-06 17:22 ` Sudeep Holla
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Stephen Boyd @ 2018-08-06 17:10 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Wei-Ning Huang, Julius Werner, Brian Norris,
	Samuel Holland, Thierry Escande

Now that the /firmware/coreboot node in DT is populated by the core DT
platform code with commit 3aa0582fdb82 ("of: platform: populate
/firmware/ node from of_platform_default_populate_init()") we should and
can remove the platform device creation here. Otherwise, the
of_platform_device_create() call will fail, the coreboot of driver won't
be registered, and this driver will never bind. At the same time, we can
move this driver to use platform device APIs instead of DT specific ones
and drop the of_node handling that was presumably placed there to hold a
reference to the DT node created during module init.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Cc: Thierry Escande <thierry.escande@collabora.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table-of.c | 39 +++++----------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
index f15bf404c579..270f112bdc54 100644
--- a/drivers/firmware/google/coreboot_table-of.c
+++ b/drivers/firmware/google/coreboot_table-of.c
@@ -18,21 +18,20 @@
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/module.h>
-#include <linux/of_address.h>
-#include <linux/of_platform.h>
+#include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
 
 #include "coreboot_table.h"
 
 static int coreboot_table_of_probe(struct platform_device *pdev)
 {
-	struct device_node *fw_dn = pdev->dev.of_node;
 	void __iomem *ptr;
+	struct resource *res;
 
-	ptr = of_iomap(fw_dn, 0);
-	of_node_put(fw_dn);
-	if (!ptr)
-		return -ENOMEM;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ptr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
 
 	return coreboot_table_init(&pdev->dev, ptr);
 }
@@ -44,8 +43,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
 
 static const struct of_device_id coreboot_of_match[] = {
 	{ .compatible = "coreboot" },
-	{},
+	{}
 };
+MODULE_DEVICE_TABLE(of, coreboot_of_match);
 
 static struct platform_driver coreboot_table_of_driver = {
 	.probe = coreboot_table_of_probe,
@@ -55,28 +55,7 @@ static struct platform_driver coreboot_table_of_driver = {
 		.of_match_table = coreboot_of_match,
 	},
 };
-
-static int __init platform_coreboot_table_of_init(void)
-{
-	struct platform_device *pdev;
-	struct device_node *of_node;
-
-	/* Limit device creation to the presence of /firmware/coreboot node */
-	of_node = of_find_node_by_path("/firmware/coreboot");
-	if (!of_node)
-		return -ENODEV;
-
-	if (!of_match_node(coreboot_of_match, of_node))
-		return -ENODEV;
-
-	pdev = of_platform_device_create(of_node, "coreboot_table_of", NULL);
-	if (!pdev)
-		return -ENODEV;
-
-	return platform_driver_register(&coreboot_table_of_driver);
-}
-
-module_init(platform_coreboot_table_of_init);
+module_platform_driver(coreboot_table_of_driver);
 
 MODULE_AUTHOR("Google, Inc.");
 MODULE_LICENSE("GPL");
-- 
Sent by a computer through tubes


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: coreboot: Let OF core populate platform device
  2018-08-06 17:10 [PATCH] firmware: coreboot: Let OF core populate platform device Stephen Boyd
@ 2018-08-06 17:22 ` Sudeep Holla
  2018-08-06 17:38 ` Brian Norris
  2018-08-07 21:41 ` Brian Norris
  2 siblings, 0 replies; 8+ messages in thread
From: Sudeep Holla @ 2018-08-06 17:22 UTC (permalink / raw)
  To: swboyd
  Cc: Greg Kroah-Hartman, open list, wnhuang, jwerner, briannorris,
	samuel, thierry.escande, Sudeep Holla

On Mon, Aug 6, 2018 at 6:13 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Now that the /firmware/coreboot node in DT is populated by the core DT
> platform code with commit 3aa0582fdb82 ("of: platform: populate
> /firmware/ node from of_platform_default_populate_init()") we should and
> can remove the platform device creation here. Otherwise, the
> of_platform_device_create() call will fail, the coreboot of driver won't
> be registered, and this driver will never bind. At the same time, we can
> move this driver to use platform device APIs instead of DT specific ones
> and drop the of_node handling that was presumably placed there to hold a
> reference to the DT node created during module init.
>

Nice cleanup.

Reviewed-by: Sudeep Holla <Sudeep.Holla@arm.com>

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: coreboot: Let OF core populate platform device
  2018-08-06 17:10 [PATCH] firmware: coreboot: Let OF core populate platform device Stephen Boyd
  2018-08-06 17:22 ` Sudeep Holla
@ 2018-08-06 17:38 ` Brian Norris
  2018-08-06 18:05   ` Stephen Boyd
  2018-08-07 21:41 ` Brian Norris
  2 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2018-08-06 17:38 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland, Thierry Escande

On Mon, Aug 06, 2018 at 10:10:47AM -0700, Stephen Boyd wrote:
> Now that the /firmware/coreboot node in DT is populated by the core DT
> platform code with commit 3aa0582fdb82 ("of: platform: populate
> /firmware/ node from of_platform_default_populate_init()") we should and

Does this deserve a 'Fixes' tag?

Fixes: 3aa0582fdb82 ("of: platform: populate /firmware/ node from of_platform_default_populate_init()")

The coreboot-of driver was in 4.12, and it seems to have broken in 4.16,
along with the above patch.

> can remove the platform device creation here. Otherwise, the
> of_platform_device_create() call will fail, the coreboot of driver won't
> be registered, and this driver will never bind. At the same time, we can
> move this driver to use platform device APIs instead of DT specific ones
> and drop the of_node handling that was presumably placed there to hold a
> reference to the DT node created during module init.
> 
> Cc: Wei-Ning Huang <wnhuang@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>

Reviewed-by: Brian Norris <briannorris@chromium.org>

> Cc: Samuel Holland <samuel@sholland.org>
> Cc: Thierry Escande <thierry.escande@collabora.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/firmware/google/coreboot_table-of.c | 39 +++++----------------
>  1 file changed, 9 insertions(+), 30 deletions(-)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: coreboot: Let OF core populate platform device
  2018-08-06 17:38 ` Brian Norris
@ 2018-08-06 18:05   ` Stephen Boyd
  2018-08-06 18:29     ` Julius Werner
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2018-08-06 18:05 UTC (permalink / raw)
  To: Brian Norris
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland, Thierry Escande

Quoting Brian Norris (2018-08-06 10:38:08)
> On Mon, Aug 06, 2018 at 10:10:47AM -0700, Stephen Boyd wrote:
> > Now that the /firmware/coreboot node in DT is populated by the core DT
> > platform code with commit 3aa0582fdb82 ("of: platform: populate
> > /firmware/ node from of_platform_default_populate_init()") we should and
> 
> Does this deserve a 'Fixes' tag?
> 
> Fixes: 3aa0582fdb82 ("of: platform: populate /firmware/ node from of_platform_default_populate_init()")
> 
> The coreboot-of driver was in 4.12, and it seems to have broken in 4.16,
> along with the above patch.

Yes. A fixes tag should be added.

> 
> > can remove the platform device creation here. Otherwise, the
> > of_platform_device_create() call will fail, the coreboot of driver won't
> > be registered, and this driver will never bind. At the same time, we can
> > move this driver to use platform device APIs instead of DT specific ones
> > and drop the of_node handling that was presumably placed there to hold a
> > reference to the DT node created during module init.
> > 
> > Cc: Wei-Ning Huang <wnhuang@chromium.org>
> > Cc: Julius Werner <jwerner@chromium.org>
> > Cc: Brian Norris <briannorris@chromium.org>
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: coreboot: Let OF core populate platform device
  2018-08-06 18:05   ` Stephen Boyd
@ 2018-08-06 18:29     ` Julius Werner
  0 siblings, 0 replies; 8+ messages in thread
From: Julius Werner @ 2018-08-06 18:29 UTC (permalink / raw)
  To: swboyd
  Cc: Brian Norris, Greg Kroah-Hartman, LKML, wnhuang, Julius Werner,
	samuel, Thierry Escande

Thanks for the quick fix!

Reviewed-by: Julius Werner <jwerner@chromium.org>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: coreboot: Let OF core populate platform device
  2018-08-06 17:10 [PATCH] firmware: coreboot: Let OF core populate platform device Stephen Boyd
  2018-08-06 17:22 ` Sudeep Holla
  2018-08-06 17:38 ` Brian Norris
@ 2018-08-07 21:41 ` Brian Norris
  2018-08-08  6:00   ` Stephen Boyd
  2 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2018-08-07 21:41 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland, Thierry Escande

Hi Stephen,

Although I already 'Reviewed' this, I noticed another problem when I
went to look again:

On Mon, Aug 06, 2018 at 10:10:47AM -0700, Stephen Boyd wrote:
> Now that the /firmware/coreboot node in DT is populated by the core DT
> platform code with commit 3aa0582fdb82 ("of: platform: populate
> /firmware/ node from of_platform_default_populate_init()") we should and
> can remove the platform device creation here. Otherwise, the
> of_platform_device_create() call will fail, the coreboot of driver won't
> be registered, and this driver will never bind. At the same time, we can
> move this driver to use platform device APIs instead of DT specific ones
> and drop the of_node handling that was presumably placed there to hold a
> reference to the DT node created during module init.
> 
> Cc: Wei-Ning Huang <wnhuang@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Samuel Holland <samuel@sholland.org>
> Cc: Thierry Escande <thierry.escande@collabora.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/firmware/google/coreboot_table-of.c | 39 +++++----------------
>  1 file changed, 9 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
> index f15bf404c579..270f112bdc54 100644
> --- a/drivers/firmware/google/coreboot_table-of.c
> +++ b/drivers/firmware/google/coreboot_table-of.c
> @@ -18,21 +18,20 @@
>  #include <linux/device.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> -#include <linux/of_address.h>
> -#include <linux/of_platform.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
>  
>  #include "coreboot_table.h"
>  
>  static int coreboot_table_of_probe(struct platform_device *pdev)
>  {
> -	struct device_node *fw_dn = pdev->dev.of_node;
>  	void __iomem *ptr;
> +	struct resource *res;
>  
> -	ptr = of_iomap(fw_dn, 0);
> -	of_node_put(fw_dn);
> -	if (!ptr)
> -		return -ENOMEM;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ptr = devm_ioremap_resource(&pdev->dev, res);

You're making this a devm_* ioremap, but coreboot_table_exit() will
handle unmapping this for you already. So you're introducing a potential
double-unmap.

> +	if (IS_ERR(ptr))
> +		return PTR_ERR(ptr);
>  
>  	return coreboot_table_init(&pdev->dev, ptr);
>  }
> @@ -44,8 +43,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id coreboot_of_match[] = {
>  	{ .compatible = "coreboot" },
> -	{},
> +	{}

...and since I had a *real* comment, I'll mention this bit I noticed
previously...this diff seems a bit superfluous. I understand we don't
really want to extend this list beyond the NULL terminator, but
normally, I'd expect we leave it alone.

Brian

>  };
> +MODULE_DEVICE_TABLE(of, coreboot_of_match);
>  
>  static struct platform_driver coreboot_table_of_driver = {
>  	.probe = coreboot_table_of_probe,
> @@ -55,28 +55,7 @@ static struct platform_driver coreboot_table_of_driver = {
>  		.of_match_table = coreboot_of_match,
>  	},
>  };
> -
> -static int __init platform_coreboot_table_of_init(void)
> -{
> -	struct platform_device *pdev;
> -	struct device_node *of_node;
> -
> -	/* Limit device creation to the presence of /firmware/coreboot node */
> -	of_node = of_find_node_by_path("/firmware/coreboot");
> -	if (!of_node)
> -		return -ENODEV;
> -
> -	if (!of_match_node(coreboot_of_match, of_node))
> -		return -ENODEV;
> -
> -	pdev = of_platform_device_create(of_node, "coreboot_table_of", NULL);
> -	if (!pdev)
> -		return -ENODEV;
> -
> -	return platform_driver_register(&coreboot_table_of_driver);
> -}
> -
> -module_init(platform_coreboot_table_of_init);
> +module_platform_driver(coreboot_table_of_driver);
>  
>  MODULE_AUTHOR("Google, Inc.");
>  MODULE_LICENSE("GPL");
> -- 
> Sent by a computer through tubes
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: coreboot: Let OF core populate platform device
  2018-08-07 21:41 ` Brian Norris
@ 2018-08-08  6:00   ` Stephen Boyd
  2018-08-09 17:13     ` Brian Norris
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2018-08-08  6:00 UTC (permalink / raw)
  To: Brian Norris
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland, Thierry Escande

Quoting Brian Norris (2018-08-07 14:41:04)
> On Mon, Aug 06, 2018 at 10:10:47AM -0700, Stephen Boyd wrote:
> > diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
> > index f15bf404c579..270f112bdc54 100644
> > --- a/drivers/firmware/google/coreboot_table-of.c
> > +++ b/drivers/firmware/google/coreboot_table-of.c
> > @@ -18,21 +18,20 @@
> >  #include <linux/device.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > -#include <linux/of_address.h>
> > -#include <linux/of_platform.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/platform_device.h>
> >  
> >  #include "coreboot_table.h"
> >  
> >  static int coreboot_table_of_probe(struct platform_device *pdev)
> >  {
> > -     struct device_node *fw_dn = pdev->dev.of_node;
> >       void __iomem *ptr;
> > +     struct resource *res;
> >  
> > -     ptr = of_iomap(fw_dn, 0);
> > -     of_node_put(fw_dn);
> > -     if (!ptr)
> > -             return -ENOMEM;
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     ptr = devm_ioremap_resource(&pdev->dev, res);
> 
> You're making this a devm_* ioremap, but coreboot_table_exit() will
> handle unmapping this for you already. So you're introducing a potential
> double-unmap.

Thanks! I'll split this patch into essential and non-essential pieces
then and layer shortening patches on top.

> 
> > +     if (IS_ERR(ptr))
> > +             return PTR_ERR(ptr);
> >  
> >       return coreboot_table_init(&pdev->dev, ptr);
> >  }
> > @@ -44,8 +43,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
> >  
> >  static const struct of_device_id coreboot_of_match[] = {
> >       { .compatible = "coreboot" },
> > -     {},
> > +     {}
> 
> ...and since I had a *real* comment, I'll mention this bit I noticed
> previously...this diff seems a bit superfluous. I understand we don't
> really want to extend this list beyond the NULL terminator, but
> normally, I'd expect we leave it alone.
> 

I was adding MODULE_DEVICE_TABLE nearby. I'll leave the removal here
unless you really hate the style cleanup.

> >  };
> > +MODULE_DEVICE_TABLE(of, coreboot_of_match);
> >  

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: coreboot: Let OF core populate platform device
  2018-08-08  6:00   ` Stephen Boyd
@ 2018-08-09 17:13     ` Brian Norris
  0 siblings, 0 replies; 8+ messages in thread
From: Brian Norris @ 2018-08-09 17:13 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland, Thierry Escande

On Tue, Aug 07, 2018 at 11:00:50PM -0700, Stephen Boyd wrote:
> Quoting Brian Norris (2018-08-07 14:41:04)
> > On Mon, Aug 06, 2018 at 10:10:47AM -0700, Stephen Boyd wrote:
> > > @@ -44,8 +43,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
> > >  
> > >  static const struct of_device_id coreboot_of_match[] = {
> > >       { .compatible = "coreboot" },
> > > -     {},
> > > +     {}
> > 
> > ...and since I had a *real* comment, I'll mention this bit I noticed
> > previously...this diff seems a bit superfluous. I understand we don't
> > really want to extend this list beyond the NULL terminator, but
> > normally, I'd expect we leave it alone.
> > 
> 
> I was adding MODULE_DEVICE_TABLE nearby. I'll leave the removal here
> unless you really hate the style cleanup.

My ugly and imprecise grepping tells me you're going to have a fun time
enforcing your sense of style, but good luck ;)

$ git grep -n -A 10 'of_device_id.*\[\] = ' | grep '{\s*}$' | wc -l
1020
$ git grep -n -A 10 'of_device_id.*\[\] = ' | grep '{\s*},$' | wc -l
1348

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-08-09 17:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-06 17:10 [PATCH] firmware: coreboot: Let OF core populate platform device Stephen Boyd
2018-08-06 17:22 ` Sudeep Holla
2018-08-06 17:38 ` Brian Norris
2018-08-06 18:05   ` Stephen Boyd
2018-08-06 18:29     ` Julius Werner
2018-08-07 21:41 ` Brian Norris
2018-08-08  6:00   ` Stephen Boyd
2018-08-09 17:13     ` Brian Norris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).