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

login
register
mail settings
Submitter Brian Masney
Date Jan. 7, 2019, 2:11 a.m.
Message ID <20190107021145.6370-3-masneyb@onstation.org>
Download mbox | patch
Permalink /patch/693531/
State New
Headers show

Comments

Brian Masney - Jan. 7, 2019, 2:11 a.m.
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(-)
Linus Walleij - Jan. 7, 2019, 10:53 a.m.
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
Brian Masney - Jan. 7, 2019, 11:30 a.m.
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
Stephen Boyd - Jan. 7, 2019, 9:41 p.m.
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.
Brian Masney - Jan. 8, 2019, 10:32 a.m.
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

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f461460a2aeb..ef27b3927295 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -960,6 +960,7 @@  config MFD_SPMI_PMIC
 	depends on ARCH_QCOM || COMPILE_TEST
 	depends on OF
 	depends on SPMI
+	select MFD_CORE
 	select REGMAP_SPMI
 	help
 	  This enables support for the Qualcomm SPMI PMICs.
diff --git a/drivers/mfd/qcom-spmi-pmic.c b/drivers/mfd/qcom-spmi-pmic.c
index e2e95de649a4..21aa880e3503 100644
--- a/drivers/mfd/qcom-spmi-pmic.c
+++ b/drivers/mfd/qcom-spmi-pmic.c
@@ -12,10 +12,10 @@ 
  */
 
 #include <linux/kernel.h>
+#include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/spmi.h>
 #include <linux/regmap.h>
-#include <linux/of_platform.h>
 
 #define PMIC_REV2		0x101
 #define PMIC_REV3		0x102
@@ -124,6 +124,61 @@  static const struct regmap_config spmi_regmap_config = {
 	.fast_io	= true,
 };
 
+static const struct mfd_cell pmic_spmi_cells[] = {
+	{
+		.name = "spmi-mpp",
+		.of_compatible = "qcom,spmi-mpp",
+	},
+	{
+		.name = "spmi-temp-alarm",
+		.of_compatible = "qcom,spmi-temp-alarm",
+	},
+	{
+		.name = "pm8941-rtc",
+		.of_compatible = "qcom,pm8941-rtc",
+	},
+	{
+		.name = "pm8941-pwrkey",
+		.of_compatible = "qcom,pm8941-pwrkey",
+	},
+	{
+		.name = "pm8941-misc",
+		.of_compatible = "qcom,pm8941-misc",
+	},
+	{
+		.name = "pm8941-charger",
+		.of_compatible = "qcom,pm8941-charger",
+	},
+	{
+		.name = "spmi-gpio",
+		.of_compatible = "qcom,spmi-gpio",
+	},
+	{
+		.name = "spmi-mpp",
+		.of_compatible = "qcom,spmi-mpp",
+	},
+	{
+		.name = "spmi-vadc",
+		.of_compatible = "qcom,spmi-vadc",
+	},
+	{
+		.name = "spmi-iadc",
+		.of_compatible = "qcom,spmi-iadc",
+	},
+	{
+		.name = "pm8941-coincell",
+		.of_compatible = "qcom,pm8941-coincell",
+	},
+	{
+		.name = "pm8941-wled",
+		.of_compatible = "qcom,pm8941-wled",
+	},
+	{
+		.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);
 }
 
 MODULE_DEVICE_TABLE(of, pmic_spmi_id_table);