| Message ID | 20160829182200.GD19826@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Stephen, 2016-08-30 3:22 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>: > On 08/29, Masahiro Yamada wrote: >> Hi Stephen, >> >> >> 2016-08-20 4:16 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>: >> >> >> >> >> + >> >> >> + parent = of_get_parent(dev->of_node); /* parent should be syscon node */ >> >> >> + regmap = syscon_node_to_regmap(parent); >> >> >> + of_node_put(parent); >> >> > >> >> > devm_get_regmap(dev->parent) should work then? Why do we need to >> >> > use OF APIs? >> >> >> >> "git grep devm_get_regmap" did not hit anything. >> >> >> >> Where is it defined? >> >> >> > >> > Sorry I meant dev_get_regmap(). >> > >> >> I tried this, but it did not work. >> >> To make dev_get_regmap() work, >> the parent device needs to call dev_regmap_init_mmio() beforehand. >> >> >> Since commit bdb0066df96e74a4002125467ebe459feff1ebef >> (mfd: syscon: Decouple syscon interface from platform devices), >> syscon_probe() is not called for platform devices, >> so that never happens. >> > > Ok. Is the syscon also a simple-mfd? > > It sounds like there's a device for the parent, but we've failed > to attach a regmap to it. Maybe the core DT code should assign > the regmap to the parent device when it creates it so that child > devices don't need to know this detail? It could look for > simple-mfd devices with compatible = "syscon" and then create the > regmap? Here's a totally untested patch for that. > > ----8<---- > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > index 2f2225e845ef..5f7d3f015b82 100644 > --- a/drivers/mfd/syscon.c > +++ b/drivers/mfd/syscon.c > @@ -136,6 +136,17 @@ struct regmap *syscon_node_to_regmap(struct device_node *np) > } > EXPORT_SYMBOL_GPL(syscon_node_to_regmap); > > +int device_attach_syscon(struct device *dev) > +{ > + struct regmap *regmap; > + > + regmap = syscon_node_to_regmap(dev->of_node); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + return regmap_attach_dev(dev, regmap, &syscon_regmap_config); > +} > + > struct regmap *syscon_regmap_lookup_by_compatible(const char *s) > { > struct device_node *syscon_np; > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 8aa197691074..58a018e15006 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -25,6 +25,7 @@ > #include <linux/of_irq.h> > #include <linux/of_platform.h> > #include <linux/platform_device.h> > +#include <linux/mfd/syscon.h> > > const struct of_device_id of_default_bus_match_table[] = { > { .compatible = "simple-bus", }, > @@ -383,7 +384,12 @@ static int of_platform_bus_create(struct device_node *bus, > return 0; > } > > + > dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent); > + if (of_device_is_compatible(bus, "simple-mfd") && > + of_device_is_compatible(bus, "syscon")) > + device_attach_syscon(&dev->dev); > + > if (!dev || !of_match_node(matches, bus)) > return 0; > > diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h > index 40a76b97b7ab..e19e5d15f4e6 100644 > --- a/include/linux/mfd/syscon.h > +++ b/include/linux/mfd/syscon.h > @@ -18,9 +18,11 @@ > #include <linux/err.h> > #include <linux/errno.h> > > +struct device; > struct device_node; > > #ifdef CONFIG_MFD_SYSCON > +extern int device_attach_syscon(struct device *dev); > extern struct regmap *syscon_node_to_regmap(struct device_node *np); > extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s); > extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s); > @@ -28,6 +30,11 @@ extern struct regmap *syscon_regmap_lookup_by_phandle( > struct device_node *np, > const char *property); > #else > +static inline int device_attach_syscon(struct device *dev) > +{ > + return 0; > +} > + > static inline struct regmap *syscon_node_to_regmap(struct device_node *np) > { > return ERR_PTR(-ENOTSUPP); I was not quire sure about this, but maybe worth submitting to DT subsystem? Anyway, I will go with syscon_node_to_regmap() for v7. Of course, I am happy to replace it with dev_get_regmap() when the issue is solved in the mainline.
On 09/05, Masahiro Yamada wrote: > 2016-08-30 3:22 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>: > > On 08/29, Masahiro Yamada wrote: > >> I tried this, but it did not work. > >> > >> To make dev_get_regmap() work, > >> the parent device needs to call dev_regmap_init_mmio() beforehand. > >> > >> > >> Since commit bdb0066df96e74a4002125467ebe459feff1ebef > >> (mfd: syscon: Decouple syscon interface from platform devices), > >> syscon_probe() is not called for platform devices, > >> so that never happens. > >> > > > > Ok. Is the syscon also a simple-mfd? > > > > It sounds like there's a device for the parent, but we've failed > > to attach a regmap to it. Maybe the core DT code should assign > > the regmap to the parent device when it creates it so that child > > devices don't need to know this detail? It could look for > > simple-mfd devices with compatible = "syscon" and then create the > > regmap? Here's a totally untested patch for that. > > > > > I was not quire sure about this, > but maybe worth submitting to DT subsystem? > > Anyway, I will go with syscon_node_to_regmap() for v7. > Of course, I am happy to replace it with dev_get_regmap() > when the issue is solved in the mainline. > Ok that's fine. Did my patch fix dev_get_regmap() for you though? That would be useful to know so that this patch can be merged in parallel to yours.
Hi Stephen, 2016-09-07 9:32 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>: > On 09/05, Masahiro Yamada wrote: >> 2016-08-30 3:22 GMT+09:00 Stephen Boyd <sboyd@codeaurora.org>: >> > On 08/29, Masahiro Yamada wrote: >> >> I tried this, but it did not work. >> >> >> >> To make dev_get_regmap() work, >> >> the parent device needs to call dev_regmap_init_mmio() beforehand. >> >> >> >> >> >> Since commit bdb0066df96e74a4002125467ebe459feff1ebef >> >> (mfd: syscon: Decouple syscon interface from platform devices), >> >> syscon_probe() is not called for platform devices, >> >> so that never happens. >> >> >> > >> > Ok. Is the syscon also a simple-mfd? >> > >> > It sounds like there's a device for the parent, but we've failed >> > to attach a regmap to it. Maybe the core DT code should assign >> > the regmap to the parent device when it creates it so that child >> > devices don't need to know this detail? It could look for >> > simple-mfd devices with compatible = "syscon" and then create the >> > regmap? Here's a totally untested patch for that. >> > >> >> >> I was not quire sure about this, >> but maybe worth submitting to DT subsystem? >> >> Anyway, I will go with syscon_node_to_regmap() for v7. >> Of course, I am happy to replace it with dev_get_regmap() >> when the issue is solved in the mainline. >> > > Ok that's fine. Did my patch fix dev_get_regmap() for you though? > That would be useful to know so that this patch can be merged in > parallel to yours. Sorry for my late reply. Yup, your patch worked fine! And, I've posted v7 for my SoC clk driver.
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index 2f2225e845ef..5f7d3f015b82 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -136,6 +136,17 @@ struct regmap *syscon_node_to_regmap(struct device_node *np) } EXPORT_SYMBOL_GPL(syscon_node_to_regmap); +int device_attach_syscon(struct device *dev) +{ + struct regmap *regmap; + + regmap = syscon_node_to_regmap(dev->of_node); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + return regmap_attach_dev(dev, regmap, &syscon_regmap_config); +} + struct regmap *syscon_regmap_lookup_by_compatible(const char *s) { struct device_node *syscon_np; diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 8aa197691074..58a018e15006 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -25,6 +25,7 @@ #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/mfd/syscon.h> const struct of_device_id of_default_bus_match_table[] = { { .compatible = "simple-bus", }, @@ -383,7 +384,12 @@ static int of_platform_bus_create(struct device_node *bus, return 0; } + dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent); + if (of_device_is_compatible(bus, "simple-mfd") && + of_device_is_compatible(bus, "syscon")) + device_attach_syscon(&dev->dev); + if (!dev || !of_match_node(matches, bus)) return 0; diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h index 40a76b97b7ab..e19e5d15f4e6 100644 --- a/include/linux/mfd/syscon.h +++ b/include/linux/mfd/syscon.h @@ -18,9 +18,11 @@ #include <linux/err.h> #include <linux/errno.h> +struct device; struct device_node; #ifdef CONFIG_MFD_SYSCON +extern int device_attach_syscon(struct device *dev); extern struct regmap *syscon_node_to_regmap(struct device_node *np); extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s); extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s); @@ -28,6 +30,11 @@ extern struct regmap *syscon_regmap_lookup_by_phandle( struct device_node *np, const char *property); #else +static inline int device_attach_syscon(struct device *dev) +{ + return 0; +} + static inline struct regmap *syscon_node_to_regmap(struct device_node *np) { return ERR_PTR(-ENOTSUPP);