Messages in this thread | | | From | Stephen Boyd <> | Subject | Re: [PATCH] firmware: coreboot: Let OF core populate platform device | Date | Tue, 07 Aug 2018 23:00:50 -0700 |
| |
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); > >
| |