| Message ID | 1398213110-28135-1-git-send-email-courtney.cavin@sonymobile.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
> From: Josh Cartwright <joshc@codeaurora.org> > > 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 <joshc@codeaurora.org> > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > --- > 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);
Hi, On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote: > From: Josh Cartwright <joshc@codeaurora.org> > > 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 <joshc@codeaurora.org> > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 23, 2014 at 12:50:55PM +0200, Lee Jones wrote: > > From: Josh Cartwright <joshc@codeaurora.org> > > > > 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 <joshc@codeaurora.org> > > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <joshc@codeaurora.org> > > > > 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 <joshc@codeaurora.org> > > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <joshc@codeaurora.org> > > > > > > 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 <joshc@codeaurora.org> > > > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > > > --- > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 22, 2014 at 05:31:49PM -0700, Courtney Cavin wrote: > From: Josh Cartwright <joshc@codeaurora.org> > > 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 <joshc@codeaurora.org> > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> 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
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 <joshc@codeaurora.org> > > > > > > > > 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 <joshc@codeaurora.org> > > > > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > > > > --- > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <joshc@codeaurora.org> > > > > 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 <joshc@codeaurora.org> > > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 23, 2014 at 8:19 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote: > > Hi, > > On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote: >> From: Josh Cartwright <joshc@codeaurora.org> >> >> 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 <joshc@codeaurora.org> >> Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 <joshc@codeaurora.org> >> >> 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 <joshc@codeaurora.org> >> Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 23, 2014 at 8:19 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote: > > Hi, > > On Tue, 2014-04-22 at 17:31 -0700, Courtney Cavin wrote: >> From: Josh Cartwright <joshc@codeaurora.org> >> >> 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 <joshc@codeaurora.org> >> Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> >> --- >> 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>; > }; > }; > }; > I've created a similar test case, but do not reproduce the duplicate name issue. In my test, the lack of ranges property causes translation to fail so devices are named dev.N. The above should have similar behavior.1 Here's the test case: / { testcase-data { platform-tests { #address-cells = <1>; #size-cells = <1>; test-device@0 { compatible = "test-device"; reg = <0x0 0>; #address-cells = <1>; #size-cells = <1>; dev@100 { compatible = "test-sub-device"; reg = <0x100 0x100>; }; }; test-device@1 { compatible = "test-device"; reg = <0x1 0>; #address-cells = <1>; #size-cells = <1>; dev@100 { compatible = "test-sub-device"; reg = <0x100 0x100>; }; }; }; }; }; Rob -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, 2014-04-24 at 13:18 -0500, Josh Cartwright wrote: > 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: > [..] <snip> > > $ 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. Is there a conclusion on this issue? I am voting for generic name :-) "qcom,pm-qpnp". Further complication is that several sub function drivers expect to runtime detect the exact version of the controller ("qcom, qpnp-iadc", "qcom, qpnp-vadc", "qcom, qpnp-linear-charger"). This is realized by the exported function of the driver "qcom, qpnp-revid". Would it be good idea to merge qpnp-revid and "qcom,pm-qpnp" driver? Regards, Ivan > > Josh > > [1]: git://codeaurora.org/quic/la/kernel/msm-3.10#msm-3.10 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 09, 2014 at 02:45:30PM +0200, Ivan T. Ivanov wrote: [...] > > > > > > > > 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. > > Is there a conclusion on this issue? I am voting for generic name :-) > "qcom,pm-qpnp". Josh and I have discussed this offline, and I think we have come to the conclusion that this should be a generic driver with only a generic binding. The current proposed name is "spmi-ext", as there is specific functional relation to Qualcomm, PMICs or QPNP. Further, the binding documentation should be specific to pm8[89]41 as 'mfd/pm8x41.txt', and should contain the compatibles: - "qcom,pm8941", "spmi-ext" - "qcom,pm8841", "spmi-ext" This naming has been discussed to death, so a few more shed color suggestions can't possibly hurt. > Further complication is that several sub function drivers expect to > runtime detect the exact version of the controller ("qcom, qpnp-iadc", > "qcom, qpnp-vadc", "qcom, qpnp-linear-charger"). This is realized by the > exported function of the driver "qcom, qpnp-revid". Would it be good > idea to merge qpnp-revid and "qcom,pm-qpnp" driver? Each block within the PMICs have--undocumented--version registers, so a global version number is not particularly useful. A good example of this is the ADC code [1], as you mentioned. -Courtney [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/thermal/qpnp-adc-tm.c#n469 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Courtney, On Fri, 2014-05-09 at 13:30 -0700, Courtney Cavin wrote: > > > 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. > > > > Is there a conclusion on this issue? I am voting for generic name :-) > > "qcom,pm-qpnp". > > Josh and I have discussed this offline, and I think we have come to the > conclusion that this should be a generic driver with only a generic > binding. The current proposed name is "spmi-ext", as there is specific > functional relation to Qualcomm, PMICs or QPNP. > > Further, the binding documentation should be specific to pm8[89]41 as > 'mfd/pm8x41.txt', and should contain the compatibles: > - "qcom,pm8941", "spmi-ext" > - "qcom,pm8841", "spmi-ext" > > This naming has been discussed to death, so a few more shed color > suggestions can't possibly hurt. I am fine with this. Thanks. > > > Further complication is that several sub function drivers expect to > > runtime detect the exact version of the controller ("qcom, qpnp-iadc", > > "qcom, qpnp-vadc", "qcom, qpnp-linear-charger"). This is realized by the > > exported function of the driver "qcom, qpnp-revid". Would it be good > > idea to merge qpnp-revid and "qcom,pm-qpnp" driver? > > Each block within the PMICs have--undocumented--version registers, so a > global version number is not particularly useful. A good example of > this is the ADC code [1], as you mentioned. Do you happen to know how to match local subdevices revisions to global PMIC revision? Earlier mentioned drivers are using global chip revision. I will not be surprised if local subdevice version is not changed, but instead global version is changed, when only subdevice functionality is changed ;-) Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);