[v2,2/5] mfd: qcom-spmi-pmic: use devm_mfd_add_devices instead of devm_of_platform_populate

pmic_spmi_probe calls devm_of_platform_populate, which traverses all
of the children in device tree from the parent down to the children,
grandchildren, etc. of_irq_count is called on most of the nodes (via
of_device_alloc) and this initializes all of the IRQs. Further along in
the boot process, spmi-gpio is initialized as a hierarchical IRQ chip
in a later patch (with spmi-arb as the parent IRQ domain), and the same
hwirq is now associated with two Linux virqs and IRQs will not work as
expected. Correct this issue by using devm_mfd_add_devices to initialize
just the children so that IRQs are initialized on an as-needed basis.

This patch also selects CONFIG_MFD_CORE since this is required by
devm_mfd_add_devices.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/mfd/Kconfig          |  1 +
 drivers/mfd/qcom-spmi-pmic.c | 61 ++++++++++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 2 deletions(-)

On Mon, Jan 7, 2019 at 3:11 AM Brian Masney <masneyb@onstation.org> wrote:

> pmic_spmi_probe calls devm_of_platform_populate, which traverses all
> of the children in device tree from the parent down to the children,
> grandchildren, etc. of_irq_count is called on most of the nodes (via
> of_device_alloc) and this initializes all of the IRQs. Further along in
> the boot process, spmi-gpio is initialized as a hierarchical IRQ chip
> in a later patch (with spmi-arb as the parent IRQ domain), and the same
> hwirq is now associated with two Linux virqs and IRQs will not work as
> expected. Correct this issue by using devm_mfd_add_devices to initialize
> just the children so that IRQs are initialized on an as-needed basis.
>
> This patch also selects CONFIG_MFD_CORE since this is required by
> devm_mfd_add_devices.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>

Good catch! Now I see why this was acting so weird for you.
Also the MFD semantics are standardized and make much more
sense after this.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I suppose Lee can merge this in orthogonal in the MFD tree,
the end result will be functional after the v5.1 merge window.

I think I should also fix qcom-pm8xxx after this, as well as
the similar SSBI code for elder platforms.

Yours,
Linus Walleij

On Mon, Jan 07, 2019 at 11:53:07AM +0100, Linus Walleij wrote:
> I suppose Lee can merge this in orthogonal in the MFD tree,
> the end result will be functional after the v5.1 merge window.

To keep git bisect happy, it would be nice if either all of the patches
go in through the same tree (with the proper ACKs), or if the first four
patches are applied during the v5.1 merge window, and the last patch is
applied during the v5.2 merge window.

Brian

Quoting Brian Masney (2019-01-06 18:11:42)
> +       },
> +       {
> +               .name = "pm8941-regulators",
> +               .of_compatible = "qcom,pm8941-regulators",
> +       },
> +};
> +
>  static int pmic_spmi_probe(struct spmi_device *sdev)
>  {
>         struct regmap *regmap;
> @@ -136,7 +191,9 @@ static int pmic_spmi_probe(struct spmi_device *sdev)
>         if (sdev->usid % 2 == 0)
>                 pmic_spmi_show_revid(regmap, &sdev->dev);
>  
> -       return devm_of_platform_populate(&sdev->dev);
> +       return devm_mfd_add_devices(&sdev->dev, PLATFORM_DEVID_AUTO,
> +                                   pmic_spmi_cells,
> +                                   ARRAY_SIZE(pmic_spmi_cells), NULL, 0, NULL);

Now this seems worse. We have gotten by without having to explicitly
list all the devices that are inside the PMIC as mfd cells. But now, to
avoid creating the irqs before the hierarchy is installed, we have to
undo all of that and rely on the difference in behavior of
of_platform_populate() and mfd_add_devices(). That's pretty obscure to
figure out.

I'd prefer we drop this patch and keep disassociating virqs and
reassociating them in the gpio driver. Then we can remove the interrupts
properties in all the DTS files and finally remove the disassociate and
reassociating code in the gpio driver when all the DT files are cleaned
up. It makes things less confusing that way and doesn't require updates
to this driver.

On Mon, Jan 07, 2019 at 01:41:30PM -0800, Stephen Boyd wrote:
> Now this seems worse. We have gotten by without having to explicitly
> list all the devices that are inside the PMIC as mfd cells. But now, to
> avoid creating the irqs before the hierarchy is installed, we have to
> undo all of that and rely on the difference in behavior of
> of_platform_populate() and mfd_add_devices(). That's pretty obscure to
> figure out.
> 
> I'd prefer we drop this patch and keep disassociating virqs and
> reassociating them in the gpio driver. Then we can remove the interrupts
> properties in all the DTS files and finally remove the disassociate and
> reassociating code in the gpio driver when all the DT files are cleaned
> up. It makes things less confusing that way and doesn't require updates
> to this driver.

You are right that we can get this working without this patch. The issue
that I experienced was caused by the interrupts property on the
spmi-gpio node. I thought that I tested this with that configuration but
I obviously didn't.

qcom-pm8941.dtsi and qcom-pma8084.dtsi are the only two in-tree users of
spmi-gpio. I'll include the fix for qcom-pma8084.dtsi as well.

Brian