From: Josh Cartwright <[email protected]>
The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
800 series SoC family. This driver exists largely as a glue mfd component,
it exists to be an owner of an SPMI regmap for children devices
described in device tree.
Signed-off-by: Josh Cartwright <[email protected]>
Signed-off-by: Courtney Cavin <[email protected]>
---
drivers/mfd/Kconfig | 13 +++++++++++
drivers/mfd/Makefile | 1 +
drivers/mfd/pm8x41.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 77 insertions(+)
create mode 100644 drivers/mfd/pm8x41.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3383412..f5ff799 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -502,6 +502,19 @@ config MFD_PM8921_CORE
Say M here if you want to include support for PM8921 chip as a module.
This will build a module called "pm8921-core".
+config MFD_PM8X41
+ bool "Qualcomm PM8x41 PMIC"
+ depends on ARCH_QCOM
+ select REGMAP_SPMI
+ help
+ This enables basic support for the Qualcomm 8941 and 8841 PMICs.
+ These PMICs are currently used with the Snapdragon 800 series of
+ SoCs. Note, that this will only be useful paired with descriptions
+ of the independent functions as children nodes in the device tree.
+
+ Say M here if you want to include support for the PM8x41 series as a
+ module. The module will be called "pm8x41".
+
config MFD_RDC321X
tristate "RDC R-321x southbridge"
select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 2851275..f0df41d 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o
obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o
+obj-$(CONFIG_MFD_PM8X41) += pm8x41.o
obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
obj-$(CONFIG_MFD_TPS65090) += tps65090.o
obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c
new file mode 100644
index 0000000..c85e0d6
--- /dev/null
+++ b/drivers/mfd/pm8x41.c
@@ -0,0 +1,63 @@
+/* Copyright (c) 2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spmi.h>
+#include <linux/regmap.h>
+#include <linux/of_platform.h>
+
+static const struct regmap_config pm8x41_regmap_config = {
+ .reg_bits = 16,
+ .val_bits = 8,
+ .max_register = 0xFFFF,
+};
+
+static int pm8x41_remove_child(struct device *dev, void *unused)
+{
+ platform_device_unregister(to_platform_device(dev));
+ return 0;
+}
+
+static void pm8x41_remove(struct spmi_device *sdev)
+{
+ device_for_each_child(&sdev->dev, NULL, pm8x41_remove_child);
+}
+
+static int pm8x41_probe(struct spmi_device *sdev)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_dbg(&sdev->dev, "regmap creation failed.\n");
+ return PTR_ERR(regmap);
+ }
+
+ return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
+}
+
+static const struct of_device_id pm8x41_id_table[] = {
+ { .compatible = "qcom,pm8841", },
+ { .compatible = "qcom,pm8941", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, pm8x41_id_table);
+
+static struct spmi_driver pm8x41_driver = {
+ .probe = pm8x41_probe,
+ .remove = pm8x41_remove,
+ .driver = {
+ .name = "pm8x41",
+ .of_match_table = pm8x41_id_table,
+ },
+};
+module_spmi_driver(pm8x41_driver);
--
1.8.1.5
From: Josh Cartwright <[email protected]>
Document the bindings used to describe the Qualcomm 8x41 PMICs.
Signed-off-by: Josh Cartwright <[email protected]>
Signed-off-by: Courtney Cavin <[email protected]>
---
Documentation/devicetree/bindings/mfd/pm8x41.txt | 34 ++++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/pm8x41.txt
diff --git a/Documentation/devicetree/bindings/mfd/pm8x41.txt b/Documentation/devicetree/bindings/mfd/pm8x41.txt
new file mode 100644
index 0000000..b865a4f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/pm8x41.txt
@@ -0,0 +1,34 @@
+Qualcomm PM8841 and PM8941 PMIC multi-function device bindings
+
+The PM8x41 PMICs are used with the Qualcomm Snapdragon 800 series SoCs, and are
+interfaced to the chip via the SPMI (System Power Management Interface) bus.
+Support for multiple independent functions are implemented by splitting the
+16-bit SPMI slave address space into 256 smaller fixed-size regions, 256 bytes
+each. A function can consume one or more of these fixed-size register regions.
+
+Required properties:
+- compatible: Must be one of:
+ "qcom,pm8841"
+ "qcom,pm8941"
+- reg: Specifies the SPMI USID slave address for this device;
+ See bindings/spmi/spmi.txt
+- #address-cells = <1>
+- #size-cells = <0>
+
+Each child node represents a function of the PM8x41. Each child 'reg' entry
+describes an offset within the USID slave address where the region starts.
+
+Example:
+
+pm8941@0 {
+ compatible = "qcom,pm8941";
+ reg = <0x0 SPMI_USID>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rtc@6000 {
+ compatible = "...";
+ reg = <0x6000 0x6100>;
+ };
+};
--
1.8.1.5
> From: Josh Cartwright <[email protected]>
>
> The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> 800 series SoC family. This driver exists largely as a glue mfd component,
> it exists to be an owner of an SPMI regmap for children devices
> described in device tree.
>
> Signed-off-by: Josh Cartwright <[email protected]>
> Signed-off-by: Courtney Cavin <[email protected]>
> ---
> drivers/mfd/Kconfig | 13 +++++++++++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/pm8x41.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 77 insertions(+)
> create mode 100644 drivers/mfd/pm8x41.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3383412..f5ff799 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -502,6 +502,19 @@ config MFD_PM8921_CORE
> Say M here if you want to include support for PM8921 chip as a module.
> This will build a module called "pm8921-core".
>
> +config MFD_PM8X41
> + bool "Qualcomm PM8x41 PMIC"
> + depends on ARCH_QCOM
depends on OF?
> + select REGMAP_SPMI
> + help
> + This enables basic support for the Qualcomm 8941 and 8841 PMICs.
> + These PMICs are currently used with the Snapdragon 800 series of
> + SoCs. Note, that this will only be useful paired with descriptions
> + of the independent functions as children nodes in the device tree.
> +
> + Say M here if you want to include support for the PM8x41 series as a
> + module. The module will be called "pm8x41".
> +
> config MFD_RDC321X
> tristate "RDC R-321x southbridge"
> select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 2851275..f0df41d 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o
> obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
> obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
> obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o
> +obj-$(CONFIG_MFD_PM8X41) += pm8x41.o
> obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
> obj-$(CONFIG_MFD_TPS65090) += tps65090.o
> obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
> diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c
> new file mode 100644
> index 0000000..c85e0d6
> --- /dev/null
> +++ b/drivers/mfd/pm8x41.c
> @@ -0,0 +1,63 @@
> +/* Copyright (c) 2013, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spmi.h>
> +#include <linux/regmap.h>
> +#include <linux/of_platform.h>
> +
> +static const struct regmap_config pm8x41_regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 8,
> + .max_register = 0xFFFF,
I've never seen this many registers registered before.
Are you sure you want to regmap the entire bank?
> +};
> +
> +static int pm8x41_remove_child(struct device *dev, void *unused)
> +{
> + platform_device_unregister(to_platform_device(dev));
> + return 0;
> +}
> +
> +static void pm8x41_remove(struct spmi_device *sdev)
> +{
> + device_for_each_child(&sdev->dev, NULL, pm8x41_remove_child);
> +}
Nit: It's strange to see the .remove above the .probe.
> +static int pm8x41_probe(struct spmi_device *sdev)
> +{
> + struct regmap *regmap;
> +
> + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_dbg(&sdev->dev, "regmap creation failed.\n");
This is not debug, it's an error.
> + return PTR_ERR(regmap);
> + }
> +
> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> +}
> +
> +static const struct of_device_id pm8x41_id_table[] = {
> + { .compatible = "qcom,pm8841", },
> + { .compatible = "qcom,pm8941", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, pm8x41_id_table);
> +
> +static struct spmi_driver pm8x41_driver = {
> + .probe = pm8x41_probe,
> + .remove = pm8x41_remove,
> + .driver = {
> + .name = "pm8x41",
> + .of_match_table = pm8x41_id_table,
of_match_ptr()
> + },
> +};
> +module_spmi_driver(pm8x41_driver);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi,
On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote:
> From: Josh Cartwright <[email protected]>
>
> The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> 800 series SoC family. This driver exists largely as a glue mfd component,
> it exists to be an owner of an SPMI regmap for children devices
> described in device tree.
>
Thanks. This is exactly what I have planed to do :-)
> Signed-off-by: Josh Cartwright <[email protected]>
> Signed-off-by: Courtney Cavin <[email protected]>
> ---
> drivers/mfd/Kconfig | 13 +++++++++++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/pm8x41.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 77 insertions(+)
> create mode 100644 drivers/mfd/pm8x41.c
>
<snip>
> +
> +static int pm8x41_probe(struct spmi_device *sdev)
> +{
> + struct regmap *regmap;
> +
> + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_dbg(&sdev->dev, "regmap creation failed.\n");
> + return PTR_ERR(regmap);
> + }
> +
> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
I think that this will not going to work. For example in this particular
case, both controllers have "qcom,qpnp-revid" peripheral which is
located at offset 0x100.
And the result is:
[ 0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid'
DT looks like this:
spmi {
compatible = "qcom,spmi-pmic-arb";
reg-names = "core", "intr", "cnfg";
reg = <0xfc4cf000 0x1000>,
<0xfc4cb000 0x1000>,
<0xfc4ca000 0x1000>;
interrupt-names = "periph_irq";
interrupts = <0 190 0>;
qcom,ee = <0>;
qcom,channel = <0>;
#address-cells = <2>;
#size-cells = <0>;
interrupt-controller;
#interrupt-cells = <4>;
pm8941@0 {
compatible = "qcom,pm8941";
reg = <0x0 SPMI_USID>;
#address-cells = <1>;
#size-cells = <0>;
revid@100 {
compatible = "qcom,qpnp-revid";
reg = <0x100 0x100>;
};
};
pm8841@4 {
compatible = "qcom,pm8941";
reg = <0x4 SPMI_USID>;
#address-cells = <1>;
#size-cells = <0>;
revid@100 {
compatible = "qcom,qpnp-revid";
reg = <0x100 0x100>;
};
};
};
Any suggestions?
Thanks,
Ivan
On Wed, Apr 23, 2014 at 12:50:55PM +0200, Lee Jones wrote:
> > From: Josh Cartwright <[email protected]>
> >
> > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> > 800 series SoC family. This driver exists largely as a glue mfd component,
> > it exists to be an owner of an SPMI regmap for children devices
> > described in device tree.
> >
> > Signed-off-by: Josh Cartwright <[email protected]>
> > Signed-off-by: Courtney Cavin <[email protected]>
> > ---
> > drivers/mfd/Kconfig | 13 +++++++++++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/pm8x41.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 77 insertions(+)
> > create mode 100644 drivers/mfd/pm8x41.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 3383412..f5ff799 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -502,6 +502,19 @@ config MFD_PM8921_CORE
> > Say M here if you want to include support for PM8921 chip as a module.
> > This will build a module called "pm8921-core".
> >
> > +config MFD_PM8X41
> > + bool "Qualcomm PM8x41 PMIC"
Hrm. This should be tristate.
> > + depends on ARCH_QCOM
>
> depends on OF?
Yea, that's probably best.
> > + select REGMAP_SPMI
> > + help
> > + This enables basic support for the Qualcomm 8941 and 8841 PMICs.
> > + These PMICs are currently used with the Snapdragon 800 series of
> > + SoCs. Note, that this will only be useful paired with descriptions
> > + of the independent functions as children nodes in the device tree.
> > +
> > + Say M here if you want to include support for the PM8x41 series as a
> > + module. The module will be called "pm8x41".
> > +
> > config MFD_RDC321X
> > tristate "RDC R-321x southbridge"
> > select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 2851275..f0df41d 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o
> > obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
> > obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
> > obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o
> > +obj-$(CONFIG_MFD_PM8X41) += pm8x41.o
> > obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
> > obj-$(CONFIG_MFD_TPS65090) += tps65090.o
> > obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
> > diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c
> > new file mode 100644
> > index 0000000..c85e0d6
> > --- /dev/null
> > +++ b/drivers/mfd/pm8x41.c
> > @@ -0,0 +1,63 @@
> > +/* Copyright (c) 2013, The Linux Foundation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/spmi.h>
> > +#include <linux/regmap.h>
> > +#include <linux/of_platform.h>
> > +
> > +static const struct regmap_config pm8x41_regmap_config = {
> > + .reg_bits = 16,
> > + .val_bits = 8,
> > + .max_register = 0xFFFF,
>
> I've never seen this many registers registered before.
>
> Are you sure you want to regmap the entire bank?
>
Quite sure. The pm8941 has blocks at each 0x100, up to 0xE700.
Additionally, from what I've heard, there are some undocumented (for me
at least) blocks higher than that.
> > +};
> > +
> > +static int pm8x41_remove_child(struct device *dev, void *unused)
> > +{
> > + platform_device_unregister(to_platform_device(dev));
> > + return 0;
> > +}
> > +
> > +static void pm8x41_remove(struct spmi_device *sdev)
> > +{
> > + device_for_each_child(&sdev->dev, NULL, pm8x41_remove_child);
> > +}
>
> Nit: It's strange to see the .remove above the .probe.
>
Really? Alright, I can move it.
> > +static int pm8x41_probe(struct spmi_device *sdev)
> > +{
> > + struct regmap *regmap;
> > +
> > + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
> > + if (IS_ERR(regmap)) {
> > + dev_dbg(&sdev->dev, "regmap creation failed.\n");
>
> This is not debug, it's an error.
>
Indeed.
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> > +}
> > +
> > +static const struct of_device_id pm8x41_id_table[] = {
> > + { .compatible = "qcom,pm8841", },
> > + { .compatible = "qcom,pm8941", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, pm8x41_id_table);
> > +
> > +static struct spmi_driver pm8x41_driver = {
> > + .probe = pm8x41_probe,
> > + .remove = pm8x41_remove,
> > + .driver = {
> > + .name = "pm8x41",
> > + .of_match_table = pm8x41_id_table,
>
> of_match_ptr()
>
Ok.
> > + },
> > +};
> > +module_spmi_driver(pm8x41_driver);
Thanks for the review.
-Courtney
On Wed, Apr 23, 2014 at 03:19:28PM +0200, Ivan T. Ivanov wrote:
>
> Hi,
>
> On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote:
> > From: Josh Cartwright <[email protected]>
> >
> > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> > 800 series SoC family. This driver exists largely as a glue mfd component,
> > it exists to be an owner of an SPMI regmap for children devices
> > described in device tree.
> >
>
> Thanks. This is exactly what I have planed to do :-)
>
Sorry if I usurped your work!
> > Signed-off-by: Josh Cartwright <[email protected]>
> > Signed-off-by: Courtney Cavin <[email protected]>
> > ---
> > drivers/mfd/Kconfig | 13 +++++++++++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/pm8x41.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 77 insertions(+)
> > create mode 100644 drivers/mfd/pm8x41.c
> >
>
> <snip>
>
> > +
> > +static int pm8x41_probe(struct spmi_device *sdev)
> > +{
> > + struct regmap *regmap;
> > +
> > + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
> > + if (IS_ERR(regmap)) {
> > + dev_dbg(&sdev->dev, "regmap creation failed.\n");
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
>
> I think that this will not going to work. For example in this particular
> case, both controllers have "qcom,qpnp-revid" peripheral which is
> located at offset 0x100.
>
> And the result is:
>
> [ 0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid'
>
[...]
>
> Any suggestions?
That's expected behavior actually. You have two nodes in DT named the
same thing and at the same address. This error is due to the fact that
all devices are put in '/bus/platform/devices/' with a name made from
the unit address and name specified in DT. There's no other unique
information used to differentiate the devices.
If you simply change the names in DT, it works. Example follows:
qcom,pm8941@0 {
compatible = "qcom,pm8941";
reg = <0x0 SPMI_USID>;
#address-cells = <1>;
#size-cells = <0>;
qcom,pm8941-revid@100 {
compatible = "qcom,qpnp-revid";
reg = <0x100>;
};
};
qcom,pm8841@4 {
compatible = "qcom,pm8841";
reg = <0x4 SPMI_USID>;
#address-cells = <1>;
#size-cells = <0>;
qcom,pm8841-revid@100 {
compatible = "qcom,qpnp-revid";
reg = <0x100>;
};
};
[...] qcom,qpnp-revid 100.qcom,pm8841-revid: PM8841 v2.0 options: 0, 0, 2, 2
[...] qcom,qpnp-revid 100.qcom,pm8941-revid: PM8941 v3.0 options: 2, 0, 0, 0
Whether this should be "fixed" in the device/bus/sysfs core, I don't
know, but it isn't specifically an issue with this driver, and there's
little-to-nothing I can do to fix it here.
-Courtney
On Wed, 2014-04-23 at 11:16 -0700, Courtney Cavin wrote:
> On Wed, Apr 23, 2014 at 03:19:28PM +0200, Ivan T. Ivanov wrote:
> >
> > Hi,
> >
> > On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote:
> > > From: Josh Cartwright <[email protected]>
> > >
> > > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> > > 800 series SoC family. This driver exists largely as a glue mfd component,
> > > it exists to be an owner of an SPMI regmap for children devices
> > > described in device tree.
> > >
> >
> > Thanks. This is exactly what I have planed to do :-)
> >
>
> Sorry if I usurped your work!
Noting to worry. I just was surprised how close it is to my vision ;-).
>
> > > Signed-off-by: Josh Cartwright <[email protected]>
> > > Signed-off-by: Courtney Cavin <[email protected]>
> > > ---
> > > drivers/mfd/Kconfig | 13 +++++++++++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/pm8x41.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 77 insertions(+)
> > > create mode 100644 drivers/mfd/pm8x41.c
> > >
> >
> > <snip>
> >
> > > +
> > > +static int pm8x41_probe(struct spmi_device *sdev)
> > > +{
> > > + struct regmap *regmap;
> > > +
> > > + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
> > > + if (IS_ERR(regmap)) {
> > > + dev_dbg(&sdev->dev, "regmap creation failed.\n");
> > > + return PTR_ERR(regmap);
> > > + }
> > > +
> > > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> >
> > I think that this will not going to work. For example in this particular
> > case, both controllers have "qcom,qpnp-revid" peripheral which is
> > located at offset 0x100.
> >
> > And the result is:
> >
> > [ 0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid'
> >
> [...]
> >
> > Any suggestions?
>
> That's expected behavior actually. You have two nodes in DT named the
> same thing and at the same address. This error is due to the fact that
> all devices are put in '/bus/platform/devices/' with a name made from
> the unit address and name specified in DT. There's no other unique
> information used to differentiate the devices.
>
> If you simply change the names in DT, it works.
Sure, it will work. But they are part of different address spaces.
Why we should add, IMHO, artificial requirement that names should
be unique? Is it possible to prefix child nodes with parent device
address? As side note, why they should be registered on the platform
bus at all? To be honest I don't have solution.
Regards,
Ivan
> [...] qcom,qpnp-revid 100.qcom,pm8841-revid: PM8841 v2.0 options: 0, 0, 2, 2
> [...] qcom,qpnp-revid 100.qcom,pm8941-revid: PM8941 v3.0 options: 2, 0, 0, 0
>
> Whether this should be "fixed" in the device/bus/sysfs core, I don't
> know, but it isn't specifically an issue with this driver, and there's
> little-to-nothing I can do to fix it here.
>
> -Courtney
On Tue, Apr 22, 2014 at 05:31:49PM -0700, Courtney Cavin wrote:
> From: Josh Cartwright <[email protected]>
>
> The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> 800 series SoC family. This driver exists largely as a glue mfd component,
> it exists to be an owner of an SPMI regmap for children devices
> described in device tree.
>
> Signed-off-by: Josh Cartwright <[email protected]>
> Signed-off-by: Courtney Cavin <[email protected]>
Hey Courtney-
Thanks for picking this up!
One thing that I had meant to do is rename this thing. Nothing about
this is PM8841/PM8941 specific at all. It should apply equally to all
Qualcomm's PMICs which implement QPNP.
Perhaps a better name would be "qcom-pmic-qpnp".
[..]
> +++ b/drivers/mfd/pm8x41.c
> @@ -0,0 +1,63 @@
> +/* Copyright (c) 2013, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spmi.h>
> +#include <linux/regmap.h>
> +#include <linux/of_platform.h>
> +
> +static const struct regmap_config pm8x41_regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 8,
> + .max_register = 0xFFFF,
> +};
This reminds me. David Collins (CC'd) noticed that there are usecases
where peripheral drivers will need to be accessing registers from atomic
context, so we should probably be setting .fast_io in the SPMI
regmap_bus structures, but we can tackle that when we get there.
> +
> +static int pm8x41_remove_child(struct device *dev, void *unused)
> +{
> + platform_device_unregister(to_platform_device(dev));
> + return 0;
> +}
> +
> +static void pm8x41_remove(struct spmi_device *sdev)
> +{
> + device_for_each_child(&sdev->dev, NULL, pm8x41_remove_child);
> +}
> +
> +static int pm8x41_probe(struct spmi_device *sdev)
> +{
> + struct regmap *regmap;
> +
> + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_dbg(&sdev->dev, "regmap creation failed.\n");
> + return PTR_ERR(regmap);
> + }
> +
> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> +}
> +
> +static const struct of_device_id pm8x41_id_table[] = {
> + { .compatible = "qcom,pm8841", },
> + { .compatible = "qcom,pm8941", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, pm8x41_id_table);
I'm thinking we should probably have a generic compatible entry as well,
"qcom,pmic-qpnp" or similar. We should still specify in the binding
that PMIC slaves specify a version-specific string as well as the
generic string. That is, a slave should have:
compatible = "qcom,pm8841", "qcom,pmic-qpnp";
...in case we would ever need to differentiate in the future.
(I recall that in a previous version I had done this, but I don't
remember why I had changed it..)
Thanks again,
Josh
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Wed, Apr 23, 2014 at 10:34:32PM +0200, Ivan T. Ivanov wrote:
>
> On Wed, 2014-04-23 at 11:16 -0700, Courtney Cavin wrote:
> > On Wed, Apr 23, 2014 at 03:19:28PM +0200, Ivan T. Ivanov wrote:
> > >
> > > Hi,
> > >
> > > On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote:
> > > > From: Josh Cartwright <[email protected]>
> > > >
> > > > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> > > > 800 series SoC family. This driver exists largely as a glue mfd component,
> > > > it exists to be an owner of an SPMI regmap for children devices
> > > > described in device tree.
> > > >
> > >
> > > Thanks. This is exactly what I have planed to do :-)
> > >
> >
> > Sorry if I usurped your work!
>
> Noting to worry. I just was surprised how close it is to my vision ;-).
>
Well, you might notice how extremely close it is to what Josh posted in
October in his SPMI series [1], so I can't take any credit there.
> >
> > > > Signed-off-by: Josh Cartwright <[email protected]>
> > > > Signed-off-by: Courtney Cavin <[email protected]>
> > > > ---
> > > > drivers/mfd/Kconfig | 13 +++++++++++
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/pm8x41.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 77 insertions(+)
> > > > create mode 100644 drivers/mfd/pm8x41.c
> > > >
> > >
> > > <snip>
> > >
> > > > +
> > > > +static int pm8x41_probe(struct spmi_device *sdev)
> > > > +{
> > > > + struct regmap *regmap;
> > > > +
> > > > + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
> > > > + if (IS_ERR(regmap)) {
> > > > + dev_dbg(&sdev->dev, "regmap creation failed.\n");
> > > > + return PTR_ERR(regmap);
> > > > + }
> > > > +
> > > > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> > >
> > > I think that this will not going to work. For example in this particular
> > > case, both controllers have "qcom,qpnp-revid" peripheral which is
> > > located at offset 0x100.
> > >
> > > And the result is:
> > >
> > > [ 0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid'
> > >
> > [...]
> > >
> > > Any suggestions?
> >
> > That's expected behavior actually. You have two nodes in DT named the
> > same thing and at the same address. This error is due to the fact that
> > all devices are put in '/bus/platform/devices/' with a name made from
> > the unit address and name specified in DT. There's no other unique
> > information used to differentiate the devices.
> >
> > If you simply change the names in DT, it works.
>
> Sure, it will work. But they are part of different address spaces.
> Why we should add, IMHO, artificial requirement that names should
> be unique? Is it possible to prefix child nodes with parent device
> address? As side note, why they should be registered on the platform
> bus at all? To be honest I don't have solution.
I agree, and it would appear that the ePAPR does as well. Feel free to
send patches!
> > [...] qcom,qpnp-revid 100.qcom,pm8841-revid: PM8841 v2.0 options: 0, 0, 2, 2
> > [...] qcom,qpnp-revid 100.qcom,pm8941-revid: PM8941 v3.0 options: 2, 0, 0, 0
> >
> > Whether this should be "fixed" in the device/bus/sysfs core, I don't
> > know, but it isn't specifically an issue with this driver, and there's
> > little-to-nothing I can do to fix it here.
> >
> > -Courtney
On Wed, Apr 23, 2014 at 11:46:26PM +0200, Josh Cartwright wrote:
> On Tue, Apr 22, 2014 at 05:31:49PM -0700, Courtney Cavin wrote:
> > From: Josh Cartwright <[email protected]>
> >
> > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> > 800 series SoC family. This driver exists largely as a glue mfd component,
> > it exists to be an owner of an SPMI regmap for children devices
> > described in device tree.
> >
> > Signed-off-by: Josh Cartwright <[email protected]>
> > Signed-off-by: Courtney Cavin <[email protected]>
>
> Hey Courtney-
>
> Thanks for picking this up!
Well, I've been poking you about it for months now, so I figured I'd
stop being annoying, and start being productive.
>
> One thing that I had meant to do is rename this thing. Nothing about
> this is PM8841/PM8941 specific at all. It should apply equally to all
> Qualcomm's PMICs which implement QPNP.
>
> Perhaps a better name would be "qcom-pmic-qpnp".
What's a QPNP? Really. I've heard you speak about it before as being a
definition of the register layout for interrupts, but I have no
documentation on it.
I would argue here from my understanding that this driver isn't specific
to QPNP either. With that in mind we could just go with
"qcom-pmic-spmi". In fact just "spmi-ext" would not be incorrect, as
this driver has little to do with PMICs at all.
My point here is that we can easily make this into something very
generic, but that only causes problems in the future when it's not
"generic enough", and we have to add quirks. If in the future Qualcomm
releases a pm8A41, and it's qpnp, but not spmi, or spmi, but not 'ext',
then we need to either change this driver dramatically, or write a new
one. I like keeping this driver name specific to what we know it
supports. We can rename it in the future if deemed appropriate, but I'd
rather not make it something that which turns out to be wrong at some
later point.
Having said all of this, I have no doubt that your HW engineers will
find a way to break our interpretation of their naming scheme... once
again.
>
> [..]
> > +++ b/drivers/mfd/pm8x41.c
> > @@ -0,0 +1,63 @@
> > +/* Copyright (c) 2013, The Linux Foundation. All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 and
> > + * only version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/spmi.h>
> > +#include <linux/regmap.h>
> > +#include <linux/of_platform.h>
> > +
> > +static const struct regmap_config pm8x41_regmap_config = {
> > + .reg_bits = 16,
> > + .val_bits = 8,
> > + .max_register = 0xFFFF,
> > +};
>
> This reminds me. David Collins (CC'd) noticed that there are usecases
> where peripheral drivers will need to be accessing registers from atomic
> context, so we should probably be setting .fast_io in the SPMI
> regmap_bus structures, but we can tackle that when we get there.
Hrm. Please comment on this David. I would like to see a solid
use-case before even considering that.
> > +
> > +static int pm8x41_remove_child(struct device *dev, void *unused)
> > +{
> > + platform_device_unregister(to_platform_device(dev));
> > + return 0;
> > +}
> > +
> > +static void pm8x41_remove(struct spmi_device *sdev)
> > +{
> > + device_for_each_child(&sdev->dev, NULL, pm8x41_remove_child);
> > +}
> > +
> > +static int pm8x41_probe(struct spmi_device *sdev)
> > +{
> > + struct regmap *regmap;
> > +
> > + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
> > + if (IS_ERR(regmap)) {
> > + dev_dbg(&sdev->dev, "regmap creation failed.\n");
> > + return PTR_ERR(regmap);
> > + }
> > +
> > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> > +}
> > +
> > +static const struct of_device_id pm8x41_id_table[] = {
> > + { .compatible = "qcom,pm8841", },
> > + { .compatible = "qcom,pm8941", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, pm8x41_id_table);
>
> I'm thinking we should probably have a generic compatible entry as well,
> "qcom,pmic-qpnp" or similar. We should still specify in the binding
> that PMIC slaves specify a version-specific string as well as the
> generic string. That is, a slave should have:
>
> compatible = "qcom,pm8841", "qcom,pmic-qpnp";
>
> ...in case we would ever need to differentiate in the future.
>
> (I recall that in a previous version I had done this, but I don't
> remember why I had changed it..)
I gave this some thought but came to the conclusion that there is no
benefit of adding a generic compatible to a new binding. Please clarify
a use-case where this would be ... useful.
Thanks for the review!
-Courtney
On Wed, Apr 23, 2014 at 8:19 AM, Ivan T. Ivanov <[email protected]> wrote:
>
> Hi,
>
> On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote:
>> From: Josh Cartwright <[email protected]>
>>
>> The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
>> 800 series SoC family. This driver exists largely as a glue mfd component,
>> it exists to be an owner of an SPMI regmap for children devices
>> described in device tree.
>>
>
> Thanks. This is exactly what I have planed to do :-)
>
>> Signed-off-by: Josh Cartwright <[email protected]>
>> Signed-off-by: Courtney Cavin <[email protected]>
>> ---
>> drivers/mfd/Kconfig | 13 +++++++++++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/pm8x41.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 77 insertions(+)
>> create mode 100644 drivers/mfd/pm8x41.c
>>
>
> <snip>
>
>> +
>> +static int pm8x41_probe(struct spmi_device *sdev)
>> +{
>> + struct regmap *regmap;
>> +
>> + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
>> + if (IS_ERR(regmap)) {
>> + dev_dbg(&sdev->dev, "regmap creation failed.\n");
>> + return PTR_ERR(regmap);
>> + }
>> +
>> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
>
> I think that this will not going to work. For example in this particular
> case, both controllers have "qcom,qpnp-revid" peripheral which is
> located at offset 0x100.
>
> And the result is:
>
> [ 0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid'
>
> DT looks like this:
>
> spmi {
> compatible = "qcom,spmi-pmic-arb";
> reg-names = "core", "intr", "cnfg";
> reg = <0xfc4cf000 0x1000>,
> <0xfc4cb000 0x1000>,
> <0xfc4ca000 0x1000>;
>
> interrupt-names = "periph_irq";
> interrupts = <0 190 0>;
>
> qcom,ee = <0>;
> qcom,channel = <0>;
>
> #address-cells = <2>;
> #size-cells = <0>;
>
> interrupt-controller;
> #interrupt-cells = <4>;
>
> pm8941@0 {
> compatible = "qcom,pm8941";
> reg = <0x0 SPMI_USID>;
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> revid@100 {
> compatible = "qcom,qpnp-revid";
> reg = <0x100 0x100>;
> };
> };
>
> pm8841@4 {
> compatible = "qcom,pm8941";
> reg = <0x4 SPMI_USID>;
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> revid@100 {
> compatible = "qcom,qpnp-revid";
> reg = <0x100 0x100>;
> };
> };
> };
>
> Any suggestions?
Probably we should only use the unit address when we have translatable
addresses. Or we could append the parent reg address to the name, but
you may have cases that don't have a parent reg value.
Rob
On Wed, Apr 23, 2014 at 04:36:22PM -0700, Courtney Cavin wrote:
> On Wed, Apr 23, 2014 at 11:46:26PM +0200, Josh Cartwright wrote:
> > On Tue, Apr 22, 2014 at 05:31:49PM -0700, Courtney Cavin wrote:
[..]
> > One thing that I had meant to do is rename this thing. Nothing about
> > this is PM8841/PM8941 specific at all. It should apply equally to all
> > Qualcomm's PMICs which implement QPNP.
> >
> > Perhaps a better name would be "qcom-pmic-qpnp".
>
> What's a QPNP? Really. I've heard you speak about it before as being a
> definition of the register layout for interrupts, but I have no
> documentation on it.
QPNP is effectively (as I explained before) a partitioning scheme for
dividing the SPMI extended register space up into logical pieces, and
set of fixed register locations/definitions within these regions, with
some of these regions specifically used for interrupt handling.
> I would argue here from my understanding that this driver isn't specific
> to QPNP either. With that in mind we could just go with
> "qcom-pmic-spmi". In fact just "spmi-ext" would not be incorrect, as
> this driver has little to do with PMICs at all.
I'm actually not opposed to either of those suggested names.
> My point here is that we can easily make this into something very
> generic, but that only causes problems in the future when it's not
> "generic enough", and we have to add quirks.
Yes, this is why I'd still like to require having the specific PMIC
listed in the slave node's compatible string in front of a "generic"
one. Without a perfect crystal ball, it's the best we have.
> If in the future Qualcomm releases a pm8A41, and it's qpnp, but not
> spmi, or spmi, but not 'ext', then we need to either change this
> driver dramatically, or write a new one. I like keeping this driver
> name specific to what we know it supports. We can rename it in the
> future if deemed appropriate, but I'd rather not make it something
> that which turns out to be wrong at some later point.
I don't necessarily disagree with the strategy, however, if you take a
look at the downstream msm-3.10 tree[1], you'll see that there are
already quite a few other PMICs that could be made to leverage this
driver with likely no changes (downstream the equivalent is a dt node
tagged spmi-slave-container):
$ git grep spmi-slave-container arch/arm/boot/dts
arch/arm/boot/dts/qcom/msm-pm8019.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8019.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8110.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8110.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8226.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8226.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8841.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8841.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8916.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8916.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8941.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pm8941.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pma8084.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pma8084.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pmd9635.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pmd9635.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pmi8962.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pmi8962.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pmiplutonium.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pmiplutonium.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pmplutonium.dtsi: spmi-slave-container;
arch/arm/boot/dts/qcom/msm-pmplutonium.dtsi: spmi-slave-container;
[..]
> > > +static const struct of_device_id pm8x41_id_table[] = {
> > > + { .compatible = "qcom,pm8841", },
> > > + { .compatible = "qcom,pm8941", },
> > > + {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, pm8x41_id_table);
> >
> > I'm thinking we should probably have a generic compatible entry as well,
> > "qcom,pmic-qpnp" or similar. We should still specify in the binding
> > that PMIC slaves specify a version-specific string as well as the
> > generic string. That is, a slave should have:
> >
> > compatible = "qcom,pm8841", "qcom,pmic-qpnp";
> >
> > ...in case we would ever need to differentiate in the future.
> >
> > (I recall that in a previous version I had done this, but I don't
> > remember why I had changed it..)
>
> I gave this some thought but came to the conclusion that there is no
> benefit of adding a generic compatible to a new binding. Please clarify
> a use-case where this would be ... useful.
Having a generic compatible entry allows for easily supporting new PMICs
without having to add yet another vacuous entry in the ID table. In
this case I think it's perfectly acceptable given that this driver isn't
really defining a programming model for a specific device, but rather
acting much more like a bus.
Requiring a specific PMIC listed before a generic one allows us an
escape hatch in the future if for some reason we need to add a quirk for
a specific PMIC.
Josh
[1]: git://codeaurora.org/quic/la/kernel/msm-3.10#msm-3.10
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On 4/23/2014 6:19 AM, Ivan T. Ivanov wrote:
>
> Hi,
>
> On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote:
>> From: Josh Cartwright <[email protected]>
>>
>> The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
>> 800 series SoC family. This driver exists largely as a glue mfd component,
>> it exists to be an owner of an SPMI regmap for children devices
>> described in device tree.
>>
>
> Thanks. This is exactly what I have planed to do :-)
>
>> Signed-off-by: Josh Cartwright <[email protected]>
>> Signed-off-by: Courtney Cavin <[email protected]>
>> ---
>> drivers/mfd/Kconfig | 13 +++++++++++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/pm8x41.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 77 insertions(+)
>> create mode 100644 drivers/mfd/pm8x41.c
>>
>
> <snip>
>
>> +
>> +static int pm8x41_probe(struct spmi_device *sdev)
>> +{
>> + struct regmap *regmap;
>> +
>> + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
>> + if (IS_ERR(regmap)) {
>> + dev_dbg(&sdev->dev, "regmap creation failed.\n");
>> + return PTR_ERR(regmap);
>> + }
>> +
>> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
>
> I think that this will not going to work. For example in this particular
> case, both controllers have "qcom,qpnp-revid" peripheral which is
> located at offset 0x100.
>
> And the result is:
>
> [ 0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid'
The duplicate filename error is because pm8x41_probe() is calling of_platform_populate()
directly.
If that call is removed then there is no attempt to create a revid file in
/sys/bus/platform/devices. If I add your pm8841@4 node to my dts file, then
the 100.revid file is properly created at
/sys/firmware/devicetree/base/soc/qcom,spmi@fc4cf000/pm8841@4/qcom,revid@100
and no attempt is made to create /sys/bus/platform/devices/100.revid
This appears to be correct to me, because I do not think revid should be created as
a platform device.
>
> DT looks like this:
>
> spmi {
> compatible = "qcom,spmi-pmic-arb";
> reg-names = "core", "intr", "cnfg";
> reg = <0xfc4cf000 0x1000>,
> <0xfc4cb000 0x1000>,
> <0xfc4ca000 0x1000>;
>
> interrupt-names = "periph_irq";
> interrupts = <0 190 0>;
>
> qcom,ee = <0>;
> qcom,channel = <0>;
>
> #address-cells = <2>;
> #size-cells = <0>;
>
> interrupt-controller;
> #interrupt-cells = <4>;
>
> pm8941@0 {
> compatible = "qcom,pm8941";
> reg = <0x0 SPMI_USID>;
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> revid@100 {
> compatible = "qcom,qpnp-revid";
> reg = <0x100 0x100>;
> };
> };
>
> pm8841@4 {
^^^^^^^^ typo nit - that should be pm8941@4.
The nit does not change what you reported though.
> compatible = "qcom,pm8941";
> reg = <0x4 SPMI_USID>;
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> revid@100 {
> compatible = "qcom,qpnp-revid";
> reg = <0x100 0x100>;
> };
> };
> };
>
> Any suggestions?
Remove of_platform_populate() from pm8x41_probe(). Do you know of any
other reason it can not be removed?
-Frank
On Sat, Apr 26, 2014 at 02:28:06AM +0200, Frank Rowand wrote:
> On 4/23/2014 6:19 AM, Ivan T. Ivanov wrote:
[...]
> >> +static int pm8x41_probe(struct spmi_device *sdev)
> >> +{
> >> + struct regmap *regmap;
> >> +
> >> + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
> >> + if (IS_ERR(regmap)) {
> >> + dev_dbg(&sdev->dev, "regmap creation failed.\n");
> >> + return PTR_ERR(regmap);
> >> + }
> >> +
> >> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> >
> > I think that this will not going to work. For example in this particular
> > case, both controllers have "qcom,qpnp-revid" peripheral which is
> > located at offset 0x100.
> >
> > And the result is:
> >
> > [ 0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid'
>
> The duplicate filename error is because pm8x41_probe() is calling of_platform_populate()
> directly.
>
> If that call is removed then there is no attempt to create a revid file in
> /sys/bus/platform/devices. If I add your pm8841@4 node to my dts file, then
> the 100.revid file is properly created at
>
> /sys/firmware/devicetree/base/soc/qcom,spmi@fc4cf000/pm8841@4/qcom,revid@100
>
> and no attempt is made to create /sys/bus/platform/devices/100.revid
>
> This appears to be correct to me, because I do not think revid should be created as
> a platform device.
>
Wait, what? That's the entire point of this driver.
[...]
> > Any suggestions?
>
> Remove of_platform_populate() from pm8x41_probe(). Do you know of any
> other reason it can not be removed?
I do! If you remove it, the entire driver becomes a useless pile of
nothing!
The PMICs targeted by this driver are mostly made up of independent
blocks which should be able to be written as standalone device drivers.
The design of this driver is to create a simple bus-like layer between
SPMI and these independent blocks. of_platform_populate() is what makes
it work.
-Courtney
On 4/25/2014 5:40 PM, Courtney Cavin wrote:
> On Sat, Apr 26, 2014 at 02:28:06AM +0200, Frank Rowand wrote:
>> On 4/23/2014 6:19 AM, Ivan T. Ivanov wrote:
> [...]
>>>> +static int pm8x41_probe(struct spmi_device *sdev)
>>>> +{
>>>> + struct regmap *regmap;
>>>> +
>>>> + regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
>>>> + if (IS_ERR(regmap)) {
>>>> + dev_dbg(&sdev->dev, "regmap creation failed.\n");
>>>> + return PTR_ERR(regmap);
>>>> + }
>>>> +
>>>> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
>>>
>>> I think that this will not going to work. For example in this particular
>>> case, both controllers have "qcom,qpnp-revid" peripheral which is
>>> located at offset 0x100.
>>>
>>> And the result is:
>>>
>>> [ 0.963944] sysfs: cannot create duplicate filename '/bus/platform/devices/100.revid'
>>
>> The duplicate filename error is because pm8x41_probe() is calling of_platform_populate()
>> directly.
>>
>> If that call is removed then there is no attempt to create a revid file in
>> /sys/bus/platform/devices. If I add your pm8841@4 node to my dts file, then
>> the 100.revid file is properly created at
>>
>> /sys/firmware/devicetree/base/soc/qcom,spmi@fc4cf000/pm8841@4/qcom,revid@100
>>
>> and no attempt is made to create /sys/bus/platform/devices/100.revid
>>
>> This appears to be correct to me, because I do not think revid should be created as
>> a platform device.
>>
>
> Wait, what? That's the entire point of this driver.
>
> [...]
>>> Any suggestions?
>>
>> Remove of_platform_populate() from pm8x41_probe(). Do you know of any
>> other reason it can not be removed?
>
> I do! If you remove it, the entire driver becomes a useless pile of
> nothing!
>
> The PMICs targeted by this driver are mostly made up of independent
> blocks which should be able to be written as standalone device drivers.
> The design of this driver is to create a simple bus-like layer between
> SPMI and these independent blocks. of_platform_populate() is what makes
> it work.
>
> -Courtney
>
Hmmmm, yet another device tree conceptual thing for me to grok. I'll go
off and see if a bus-like layer can exist without being a platform device.
-Frank
On 04/23/2014 04:36 PM, Courtney Cavin wrote:
> On Wed, Apr 23, 2014 at 11:46:26PM +0200, Josh Cartwright wrote:
>> On Tue, Apr 22, 2014 at 05:31:49PM -0700, Courtney Cavin wrote:
[..]
>>> +++ b/drivers/mfd/pm8x41.c
>>> @@ -0,0 +1,63 @@
>>> +/* Copyright (c) 2013, The Linux Foundation. All rights reserved.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 and
>>> + * only version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/spmi.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/of_platform.h>
>>> +
>>> +static const struct regmap_config pm8x41_regmap_config = {
>>> + .reg_bits = 16,
>>> + .val_bits = 8,
>>> + .max_register = 0xFFFF,
>>> +};
>>
>> This reminds me. David Collins (CC'd) noticed that there are usecases
>> where peripheral drivers will need to be accessing registers from atomic
>> context, so we should probably be setting .fast_io in the SPMI
>> regmap_bus structures, but we can tackle that when we get there.
>
> Hrm. Please comment on this David. I would like to see a solid
> use-case before even considering that.
Several drivers in the downstream msm-3.10 tree [1] are currently making
SPMI read and write calls from atomic context. For the most part this
corresponds to non-threaded interrupt handlers. Some examples of that are
qpnp-charger [2] and qpnp-adc-tm [3].
Another case in which atomic SPMI read and write calls are needed is when
very tight timing constraints must be ensured between successive SPMI
transactions. An example of this can be found in the qpnp-bsi MIPI-BIF
smart battery driver [4]. It disables interrupts when performing BIF
burst reads because the data register must be read within 10's of
microseconds after the status register indicates that data is ready. If
the data read is too slow, then next word in the burst read overrides the
current one.
- David
[1]: git://codeaurora.org/quic/la/kernel/msm-3.10#msm-3.10
[2]:
https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/power/qpnp-charger.c?h=msm-3.10&id=77ea4f2d70056169a80519fc9c7dd7156469ef11#n4173
[3]:
https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/thermal/qpnp-adc-tm.c?h=msm-3.10&id=77ea4f2d70056169a80519fc9c7dd7156469ef11#n2076
[4]:
https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/bif/qpnp-bsi.c?h=msm-3.10&id=77ea4f2d70056169a80519fc9c7dd7156469ef11#n943
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Hi Frank,
On Fri, 2014-04-25 at 17:28 -0700, Frank Rowand wrote:
> On 4/23/2014 6:19 AM, Ivan T. Ivanov wrote:
<snip>
> > spmi {
> > compatible = "qcom,spmi-pmic-arb";
> > reg-names = "core", "intr", "cnfg";
> > reg = <0xfc4cf000 0x1000>,
> > <0xfc4cb000 0x1000>,
> > <0xfc4ca000 0x1000>;
> >
> > interrupt-names = "periph_irq";
> > interrupts = <0 190 0>;
> >
> > qcom,ee = <0>;
> > qcom,channel = <0>;
> >
> > #address-cells = <2>;
> > #size-cells = <0>;
> >
> > interrupt-controller;
> > #interrupt-cells = <4>;
> >
> > pm8941@0 {
> > compatible = "qcom,pm8941";
> > reg = <0x0 SPMI_USID>;
> >
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > revid@100 {
> > compatible = "qcom,qpnp-revid";
> > reg = <0x100 0x100>;
This should be just reg = <0x100>;
> > };
> > };
> >
> > pm8841@4 {
>
> ^^^^^^^^ typo nit - that should be pm8941@4.
> The nit does not change what you reported though.
>
> > compatible = "qcom,pm8941";
Actually this one is incorrect, it should be "qcom,pm8841",
but as you say it doesn't make difference in the end result.
> > reg = <0x4 SPMI_USID>;
> >
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > revid@100 {
> > compatible = "qcom,qpnp-revid";
> > reg = <0x100 0x100>;
also here.
Regards,
Ivan