Re: [PATCH] firmware: coreboot: Let OF core populate platform device

From: Stephen Boyd
Date: Wed Aug 08 2018 - 02:00:58 EST


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