Hi Dong and Xiubo, On Friday, September 19, 2014, Dong Aisheng wrote, > On Fri, Sep 19, 2014 at 01:20:18PM +0800, Xiubo Li-B47053 wrote: > > [...] > > > > create child: /dcsr@20000000/dcsr-atbrepl@3a8000 > > > > create child: /dcsr@20000000/dcsr-tsgen-ctrl@3a9000 > > > > create child: /dcsr@20000000/dcsr-tsgen-read@3aa000 > > > > create child: /regulators/regulator@0 > > > > ... > > > > > > > > As default the Linux will create all the platform device for each > > > > DT node, > > > which > > > > Can be found from "drivers/of/platform.c". > > > > > > > > So we can get the pdev node using the specified DT node, and feel > > > > safe to > > > use > > > > it as Pankaj's patch does. > > > > > > > > > > I mean before the devices are populated from device tree. > > > For example, we usually call of_platform_populate in .init_machine. > > > Before it, we may not be able to get it's device, isn't it? > > > > > > > Yes, right. > > > > For this case, we'd better create the pdev or dev manually for the > > first time We use it, right ? > > Yes, that's my understanding. >
Thanks for all your inputs. First let me clarify that the main purpose of this patch, we introduced this patch mainly because if some HW IP is having secondary compatibility as "syscon", syscon driver was getting probed and since there can be only on platform_device corresponding to one device_node, actual driver corresponding to that HW IP was not getting probed. So we wanted to decouple syscon from platform device, and any such driver should be able to become syscon provider. Please see discussion here [1]. [1]: https://lkml.org/lkml/2014/6/17/331 But while doing so eventually this patch was also able to cover the feature of early availability of syscon lookup APIs. As both of you pointed out that if we use "of_find_device_by_node" and it won't work for early users of syscon. I also agree with that, but it just skipped from my mind while suggesting that approach. I reconsidered again and made following changes and tested it. Now I am able to use syscon_lookup_by_xxxxx APIs, very early (before dt_machine_init) as well as after of_platform_populate. So please review and if it is acceptable I will post next version of this patch. Basic idea is, check for device_node pointer in of_syscon_register, if device corresponding to that device_node is already populated and available use "of_find_device_by_node" else create a dummy platform device and then use it. ---------- static struct syscon *of_syscon_register(struct device_node *np) { + struct platform_device *pdev = NULL; struct syscon *syscon; struct regmap *regmap; void __iomem *base; + struct platform_device dummy_pdev = { + .name = "dummy-syscon", + .id = -1, + }; + if (!of_device_is_compatible(np, "syscon")) return ERR_PTR(-EINVAL); @@ -141,8 +149,22 @@ static struct syscon *of_syscon_register(struct device_node *np) base = of_iomap(np, 0); if (!base) return ERR_PTR(-ENOMEM); + + if (!of_device_is_available(np) || + of_node_test_and_set_flag(np, OF_POPULATED)) { + /* if device is already populated and avaiable then use it */ + pdev = of_find_device_by_node(np); + if (!(&pdev->dev)) + return ERR_PTR(-ENODEV); + + regmap = regmap_init_mmio(&pdev->dev, base, &syscon_regmap_config); + } else { + /* for early users let's create dummy syscon device and use it */ + device_initialize(&dummy_pdev.dev); + dummy_pdev.dev.of_node = np; + regmap = regmap_init_mmio(&dummy_pdev.dev, base, &syscon_regmap_config); + } - regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config); if (IS_ERR(regmap)) { pr_err("regmap init failed\n"); return ERR_CAST(regmap); -- Thanks, Pankaj Dubey > Regards > Dong Aisheng > > > > > Thanks, > > > > BRs > > Xiubo > > > > > > > Regards > > > Dong Aisheng > > > > > > > And also we must make sure that the 'syscon' DT nodes has the > > > > compatible > > > prop. > > > > > > > > Thanks, > > > > > > > > BRs > > > > Xiubo > > > > > > > > > > > > > Regards > > > > > Dong Aisheng > > > > > > > > > > > ---- > > > > > > static struct syscon *of_syscon_register(struct device_node > > > > > > *np) { > > > > > > + struct platform_device *pdev; > > > > > > struct syscon *syscon; > > > > > > struct regmap *regmap; > > > > > > void __iomem *base; > > > > > > @@ -142,7 +144,11 @@ static struct syscon > > > > > > *of_syscon_register(struct device_node *np) > > > > > > if (!base) > > > > > > return ERR_PTR(-ENOMEM); > > > > > > > > > > > > - regmap = regmap_init_mmio(NULL, base, &syscon_regmap_config); > > > > > > + pdev = of_find_device_by_node(np); > > > > > > + if (!(&pdev->dev)) > > > > > > + return ERR_PTR(-ENODEV); > > > > > > + > > > > > > + regmap = regmap_init_mmio(&pdev->dev, base, > > > &syscon_regmap_config); > > > > > > if (IS_ERR(regmap)) { > > > > > > pr_err("regmap init failed\n"); > > > > > > return ERR_CAST(regmap); > > > > > > ------- > > > > > > > > > > > > I have tested this in linux-next and it works well. In this > > > > > > way there > > > won't > > > > > > be any issues of > > > > > > dereferencing NULL pointer in regmap.c and at the same time, > > > > > > if DT has {big,little}-endian optional property in syscon > > > > > > device node, it will be taken care. > > > > > > > > > > > > So I would wait for Arnd's opinion about above mentioned > > > > > > changes and > > > then > > > > > > post a new > > > > > > change after addressing Arnd's minor comment along with this > > > > > > fix in next revision. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > Pankaj Dubey > > > > > > > Maybe we could consider create device structure for each > > > > > > > syscon > > > compatible > > > > > > device in > > > > > > > syscon driver in of_syscon_register in first time which > > > > > > > seems to be > > > > > > reasonable. > > > > > > > > > > > > > > Regards > > > > > > > Dong Aisheng > > > > > > > > > > > > > > > -------------------------------------------- > > > > > > > > Subject: [PATCH] regmap: fix NULL pointer dereference in > > > > > > > > regmap_get_val_endian > > > > > > > > > > > > > > > > Recent commits for getting reg endianess causing NULL > > > > > > > > pointer dereference if dev is passed NULL in > > > > > > > > regmap_init_mmio. This patch fixes this issue, and allows > > > > > > > > to parse reg endianess only if dev and > > > > > > > > dev->of_node exist. > > > > > > > > > > > > > > > > Signed-off-by: Pankaj Dubey <pankaj.du...@samsung.com> > > > > > > > > --- > > > > > > > > drivers/base/regmap/regmap.c | 23 ++++++++++++++------- > -- > > > > > > > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/base/regmap/regmap.c > > > > > > > > b/drivers/base/regmap/regmap.c index f2281af..455a877 > > > > > > > > 100644 > > > > > > > > --- a/drivers/base/regmap/regmap.c > > > > > > > > +++ b/drivers/base/regmap/regmap.c > > > > > > > > @@ -477,7 +477,7 @@ static enum regmap_endian > > > > > > > > regmap_get_val_endian(struct device *dev, > > > > > > > > const struct regmap_bus *bus, > > > > > > > > const struct regmap_config > *config) > > > > > > { > > > > > > > > - struct device_node *np = dev->of_node; > > > > > > > > + struct device_node *np; > > > > > > > > enum regmap_endian endian; > > > > > > > > > > > > > > > > /* Retrieve the endianness specification from the regmap > > > > > > > > config > > > > > */ > > > > > > > > @@ -487,15 +487,20 @@ static enum regmap_endian > > > > > > > > regmap_get_val_endian(struct device *dev, > > > > > > > > if (endian != REGMAP_ENDIAN_DEFAULT) > > > > > > > > return endian; > > > > > > > > > > > > > > > > - /* Parse the device's DT node for an endianness specification > */ > > > > > > > > - if (of_property_read_bool(np, "big-endian")) > > > > > > > > - endian = REGMAP_ENDIAN_BIG; > > > > > > > > - else if (of_property_read_bool(np, "little-endian")) > > > > > > > > - endian = REGMAP_ENDIAN_LITTLE; > > > > > > > > + /* If the dev and dev->of_node exist try to get > > > > > > > > +endianness from > > > > > DT > > > > > > > > */ > > > > > > > > + if (dev && dev->of_node) { > > > > > > > > + np = dev->of_node; > > > > > > > > > > > > > > > > - /* If the endianness was specified in DT, use that */ > > > > > > > > - if (endian != REGMAP_ENDIAN_DEFAULT) > > > > > > > > - return endian; > > > > > > > > + /* Parse the device's DT node for an endianness > > > > > > > > specification */ > > > > > > > > + if (of_property_read_bool(np, "big-endian")) > > > > > > > > + endian = REGMAP_ENDIAN_BIG; > > > > > > > > + else if (of_property_read_bool(np, "little-endian")) > > > > > > > > + endian = REGMAP_ENDIAN_LITTLE; > > > > > > > > + > > > > > > > > + /* If the endianness was specified in DT, use that */ > > > > > > > > + if (endian != REGMAP_ENDIAN_DEFAULT) > > > > > > > > + return endian; > > > > > > > > + } > > > > > > > > > > > > > > > > /* Retrieve the endianness specification from the bus config */ > > > > > > > > if (bus && bus->val_format_endian_default) > > > > > > > > -- > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Pankaj Dubey > > > > > > > > > > > > > > > > > Regards > > > > > > > > > Dong Aisheng > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Pankaj Dubey > > > > > > > > > > > > > > > > > > > > > Regards > > > > > > > > > > > Dong Aisheng > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > > > > > > > linux-arm-kernel mailing list > > > > > > > > > > > > linux-arm-ker...@lists.infradead.org > > > > > > > > > > > > http://lists.infradead.org/mailman/listinfo/linux- > > > > > > > > > > > > arm-kernel > > > > > > > > > > > > > > > > > > > > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
