| Message ID | 1464816460-18750-3-git-send-email-john.stultz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Subject should indicate "mfd". `git log --oneline -- drivers/mfd` > In trying to wire up the powerkey driver, I found I > needed to add this to get the pmic logic to probe PMIC > child nodes in the dt data. DT. Please use full buffer width. Wrapping at 50 chars is not necessary. > With this patch, child nodes get properly probed. > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > Cc: Kumar Gala <galak@codeaurora.org> > Cc: Lee Jones <lee.jones@linaro.org> > Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> > Cc: Wei Xu <xuwei5@hisilicon.com> > Cc: Guodong Xu <guodong.xu@linaro.org> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > drivers/mfd/hi655x-pmic.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c > index 05ddc78..3511035 100644 > --- a/drivers/mfd/hi655x-pmic.c > +++ b/drivers/mfd/hi655x-pmic.c > @@ -39,6 +39,11 @@ static const struct regmap_irq hi655x_irqs[] = { > { .reg_offset = 0, .mask = RESERVE_INT }, > }; > > +static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = { > + { .compatible = "hisilicon,hi6552-powerkey", }, > + { /* end */ } > +}; > + > static const struct regmap_irq_chip hi655x_irq_chip = { > .name = "hi655x-pmic", > .irqs = hi655x_irqs, > @@ -122,6 +127,9 @@ static int hi655x_pmic_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, pmic); > > + /* populate sub nodes */ > + of_platform_populate(np, of_hi655x_pmic_child_match_tbl, NULL, dev); > + Oh, holey poop, no! Please don't mix DT and MFD registration like this. Drivers should contain either of_platform_populate() or mfd_add_devices(), but never both. > ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, hi655x_pmic_devs, > ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL); > if (ret) { -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 8, 2016 at 7:31 AM, Lee Jones <lee.jones@linaro.org> wrote: > > Please use full buffer width. Wrapping at 50 chars is not necessary. But then how would you be able to review patches on your phone?! :) >> --- a/drivers/mfd/hi655x-pmic.c >> +++ b/drivers/mfd/hi655x-pmic.c >> @@ -39,6 +39,11 @@ static const struct regmap_irq hi655x_irqs[] = { >> { .reg_offset = 0, .mask = RESERVE_INT }, >> }; >> >> +static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = { >> + { .compatible = "hisilicon,hi6552-powerkey", }, >> + { /* end */ } >> +}; >> + >> static const struct regmap_irq_chip hi655x_irq_chip = { >> .name = "hi655x-pmic", >> .irqs = hi655x_irqs, >> @@ -122,6 +127,9 @@ static int hi655x_pmic_probe(struct platform_device *pdev) >> >> platform_set_drvdata(pdev, pmic); >> >> + /* populate sub nodes */ >> + of_platform_populate(np, of_hi655x_pmic_child_match_tbl, NULL, dev); >> + > > Oh, holey poop, no! > > Please don't mix DT and MFD registration like this. > > Drivers should contain either of_platform_populate() or > mfd_add_devices(), but never both. > >> ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, hi655x_pmic_devs, >> ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL); >> if (ret) { So apologies for my ignorance here. I'm not quite sure I see what the right thing to do. I tried adding a "hi65xx-powerkey" entry in the hi655x_pmic_devs, but then mfd_add_devices doesn't seem to provide any of the irq info from the DT. The only way I can think to make it work w/o calling to populate sub-nodes is to pull the powerkey subnode out of the pmic node. It seems to work, but I'm not sure if it properly describes the hardware then. Sorry again for being a bit clueless here. Thanks for the review! -john
On Wed, 08 Jun 2016, John Stultz wrote: > On Wed, Jun 8, 2016 at 7:31 AM, Lee Jones <lee.jones@linaro.org> wrote: > > > > Please use full buffer width. Wrapping at 50 chars is not necessary. > > But then how would you be able to review patches on your phone?! :) Sideways! ;) > >> --- a/drivers/mfd/hi655x-pmic.c > >> +++ b/drivers/mfd/hi655x-pmic.c > >> @@ -39,6 +39,11 @@ static const struct regmap_irq hi655x_irqs[] = { > >> { .reg_offset = 0, .mask = RESERVE_INT }, > >> }; > >> > >> +static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = { > >> + { .compatible = "hisilicon,hi6552-powerkey", }, > >> + { /* end */ } > >> +}; > >> + > >> static const struct regmap_irq_chip hi655x_irq_chip = { > >> .name = "hi655x-pmic", > >> .irqs = hi655x_irqs, > >> @@ -122,6 +127,9 @@ static int hi655x_pmic_probe(struct platform_device *pdev) > >> > >> platform_set_drvdata(pdev, pmic); > >> > >> + /* populate sub nodes */ > >> + of_platform_populate(np, of_hi655x_pmic_child_match_tbl, NULL, dev); > >> + > > > > Oh, holey poop, no! > > > > Please don't mix DT and MFD registration like this. > > > > Drivers should contain either of_platform_populate() or > > mfd_add_devices(), but never both. > > > >> ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, hi655x_pmic_devs, > >> ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL); > >> if (ret) { > > > So apologies for my ignorance here. I'm not quite sure I see what the > right thing to do. I tried adding a "hi65xx-powerkey" entry in the > hi655x_pmic_devs, but then mfd_add_devices doesn't seem to provide any > of the irq info from the DT. > > The only way I can think to make it work w/o calling to populate > sub-nodes is to pull the powerkey subnode out of the pmic node. It > seems to work, but I'm not sure if it properly describes the hardware > then. Sorry again for being a bit clueless here. Since we use mfd_add_devices() already, you need to create a cell in this file for the power-key driver and omit it from DT altogether. If your priority is to have each sub-device represented in DT, then you need to stop using mfd_add_devices() and have of_platform_populate() do your bidding. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
diff --git a/drivers/mfd/hi655x-pmic.c b/drivers/mfd/hi655x-pmic.c index 05ddc78..3511035 100644 --- a/drivers/mfd/hi655x-pmic.c +++ b/drivers/mfd/hi655x-pmic.c @@ -39,6 +39,11 @@ static const struct regmap_irq hi655x_irqs[] = { { .reg_offset = 0, .mask = RESERVE_INT }, }; +static const struct of_device_id of_hi655x_pmic_child_match_tbl[] = { + { .compatible = "hisilicon,hi6552-powerkey", }, + { /* end */ } +}; + static const struct regmap_irq_chip hi655x_irq_chip = { .name = "hi655x-pmic", .irqs = hi655x_irqs, @@ -122,6 +127,9 @@ static int hi655x_pmic_probe(struct platform_device *pdev) platform_set_drvdata(pdev, pmic); + /* populate sub nodes */ + of_platform_populate(np, of_hi655x_pmic_child_match_tbl, NULL, dev); + ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO, hi655x_pmic_devs, ARRAY_SIZE(hi655x_pmic_devs), NULL, 0, NULL); if (ret) {
In trying to wire up the powerkey driver, I found I needed to add this to get the pmic logic to probe child nodes in the dt data. With this patch, child nodes get properly probed. Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> Cc: Kumar Gala <galak@codeaurora.org> Cc: Lee Jones <lee.jones@linaro.org> Cc: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> Cc: Wei Xu <xuwei5@hisilicon.com> Cc: Guodong Xu <guodong.xu@linaro.org> Signed-off-by: John Stultz <john.stultz@linaro.org> --- drivers/mfd/hi655x-pmic.c | 8 ++++++++ 1 file changed, 8 insertions(+) -- 1.9.1