| Message ID | 1404393243-7324-2-git-send-email-svarbanov@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 03 Jul 2014, Stanimir Varbanov wrote: > From: Josh Cartwright <joshc@codeaurora.org> > > The Qualcomm QPNP PMIC chips 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: Stanimir Varbanov <svarbanov@mm-sol.com> > --- > drivers/mfd/Kconfig | 15 ++++++ > drivers/mfd/Makefile | 1 + > drivers/mfd/qpnp-spmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 145 insertions(+), 0 deletions(-) > create mode 100644 drivers/mfd/qpnp-spmi.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index ee8204c..258b733 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -524,6 +524,21 @@ 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_QPNP_SPMI > + tristate "Qualcomm QPNP SPMI PMIC" > + depends on ARCH_QCOM || COMPILE_TEST > + depends on OF > + select MFD_CORE > + select REGMAP_SPMI > + help > + This enables support for the Qualcomm QPNP SPMI 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. It would be helpful if the expected types of child devices were mentioned here in the help section. > + Say M here if you want to include support for the QPNP SPMI PMIC > + series as a module. The module will be called "qpnp-spmi". > + > config MFD_RDC321X > tristate "RDC R-321x southbridge" > select MFD_CORE > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 8afedba..31833d7 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -153,6 +153,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_QPNP_SPMI) += qpnp-spmi.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/qpnp-spmi.c b/drivers/mfd/qpnp-spmi.c > new file mode 100644 > index 0000000..efd7d3e > --- /dev/null > +++ b/drivers/mfd/qpnp-spmi.c > @@ -0,0 +1,129 @@ > +/* Copyright (c) 2014, The Linux Foundation. All rights reserved. > + * Non-standard multi-line comment. First line should be blank. > + * 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. No author? > + * 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_address.h> > +#include <linux/slab.h> > +#include <linux/mfd/core.h> > + > +#define QPNP_RESOURCE_SIZE 256 > + > +static const struct regmap_config qpnp_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > + .max_register = 0xffff, > +}; > + > +static int qpnp_index_to_resource(struct device_node *np, int index, > + struct resource *res) > +{ > + const char *name = NULL; > + const __be32 *addrp; > + u64 addr; > + > + addrp = of_get_address(np, index, NULL, NULL); > + if (!addrp) > + return -EINVAL; > + > + addr = of_read_number(addrp, 1); > + if (addr == OF_BAD_ADDR) > + return -EINVAL; > + > + of_property_read_string_index(np, "reg-names", index, &name); > + > + res->start = addr; > + res->end = addr + QPNP_RESOURCE_SIZE - 1; > + res->flags = IORESOURCE_REG; > + res->name = name ? name : np->name; > + > + return 0; > +} > + > +static int qpnp_add_device(struct spmi_device *root, struct device_node *child) > +{ > + struct mfd_cell cell = {}; > + struct resource *res, *r; > + int num_resources = 0; > + const char *compat; > + int ret, i; > + > + compat = of_get_property(child, "compatible", NULL); > + if (!compat) > + return -ENODEV; > + > + while (of_get_address(child, num_resources, NULL, NULL)) > + num_resources++; > + > + if (!num_resources) > + return -ENODEV; > + > + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL); > + if (!res) > + return -ENOMEM; > + > + r = res; > + for (i = 0; i < num_resources; i++, r++) > + qpnp_index_to_resource(child, i, r); > + > + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid, > + (u16)res[0].start, child->name); > + cell.of_compatible = compat; > + cell.num_resources = num_resources; > + cell.resources = res; > + > + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1, > + NULL, 0, NULL); > + > + kfree(res); > + kfree(cell.name); > + > + return ret; > +} > + > +static int qpnp_probe(struct spmi_device *sdev) > +{ > + struct device_node *root = sdev->dev.of_node; > + struct device_node *child; > + struct regmap *regmap; > + > + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + for_each_available_child_of_node(root, child) > + qpnp_add_device(sdev, child); This entire driver looks like a re-write of of_platform_populate(). Why? > + return 0; > +} > + > +static void qpnp_remove(struct spmi_device *sdev) > +{ > + mfd_remove_devices(&sdev->dev); > +} > + > +static const struct of_device_id qpnp_id_table[] = { > + { .compatible = "qcom,qpnp-spmi", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, qpnp_id_table); > + > +static struct spmi_driver qpnp_driver = { > + .probe = qpnp_probe, > + .remove = qpnp_remove, > + .driver = { > + .name = "qpnp-spmi", > + .of_match_table = qpnp_id_table, > + }, > +}; > +module_spmi_driver(qpnp_driver);
Hi Lee, Thanks for the comments. On 07/09/2014 05:34 PM, Lee Jones wrote: > On Thu, 03 Jul 2014, Stanimir Varbanov wrote: >> From: Josh Cartwright <joshc@codeaurora.org> >> >> The Qualcomm QPNP PMIC chips 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: Stanimir Varbanov <svarbanov@mm-sol.com> >> --- >> drivers/mfd/Kconfig | 15 ++++++ >> drivers/mfd/Makefile | 1 + >> drivers/mfd/qpnp-spmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 145 insertions(+), 0 deletions(-) >> create mode 100644 drivers/mfd/qpnp-spmi.c >> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >> index ee8204c..258b733 100644 >> --- a/drivers/mfd/Kconfig >> +++ b/drivers/mfd/Kconfig >> @@ -524,6 +524,21 @@ 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_QPNP_SPMI >> + tristate "Qualcomm QPNP SPMI PMIC" >> + depends on ARCH_QCOM || COMPILE_TEST >> + depends on OF >> + select MFD_CORE >> + select REGMAP_SPMI >> + help >> + This enables support for the Qualcomm QPNP SPMI 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. > > It would be helpful if the expected types of child devices were > mentioned here in the help section. They are many, but I could list the major ones. > >> + Say M here if you want to include support for the QPNP SPMI PMIC >> + series as a module. The module will be called "qpnp-spmi". >> + >> config MFD_RDC321X >> tristate "RDC R-321x southbridge" >> select MFD_CORE >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 8afedba..31833d7 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -153,6 +153,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_QPNP_SPMI) += qpnp-spmi.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/qpnp-spmi.c b/drivers/mfd/qpnp-spmi.c >> new file mode 100644 >> index 0000000..efd7d3e >> --- /dev/null >> +++ b/drivers/mfd/qpnp-spmi.c >> @@ -0,0 +1,129 @@ >> +/* Copyright (c) 2014, The Linux Foundation. All rights reserved. >> + * > > Non-standard multi-line comment. First line should be blank. sure, will fix. > >> + * 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. > > No author? > >> + * 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_address.h> >> +#include <linux/slab.h> >> +#include <linux/mfd/core.h> >> + >> +#define QPNP_RESOURCE_SIZE 256 >> + >> +static const struct regmap_config qpnp_regmap_config = { >> + .reg_bits = 16, >> + .val_bits = 8, >> + .max_register = 0xffff, >> +}; >> + >> +static int qpnp_index_to_resource(struct device_node *np, int index, >> + struct resource *res) >> +{ >> + const char *name = NULL; >> + const __be32 *addrp; >> + u64 addr; >> + >> + addrp = of_get_address(np, index, NULL, NULL); >> + if (!addrp) >> + return -EINVAL; >> + >> + addr = of_read_number(addrp, 1); >> + if (addr == OF_BAD_ADDR) >> + return -EINVAL; >> + >> + of_property_read_string_index(np, "reg-names", index, &name); >> + >> + res->start = addr; >> + res->end = addr + QPNP_RESOURCE_SIZE - 1; >> + res->flags = IORESOURCE_REG; >> + res->name = name ? name : np->name; >> + >> + return 0; >> +} >> + >> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child) >> +{ >> + struct mfd_cell cell = {}; >> + struct resource *res, *r; >> + int num_resources = 0; >> + const char *compat; >> + int ret, i; >> + >> + compat = of_get_property(child, "compatible", NULL); >> + if (!compat) >> + return -ENODEV; >> + >> + while (of_get_address(child, num_resources, NULL, NULL)) >> + num_resources++; >> + >> + if (!num_resources) >> + return -ENODEV; >> + >> + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL); >> + if (!res) >> + return -ENOMEM; >> + >> + r = res; >> + for (i = 0; i < num_resources; i++, r++) >> + qpnp_index_to_resource(child, i, r); >> + >> + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid, >> + (u16)res[0].start, child->name); >> + cell.of_compatible = compat; >> + cell.num_resources = num_resources; >> + cell.resources = res; >> + >> + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1, >> + NULL, 0, NULL); >> + >> + kfree(res); >> + kfree(cell.name); >> + >> + return ret; >> +} >> + >> +static int qpnp_probe(struct spmi_device *sdev) >> +{ >> + struct device_node *root = sdev->dev.of_node; >> + struct device_node *child; >> + struct regmap *regmap; >> + >> + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config); >> + if (IS_ERR(regmap)) >> + return PTR_ERR(regmap); >> + >> + for_each_available_child_of_node(root, child) >> + qpnp_add_device(sdev, child); > > This entire driver looks like a re-write of of_platform_populate(). > > Why? of_platform_populate is not used because the PMIC function resources are non-translatable. You can see that the resources are of type IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO. The whole point of this mfd driver is to parse devicetree to prepare resources for every child and create platform device for it through mfd_add_devices(). Then the PMIC function driver got its resources and use them as register addresses passed to regmap. These register accesses hits the SPMI controller which is the physical interface between PMIC's and SoC.
On Wed, 09 Jul 2014, Stanimir Varbanov wrote: > Hi Lee, > > Thanks for the comments. > > On 07/09/2014 05:34 PM, Lee Jones wrote: > > On Thu, 03 Jul 2014, Stanimir Varbanov wrote: > >> From: Josh Cartwright <joshc@codeaurora.org> > >> > >> The Qualcomm QPNP PMIC chips 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: Stanimir Varbanov <svarbanov@mm-sol.com> > >> --- > >> drivers/mfd/Kconfig | 15 ++++++ > >> drivers/mfd/Makefile | 1 + > >> drivers/mfd/qpnp-spmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 145 insertions(+), 0 deletions(-) > >> create mode 100644 drivers/mfd/qpnp-spmi.c > >> > >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >> index ee8204c..258b733 100644 > >> --- a/drivers/mfd/Kconfig > >> +++ b/drivers/mfd/Kconfig > >> @@ -524,6 +524,21 @@ 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_QPNP_SPMI > >> + tristate "Qualcomm QPNP SPMI PMIC" > >> + depends on ARCH_QCOM || COMPILE_TEST > >> + depends on OF > >> + select MFD_CORE > >> + select REGMAP_SPMI > >> + help > >> + This enables support for the Qualcomm QPNP SPMI 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. > > > > It would be helpful if the expected types of child devices were > > mentioned here in the help section. > > They are many, but I could list the major ones. > > > > >> + Say M here if you want to include support for the QPNP SPMI PMIC > >> + series as a module. The module will be called "qpnp-spmi". > >> + > >> config MFD_RDC321X > >> tristate "RDC R-321x southbridge" > >> select MFD_CORE > >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > >> index 8afedba..31833d7 100644 > >> --- a/drivers/mfd/Makefile > >> +++ b/drivers/mfd/Makefile > >> @@ -153,6 +153,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_QPNP_SPMI) += qpnp-spmi.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/qpnp-spmi.c b/drivers/mfd/qpnp-spmi.c > >> new file mode 100644 > >> index 0000000..efd7d3e > >> --- /dev/null > >> +++ b/drivers/mfd/qpnp-spmi.c > >> @@ -0,0 +1,129 @@ > >> +/* Copyright (c) 2014, The Linux Foundation. All rights reserved. > >> + * > > > > Non-standard multi-line comment. First line should be blank. > > sure, will fix. > > > > >> + * 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. > > > > No author? > > > >> + * 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_address.h> > >> +#include <linux/slab.h> > >> +#include <linux/mfd/core.h> > >> + > >> +#define QPNP_RESOURCE_SIZE 256 > >> + > >> +static const struct regmap_config qpnp_regmap_config = { > >> + .reg_bits = 16, > >> + .val_bits = 8, > >> + .max_register = 0xffff, > >> +}; > >> + > >> +static int qpnp_index_to_resource(struct device_node *np, int index, > >> + struct resource *res) > >> +{ > >> + const char *name = NULL; > >> + const __be32 *addrp; > >> + u64 addr; > >> + > >> + addrp = of_get_address(np, index, NULL, NULL); > >> + if (!addrp) > >> + return -EINVAL; > >> + > >> + addr = of_read_number(addrp, 1); > >> + if (addr == OF_BAD_ADDR) > >> + return -EINVAL; > >> + > >> + of_property_read_string_index(np, "reg-names", index, &name); > >> + > >> + res->start = addr; > >> + res->end = addr + QPNP_RESOURCE_SIZE - 1; > >> + res->flags = IORESOURCE_REG; > >> + res->name = name ? name : np->name; > >> + > >> + return 0; > >> +} > >> + > >> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child) > >> +{ > >> + struct mfd_cell cell = {}; > >> + struct resource *res, *r; > >> + int num_resources = 0; > >> + const char *compat; > >> + int ret, i; > >> + > >> + compat = of_get_property(child, "compatible", NULL); > >> + if (!compat) > >> + return -ENODEV; > >> + > >> + while (of_get_address(child, num_resources, NULL, NULL)) > >> + num_resources++; > >> + > >> + if (!num_resources) > >> + return -ENODEV; > >> + > >> + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL); > >> + if (!res) > >> + return -ENOMEM; > >> + > >> + r = res; > >> + for (i = 0; i < num_resources; i++, r++) > >> + qpnp_index_to_resource(child, i, r); > >> + > >> + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid, > >> + (u16)res[0].start, child->name); > >> + cell.of_compatible = compat; > >> + cell.num_resources = num_resources; > >> + cell.resources = res; > >> + > >> + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1, > >> + NULL, 0, NULL); > >> + > >> + kfree(res); > >> + kfree(cell.name); > >> + > >> + return ret; > >> +} > >> + > >> +static int qpnp_probe(struct spmi_device *sdev) > >> +{ > >> + struct device_node *root = sdev->dev.of_node; > >> + struct device_node *child; > >> + struct regmap *regmap; > >> + > >> + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config); > >> + if (IS_ERR(regmap)) > >> + return PTR_ERR(regmap); > >> + > >> + for_each_available_child_of_node(root, child) > >> + qpnp_add_device(sdev, child); > > > > This entire driver looks like a re-write of of_platform_populate(). > > > > Why? > > of_platform_populate is not used because the PMIC function resources are > non-translatable. You can see that the resources are of type > IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO. > > The whole point of this mfd driver is to parse devicetree to prepare > resources for every child and create platform device for it through > mfd_add_devices(). Then the PMIC function driver got its resources and > use them as register addresses passed to regmap. These register accesses > hits the SPMI controller which is the physical interface between PMIC's > and SoC. I can't help but think that if this is required, it should be part of the core OF code, rather than doing your own thing which looks frighteningly like existing framework functionality. Either way, I'm going to need a DT Ack before I accept this.
Hi, +Rob Rob I used your old email, sorry about that. <snip> >>>> The Qualcomm QPNP PMIC chips 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: Stanimir Varbanov <svarbanov@mm-sol.com> >>>> --- >>>> drivers/mfd/Kconfig | 15 ++++++ >>>> drivers/mfd/Makefile | 1 + >>>> drivers/mfd/qpnp-spmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 145 insertions(+), 0 deletions(-) >>>> create mode 100644 drivers/mfd/qpnp-spmi.c >>>> >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>> index ee8204c..258b733 100644 >>>> --- a/drivers/mfd/Kconfig >>>> +++ b/drivers/mfd/Kconfig >>>> @@ -524,6 +524,21 @@ 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_QPNP_SPMI >>>> + tristate "Qualcomm QPNP SPMI PMIC" >>>> + depends on ARCH_QCOM || COMPILE_TEST >>>> + depends on OF >>>> + select MFD_CORE >>>> + select REGMAP_SPMI >>>> + help >>>> + This enables support for the Qualcomm QPNP SPMI 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. >>> <snip> >>> >>>> + * 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_address.h> >>>> +#include <linux/slab.h> >>>> +#include <linux/mfd/core.h> >>>> + >>>> +#define QPNP_RESOURCE_SIZE 256 >>>> + >>>> +static const struct regmap_config qpnp_regmap_config = { >>>> + .reg_bits = 16, >>>> + .val_bits = 8, >>>> + .max_register = 0xffff, >>>> +}; >>>> + >>>> +static int qpnp_index_to_resource(struct device_node *np, int index, >>>> + struct resource *res) >>>> +{ >>>> + const char *name = NULL; >>>> + const __be32 *addrp; >>>> + u64 addr; >>>> + >>>> + addrp = of_get_address(np, index, NULL, NULL); >>>> + if (!addrp) >>>> + return -EINVAL; >>>> + >>>> + addr = of_read_number(addrp, 1); >>>> + if (addr == OF_BAD_ADDR) >>>> + return -EINVAL; >>>> + >>>> + of_property_read_string_index(np, "reg-names", index, &name); >>>> + >>>> + res->start = addr; >>>> + res->end = addr + QPNP_RESOURCE_SIZE - 1; >>>> + res->flags = IORESOURCE_REG; >>>> + res->name = name ? name : np->name; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child) >>>> +{ >>>> + struct mfd_cell cell = {}; >>>> + struct resource *res, *r; >>>> + int num_resources = 0; >>>> + const char *compat; >>>> + int ret, i; >>>> + >>>> + compat = of_get_property(child, "compatible", NULL); >>>> + if (!compat) >>>> + return -ENODEV; >>>> + >>>> + while (of_get_address(child, num_resources, NULL, NULL)) >>>> + num_resources++; >>>> + >>>> + if (!num_resources) >>>> + return -ENODEV; >>>> + >>>> + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL); >>>> + if (!res) >>>> + return -ENOMEM; >>>> + >>>> + r = res; >>>> + for (i = 0; i < num_resources; i++, r++) >>>> + qpnp_index_to_resource(child, i, r); >>>> + >>>> + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid, >>>> + (u16)res[0].start, child->name); >>>> + cell.of_compatible = compat; >>>> + cell.num_resources = num_resources; >>>> + cell.resources = res; >>>> + >>>> + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1, >>>> + NULL, 0, NULL); >>>> + >>>> + kfree(res); >>>> + kfree(cell.name); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int qpnp_probe(struct spmi_device *sdev) >>>> +{ >>>> + struct device_node *root = sdev->dev.of_node; >>>> + struct device_node *child; >>>> + struct regmap *regmap; >>>> + >>>> + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config); >>>> + if (IS_ERR(regmap)) >>>> + return PTR_ERR(regmap); >>>> + >>>> + for_each_available_child_of_node(root, child) >>>> + qpnp_add_device(sdev, child); >>> >>> This entire driver looks like a re-write of of_platform_populate(). >>> >>> Why? >> >> of_platform_populate is not used because the PMIC function resources are >> non-translatable. You can see that the resources are of type >> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO. >> >> The whole point of this mfd driver is to parse devicetree to prepare >> resources for every child and create platform device for it through >> mfd_add_devices(). Then the PMIC function driver got its resources and >> use them as register addresses passed to regmap. These register accesses >> hits the SPMI controller which is the physical interface between PMIC's >> and SoC. > > I can't help but think that if this is required, it should be part of > the core OF code, rather than doing your own thing which looks > frighteningly like existing framework functionality. Lee, does it make sense to have a common of_mfd code for devicetree parsing? Is that discussed already? I mean presently the mfd_cell resources and number of resources are passed by the mfd_add_devices users. Is it possible to have common code which parses devicetree sub-nodes of the parent device_node and fill resources/create platform devices for them. Rob, Grant I hope you guys have something to say here? > > Either way, I'm going to need a DT Ack before I accept this. > Sure.
On Thu, 10 Jul 2014, Stanimir Varbanov wrote: > >>>> The Qualcomm QPNP PMIC chips 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: Stanimir Varbanov <svarbanov@mm-sol.com> > >>>> --- > >>>> drivers/mfd/Kconfig | 15 ++++++ > >>>> drivers/mfd/Makefile | 1 + > >>>> drivers/mfd/qpnp-spmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++ > >>>> 3 files changed, 145 insertions(+), 0 deletions(-) > >>>> create mode 100644 drivers/mfd/qpnp-spmi.c > >>>> > >>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >>>> index ee8204c..258b733 100644 > >>>> --- a/drivers/mfd/Kconfig > >>>> +++ b/drivers/mfd/Kconfig > >>>> @@ -524,6 +524,21 @@ 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_QPNP_SPMI > >>>> + tristate "Qualcomm QPNP SPMI PMIC" > >>>> + depends on ARCH_QCOM || COMPILE_TEST > >>>> + depends on OF > >>>> + select MFD_CORE > >>>> + select REGMAP_SPMI > >>>> + help > >>>> + This enables support for the Qualcomm QPNP SPMI 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. > >>> > > <snip> > > >>> > >>>> + * 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_address.h> > >>>> +#include <linux/slab.h> > >>>> +#include <linux/mfd/core.h> > >>>> + > >>>> +#define QPNP_RESOURCE_SIZE 256 > >>>> + > >>>> +static const struct regmap_config qpnp_regmap_config = { > >>>> + .reg_bits = 16, > >>>> + .val_bits = 8, > >>>> + .max_register = 0xffff, > >>>> +}; > >>>> + > >>>> +static int qpnp_index_to_resource(struct device_node *np, int index, > >>>> + struct resource *res) > >>>> +{ > >>>> + const char *name = NULL; > >>>> + const __be32 *addrp; > >>>> + u64 addr; > >>>> + > >>>> + addrp = of_get_address(np, index, NULL, NULL); > >>>> + if (!addrp) > >>>> + return -EINVAL; > >>>> + > >>>> + addr = of_read_number(addrp, 1); > >>>> + if (addr == OF_BAD_ADDR) > >>>> + return -EINVAL; > >>>> + > >>>> + of_property_read_string_index(np, "reg-names", index, &name); > >>>> + > >>>> + res->start = addr; > >>>> + res->end = addr + QPNP_RESOURCE_SIZE - 1; > >>>> + res->flags = IORESOURCE_REG; > >>>> + res->name = name ? name : np->name; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child) > >>>> +{ > >>>> + struct mfd_cell cell = {}; > >>>> + struct resource *res, *r; > >>>> + int num_resources = 0; > >>>> + const char *compat; > >>>> + int ret, i; > >>>> + > >>>> + compat = of_get_property(child, "compatible", NULL); > >>>> + if (!compat) > >>>> + return -ENODEV; > >>>> + > >>>> + while (of_get_address(child, num_resources, NULL, NULL)) > >>>> + num_resources++; > >>>> + > >>>> + if (!num_resources) > >>>> + return -ENODEV; > >>>> + > >>>> + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL); > >>>> + if (!res) > >>>> + return -ENOMEM; > >>>> + > >>>> + r = res; > >>>> + for (i = 0; i < num_resources; i++, r++) > >>>> + qpnp_index_to_resource(child, i, r); > >>>> + > >>>> + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid, > >>>> + (u16)res[0].start, child->name); > >>>> + cell.of_compatible = compat; > >>>> + cell.num_resources = num_resources; > >>>> + cell.resources = res; > >>>> + > >>>> + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1, > >>>> + NULL, 0, NULL); > >>>> + > >>>> + kfree(res); > >>>> + kfree(cell.name); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static int qpnp_probe(struct spmi_device *sdev) > >>>> +{ > >>>> + struct device_node *root = sdev->dev.of_node; > >>>> + struct device_node *child; > >>>> + struct regmap *regmap; > >>>> + > >>>> + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config); > >>>> + if (IS_ERR(regmap)) > >>>> + return PTR_ERR(regmap); > >>>> + > >>>> + for_each_available_child_of_node(root, child) > >>>> + qpnp_add_device(sdev, child); > >>> > >>> This entire driver looks like a re-write of of_platform_populate(). > >>> > >>> Why? > >> > >> of_platform_populate is not used because the PMIC function resources are > >> non-translatable. You can see that the resources are of type > >> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO. > >> > >> The whole point of this mfd driver is to parse devicetree to prepare > >> resources for every child and create platform device for it through > >> mfd_add_devices(). Then the PMIC function driver got its resources and > >> use them as register addresses passed to regmap. These register accesses > >> hits the SPMI controller which is the physical interface between PMIC's > >> and SoC. > > > > I can't help but think that if this is required, it should be part of > > the core OF code, rather than doing your own thing which looks > > frighteningly like existing framework functionality. > > does it make sense to have a common of_mfd code for devicetree parsing? > Is that discussed already? I mean presently the mfd_cell resources and > number of resources are passed by the mfd_add_devices users. Is it > possible to have common code which parses devicetree sub-nodes of the > parent device_node and fill resources/create platform devices for them. I'm not sure it does. Normally users _either_ represent devices in MFD cells from within the driver _or_ populate using existing DT interfaces i.e. of_platform_populate(). This is the first time I've seen someone attempt to parse the entire MFD node structure from within a driver. After searching for some documentation to try and figure this out, I noticed that you've also missed a patch from your set: mfd: qpnp-spmi: document DT bindings for Qualcomm QPNP PMICs ... which might help enlighten the DT guys.
Hi Lee, On 07/11/2014 12:07 PM, Lee Jones wrote: > On Thu, 10 Jul 2014, Stanimir Varbanov wrote: >>>>>> The Qualcomm QPNP PMIC chips 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: Stanimir Varbanov <svarbanov@mm-sol.com> >>>>>> --- >>>>>> drivers/mfd/Kconfig | 15 ++++++ >>>>>> drivers/mfd/Makefile | 1 + >>>>>> drivers/mfd/qpnp-spmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> 3 files changed, 145 insertions(+), 0 deletions(-) >>>>>> create mode 100644 drivers/mfd/qpnp-spmi.c >>>>>> >>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>>> index ee8204c..258b733 100644 >>>>>> --- a/drivers/mfd/Kconfig >>>>>> +++ b/drivers/mfd/Kconfig >>>>>> @@ -524,6 +524,21 @@ 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_QPNP_SPMI >>>>>> + tristate "Qualcomm QPNP SPMI PMIC" >>>>>> + depends on ARCH_QCOM || COMPILE_TEST >>>>>> + depends on OF >>>>>> + select MFD_CORE >>>>>> + select REGMAP_SPMI >>>>>> + help >>>>>> + This enables support for the Qualcomm QPNP SPMI 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. >>>>> >> >> <snip> >> >>>>> >>>>>> + * 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_address.h> >>>>>> +#include <linux/slab.h> >>>>>> +#include <linux/mfd/core.h> >>>>>> + >>>>>> +#define QPNP_RESOURCE_SIZE 256 >>>>>> + >>>>>> +static const struct regmap_config qpnp_regmap_config = { >>>>>> + .reg_bits = 16, >>>>>> + .val_bits = 8, >>>>>> + .max_register = 0xffff, >>>>>> +}; >>>>>> + >>>>>> +static int qpnp_index_to_resource(struct device_node *np, int index, >>>>>> + struct resource *res) >>>>>> +{ >>>>>> + const char *name = NULL; >>>>>> + const __be32 *addrp; >>>>>> + u64 addr; >>>>>> + >>>>>> + addrp = of_get_address(np, index, NULL, NULL); >>>>>> + if (!addrp) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + addr = of_read_number(addrp, 1); >>>>>> + if (addr == OF_BAD_ADDR) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + of_property_read_string_index(np, "reg-names", index, &name); >>>>>> + >>>>>> + res->start = addr; >>>>>> + res->end = addr + QPNP_RESOURCE_SIZE - 1; >>>>>> + res->flags = IORESOURCE_REG; >>>>>> + res->name = name ? name : np->name; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child) >>>>>> +{ >>>>>> + struct mfd_cell cell = {}; >>>>>> + struct resource *res, *r; >>>>>> + int num_resources = 0; >>>>>> + const char *compat; >>>>>> + int ret, i; >>>>>> + >>>>>> + compat = of_get_property(child, "compatible", NULL); >>>>>> + if (!compat) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + while (of_get_address(child, num_resources, NULL, NULL)) >>>>>> + num_resources++; >>>>>> + >>>>>> + if (!num_resources) >>>>>> + return -ENODEV; >>>>>> + >>>>>> + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL); >>>>>> + if (!res) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + r = res; >>>>>> + for (i = 0; i < num_resources; i++, r++) >>>>>> + qpnp_index_to_resource(child, i, r); >>>>>> + >>>>>> + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid, >>>>>> + (u16)res[0].start, child->name); >>>>>> + cell.of_compatible = compat; >>>>>> + cell.num_resources = num_resources; >>>>>> + cell.resources = res; >>>>>> + >>>>>> + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1, >>>>>> + NULL, 0, NULL); >>>>>> + >>>>>> + kfree(res); >>>>>> + kfree(cell.name); >>>>>> + >>>>>> + return ret; >>>>>> +} >>>>>> + >>>>>> +static int qpnp_probe(struct spmi_device *sdev) >>>>>> +{ >>>>>> + struct device_node *root = sdev->dev.of_node; >>>>>> + struct device_node *child; >>>>>> + struct regmap *regmap; >>>>>> + >>>>>> + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config); >>>>>> + if (IS_ERR(regmap)) >>>>>> + return PTR_ERR(regmap); >>>>>> + >>>>>> + for_each_available_child_of_node(root, child) >>>>>> + qpnp_add_device(sdev, child); >>>>> >>>>> This entire driver looks like a re-write of of_platform_populate(). >>>>> >>>>> Why? >>>> >>>> of_platform_populate is not used because the PMIC function resources are >>>> non-translatable. You can see that the resources are of type >>>> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO. >>>> >>>> The whole point of this mfd driver is to parse devicetree to prepare >>>> resources for every child and create platform device for it through >>>> mfd_add_devices(). Then the PMIC function driver got its resources and >>>> use them as register addresses passed to regmap. These register accesses >>>> hits the SPMI controller which is the physical interface between PMIC's >>>> and SoC. >>> >>> I can't help but think that if this is required, it should be part of >>> the core OF code, rather than doing your own thing which looks >>> frighteningly like existing framework functionality. >> >> does it make sense to have a common of_mfd code for devicetree parsing? >> Is that discussed already? I mean presently the mfd_cell resources and >> number of resources are passed by the mfd_add_devices users. Is it >> possible to have common code which parses devicetree sub-nodes of the >> parent device_node and fill resources/create platform devices for them. > > I'm not sure it does. Normally users _either_ represent devices in > MFD cells from within the driver _or_ populate using existing DT > interfaces i.e. of_platform_populate(). This is the first time I've > seen someone attempt to parse the entire MFD node structure from > within a driver. Our goal is to use common mfd driver for various Qualcomm PMIC from the same PMIC generation (codenamed QPNP). Presently we have good abstraction of the register manipulation functions through regmap-spmi layer. Using the devicetree to represent different peripherals base addresses (reg property) gives us a benefit by avoiding huge header files in linux/mfd and make flexible transition to new PMIC's which just change the base addresses but keep the IP block the same. This way we should change the devicetree binding without touch the drivers. > > After searching for some documentation to try and figure this out, I > noticed that you've also missed a patch from your set: > > mfd: qpnp-spmi: document DT bindings for Qualcomm QPNP PMICs I sent a new version without RFC tag which includes this binding document [1]. > > ... which might help enlighten the DT guys. >
On Mon, 14 Jul 2014, Stanimir Varbanov wrote: > On 07/11/2014 12:07 PM, Lee Jones wrote: > > On Thu, 10 Jul 2014, Stanimir Varbanov wrote: > >>>>>> The Qualcomm QPNP PMIC chips 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: Stanimir Varbanov <svarbanov@mm-sol.com> > >>>>>> --- > >>>>>> drivers/mfd/Kconfig | 15 ++++++ > >>>>>> drivers/mfd/Makefile | 1 + > >>>>>> drivers/mfd/qpnp-spmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++ > >>>>>> 3 files changed, 145 insertions(+), 0 deletions(-) > >>>>>> create mode 100644 drivers/mfd/qpnp-spmi.c > >>>>>> > >>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > >>>>>> index ee8204c..258b733 100644 > >>>>>> --- a/drivers/mfd/Kconfig > >>>>>> +++ b/drivers/mfd/Kconfig > >>>>>> @@ -524,6 +524,21 @@ 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_QPNP_SPMI > >>>>>> + tristate "Qualcomm QPNP SPMI PMIC" > >>>>>> + depends on ARCH_QCOM || COMPILE_TEST > >>>>>> + depends on OF > >>>>>> + select MFD_CORE > >>>>>> + select REGMAP_SPMI > >>>>>> + help > >>>>>> + This enables support for the Qualcomm QPNP SPMI 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. > >>>>> > >> > >> <snip> > >> > >>>>> > >>>>>> + * 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_address.h> > >>>>>> +#include <linux/slab.h> > >>>>>> +#include <linux/mfd/core.h> > >>>>>> + > >>>>>> +#define QPNP_RESOURCE_SIZE 256 > >>>>>> + > >>>>>> +static const struct regmap_config qpnp_regmap_config = { > >>>>>> + .reg_bits = 16, > >>>>>> + .val_bits = 8, > >>>>>> + .max_register = 0xffff, > >>>>>> +}; > >>>>>> + > >>>>>> +static int qpnp_index_to_resource(struct device_node *np, int index, > >>>>>> + struct resource *res) > >>>>>> +{ > >>>>>> + const char *name = NULL; > >>>>>> + const __be32 *addrp; > >>>>>> + u64 addr; > >>>>>> + > >>>>>> + addrp = of_get_address(np, index, NULL, NULL); > >>>>>> + if (!addrp) > >>>>>> + return -EINVAL; > >>>>>> + > >>>>>> + addr = of_read_number(addrp, 1); > >>>>>> + if (addr == OF_BAD_ADDR) > >>>>>> + return -EINVAL; > >>>>>> + > >>>>>> + of_property_read_string_index(np, "reg-names", index, &name); > >>>>>> + > >>>>>> + res->start = addr; > >>>>>> + res->end = addr + QPNP_RESOURCE_SIZE - 1; > >>>>>> + res->flags = IORESOURCE_REG; > >>>>>> + res->name = name ? name : np->name; > >>>>>> + > >>>>>> + return 0; > >>>>>> +} > >>>>>> + > >>>>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child) > >>>>>> +{ > >>>>>> + struct mfd_cell cell = {}; > >>>>>> + struct resource *res, *r; > >>>>>> + int num_resources = 0; > >>>>>> + const char *compat; > >>>>>> + int ret, i; > >>>>>> + > >>>>>> + compat = of_get_property(child, "compatible", NULL); > >>>>>> + if (!compat) > >>>>>> + return -ENODEV; > >>>>>> + > >>>>>> + while (of_get_address(child, num_resources, NULL, NULL)) > >>>>>> + num_resources++; > >>>>>> + > >>>>>> + if (!num_resources) > >>>>>> + return -ENODEV; > >>>>>> + > >>>>>> + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL); > >>>>>> + if (!res) > >>>>>> + return -ENOMEM; > >>>>>> + > >>>>>> + r = res; > >>>>>> + for (i = 0; i < num_resources; i++, r++) > >>>>>> + qpnp_index_to_resource(child, i, r); > >>>>>> + > >>>>>> + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid, > >>>>>> + (u16)res[0].start, child->name); > >>>>>> + cell.of_compatible = compat; > >>>>>> + cell.num_resources = num_resources; > >>>>>> + cell.resources = res; > >>>>>> + > >>>>>> + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1, > >>>>>> + NULL, 0, NULL); > >>>>>> + > >>>>>> + kfree(res); > >>>>>> + kfree(cell.name); > >>>>>> + > >>>>>> + return ret; > >>>>>> +} > >>>>>> + > >>>>>> +static int qpnp_probe(struct spmi_device *sdev) > >>>>>> +{ > >>>>>> + struct device_node *root = sdev->dev.of_node; > >>>>>> + struct device_node *child; > >>>>>> + struct regmap *regmap; > >>>>>> + > >>>>>> + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config); > >>>>>> + if (IS_ERR(regmap)) > >>>>>> + return PTR_ERR(regmap); > >>>>>> + > >>>>>> + for_each_available_child_of_node(root, child) > >>>>>> + qpnp_add_device(sdev, child); > >>>>> > >>>>> This entire driver looks like a re-write of of_platform_populate(). > >>>>> > >>>>> Why? > >>>> > >>>> of_platform_populate is not used because the PMIC function resources are > >>>> non-translatable. You can see that the resources are of type > >>>> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO. > >>>> > >>>> The whole point of this mfd driver is to parse devicetree to prepare > >>>> resources for every child and create platform device for it through > >>>> mfd_add_devices(). Then the PMIC function driver got its resources and > >>>> use them as register addresses passed to regmap. These register accesses > >>>> hits the SPMI controller which is the physical interface between PMIC's > >>>> and SoC. > >>> > >>> I can't help but think that if this is required, it should be part of > >>> the core OF code, rather than doing your own thing which looks > >>> frighteningly like existing framework functionality. > >> > >> does it make sense to have a common of_mfd code for devicetree parsing? > >> Is that discussed already? I mean presently the mfd_cell resources and > >> number of resources are passed by the mfd_add_devices users. Is it > >> possible to have common code which parses devicetree sub-nodes of the > >> parent device_node and fill resources/create platform devices for them. > > > > I'm not sure it does. Normally users _either_ represent devices in > > MFD cells from within the driver _or_ populate using existing DT > > interfaces i.e. of_platform_populate(). This is the first time I've > > seen someone attempt to parse the entire MFD node structure from > > within a driver. > > Our goal is to use common mfd driver for various Qualcomm PMIC from the > same PMIC generation (codenamed QPNP). Presently we have good > abstraction of the register manipulation functions through regmap-spmi > layer. Using the devicetree to represent different peripherals base > addresses (reg property) gives us a benefit by avoiding huge header > files in linux/mfd and make flexible transition to new PMIC's which just > change the base addresses but keep the IP block the same. This way we > should change the devicetree binding without touch the drivers. I'm still not convinced that you need to write your own parser. If you're concerned about not translating the reg property, use (or don't) the 'ranges' property accordingly. > > After searching for some documentation to try and figure this out, I > > noticed that you've also missed a patch from your set: > > > > mfd: qpnp-spmi: document DT bindings for Qualcomm QPNP PMICs > > I sent a new version without RFC tag which includes this binding > document [1]. > > > > > ... which might help enlighten the DT guys. > > >
On 07/14/2014 05:03 PM, Lee Jones wrote: > On Mon, 14 Jul 2014, Stanimir Varbanov wrote: >> On 07/11/2014 12:07 PM, Lee Jones wrote: >>> On Thu, 10 Jul 2014, Stanimir Varbanov wrote: >>>>>>>> The Qualcomm QPNP PMIC chips 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: Stanimir Varbanov <svarbanov@mm-sol.com> >>>>>>>> --- >>>>>>>> drivers/mfd/Kconfig | 15 ++++++ >>>>>>>> drivers/mfd/Makefile | 1 + >>>>>>>> drivers/mfd/qpnp-spmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> 3 files changed, 145 insertions(+), 0 deletions(-) >>>>>>>> create mode 100644 drivers/mfd/qpnp-spmi.c >>>>>>>> >>>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig >>>>>>>> index ee8204c..258b733 100644 >>>>>>>> --- a/drivers/mfd/Kconfig >>>>>>>> +++ b/drivers/mfd/Kconfig >>>>>>>> @@ -524,6 +524,21 @@ 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_QPNP_SPMI >>>>>>>> + tristate "Qualcomm QPNP SPMI PMIC" >>>>>>>> + depends on ARCH_QCOM || COMPILE_TEST >>>>>>>> + depends on OF >>>>>>>> + select MFD_CORE >>>>>>>> + select REGMAP_SPMI >>>>>>>> + help >>>>>>>> + This enables support for the Qualcomm QPNP SPMI 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. >>>>>>> >>>> >>>> <snip> >>>> >>>>>>> >>>>>>>> + * 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_address.h> >>>>>>>> +#include <linux/slab.h> >>>>>>>> +#include <linux/mfd/core.h> >>>>>>>> + >>>>>>>> +#define QPNP_RESOURCE_SIZE 256 >>>>>>>> + >>>>>>>> +static const struct regmap_config qpnp_regmap_config = { >>>>>>>> + .reg_bits = 16, >>>>>>>> + .val_bits = 8, >>>>>>>> + .max_register = 0xffff, >>>>>>>> +}; >>>>>>>> + >>>>>>>> +static int qpnp_index_to_resource(struct device_node *np, int index, >>>>>>>> + struct resource *res) >>>>>>>> +{ >>>>>>>> + const char *name = NULL; >>>>>>>> + const __be32 *addrp; >>>>>>>> + u64 addr; >>>>>>>> + >>>>>>>> + addrp = of_get_address(np, index, NULL, NULL); >>>>>>>> + if (!addrp) >>>>>>>> + return -EINVAL; >>>>>>>> + >>>>>>>> + addr = of_read_number(addrp, 1); >>>>>>>> + if (addr == OF_BAD_ADDR) >>>>>>>> + return -EINVAL; >>>>>>>> + >>>>>>>> + of_property_read_string_index(np, "reg-names", index, &name); >>>>>>>> + >>>>>>>> + res->start = addr; >>>>>>>> + res->end = addr + QPNP_RESOURCE_SIZE - 1; >>>>>>>> + res->flags = IORESOURCE_REG; >>>>>>>> + res->name = name ? name : np->name; >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>>> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child) >>>>>>>> +{ >>>>>>>> + struct mfd_cell cell = {}; >>>>>>>> + struct resource *res, *r; >>>>>>>> + int num_resources = 0; >>>>>>>> + const char *compat; >>>>>>>> + int ret, i; >>>>>>>> + >>>>>>>> + compat = of_get_property(child, "compatible", NULL); >>>>>>>> + if (!compat) >>>>>>>> + return -ENODEV; >>>>>>>> + >>>>>>>> + while (of_get_address(child, num_resources, NULL, NULL)) >>>>>>>> + num_resources++; >>>>>>>> + >>>>>>>> + if (!num_resources) >>>>>>>> + return -ENODEV; >>>>>>>> + >>>>>>>> + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL); >>>>>>>> + if (!res) >>>>>>>> + return -ENOMEM; >>>>>>>> + >>>>>>>> + r = res; >>>>>>>> + for (i = 0; i < num_resources; i++, r++) >>>>>>>> + qpnp_index_to_resource(child, i, r); >>>>>>>> + >>>>>>>> + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid, >>>>>>>> + (u16)res[0].start, child->name); >>>>>>>> + cell.of_compatible = compat; >>>>>>>> + cell.num_resources = num_resources; >>>>>>>> + cell.resources = res; >>>>>>>> + >>>>>>>> + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1, >>>>>>>> + NULL, 0, NULL); >>>>>>>> + >>>>>>>> + kfree(res); >>>>>>>> + kfree(cell.name); >>>>>>>> + >>>>>>>> + return ret; >>>>>>>> +} >>>>>>>> + >>>>>>>> +static int qpnp_probe(struct spmi_device *sdev) >>>>>>>> +{ >>>>>>>> + struct device_node *root = sdev->dev.of_node; >>>>>>>> + struct device_node *child; >>>>>>>> + struct regmap *regmap; >>>>>>>> + >>>>>>>> + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config); >>>>>>>> + if (IS_ERR(regmap)) >>>>>>>> + return PTR_ERR(regmap); >>>>>>>> + >>>>>>>> + for_each_available_child_of_node(root, child) >>>>>>>> + qpnp_add_device(sdev, child); >>>>>>> >>>>>>> This entire driver looks like a re-write of of_platform_populate(). >>>>>>> >>>>>>> Why? >>>>>> >>>>>> of_platform_populate is not used because the PMIC function resources are >>>>>> non-translatable. You can see that the resources are of type >>>>>> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO. >>>>>> >>>>>> The whole point of this mfd driver is to parse devicetree to prepare >>>>>> resources for every child and create platform device for it through >>>>>> mfd_add_devices(). Then the PMIC function driver got its resources and >>>>>> use them as register addresses passed to regmap. These register accesses >>>>>> hits the SPMI controller which is the physical interface between PMIC's >>>>>> and SoC. >>>>> >>>>> I can't help but think that if this is required, it should be part of >>>>> the core OF code, rather than doing your own thing which looks >>>>> frighteningly like existing framework functionality. >>>> >>>> does it make sense to have a common of_mfd code for devicetree parsing? >>>> Is that discussed already? I mean presently the mfd_cell resources and >>>> number of resources are passed by the mfd_add_devices users. Is it >>>> possible to have common code which parses devicetree sub-nodes of the >>>> parent device_node and fill resources/create platform devices for them. >>> >>> I'm not sure it does. Normally users _either_ represent devices in >>> MFD cells from within the driver _or_ populate using existing DT >>> interfaces i.e. of_platform_populate(). This is the first time I've >>> seen someone attempt to parse the entire MFD node structure from >>> within a driver. >> >> Our goal is to use common mfd driver for various Qualcomm PMIC from the >> same PMIC generation (codenamed QPNP). Presently we have good >> abstraction of the register manipulation functions through regmap-spmi >> layer. Using the devicetree to represent different peripherals base >> addresses (reg property) gives us a benefit by avoiding huge header >> files in linux/mfd and make flexible transition to new PMIC's which just >> change the base addresses but keep the IP block the same. This way we >> should change the devicetree binding without touch the drivers. > > I'm still not convinced that you need to write your own parser. If > you're concerned about not translating the reg property, use (or > don't) the 'ranges' property accordingly. Unfortunately "ranges" property doesn't help here. The pmic addresses parsed from "reg" properties are SPMI addresses (similar to I2C addresses).
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index ee8204c..258b733 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -524,6 +524,21 @@ 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_QPNP_SPMI + tristate "Qualcomm QPNP SPMI PMIC" + depends on ARCH_QCOM || COMPILE_TEST + depends on OF + select MFD_CORE + select REGMAP_SPMI + help + This enables support for the Qualcomm QPNP SPMI 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 QPNP SPMI PMIC + series as a module. The module will be called "qpnp-spmi". + config MFD_RDC321X tristate "RDC R-321x southbridge" select MFD_CORE diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 8afedba..31833d7 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -153,6 +153,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_QPNP_SPMI) += qpnp-spmi.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/qpnp-spmi.c b/drivers/mfd/qpnp-spmi.c new file mode 100644 index 0000000..efd7d3e --- /dev/null +++ b/drivers/mfd/qpnp-spmi.c @@ -0,0 +1,129 @@ +/* Copyright (c) 2014, 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_address.h> +#include <linux/slab.h> +#include <linux/mfd/core.h> + +#define QPNP_RESOURCE_SIZE 256 + +static const struct regmap_config qpnp_regmap_config = { + .reg_bits = 16, + .val_bits = 8, + .max_register = 0xffff, +}; + +static int qpnp_index_to_resource(struct device_node *np, int index, + struct resource *res) +{ + const char *name = NULL; + const __be32 *addrp; + u64 addr; + + addrp = of_get_address(np, index, NULL, NULL); + if (!addrp) + return -EINVAL; + + addr = of_read_number(addrp, 1); + if (addr == OF_BAD_ADDR) + return -EINVAL; + + of_property_read_string_index(np, "reg-names", index, &name); + + res->start = addr; + res->end = addr + QPNP_RESOURCE_SIZE - 1; + res->flags = IORESOURCE_REG; + res->name = name ? name : np->name; + + return 0; +} + +static int qpnp_add_device(struct spmi_device *root, struct device_node *child) +{ + struct mfd_cell cell = {}; + struct resource *res, *r; + int num_resources = 0; + const char *compat; + int ret, i; + + compat = of_get_property(child, "compatible", NULL); + if (!compat) + return -ENODEV; + + while (of_get_address(child, num_resources, NULL, NULL)) + num_resources++; + + if (!num_resources) + return -ENODEV; + + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL); + if (!res) + return -ENOMEM; + + r = res; + for (i = 0; i < num_resources; i++, r++) + qpnp_index_to_resource(child, i, r); + + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid, + (u16)res[0].start, child->name); + cell.of_compatible = compat; + cell.num_resources = num_resources; + cell.resources = res; + + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1, + NULL, 0, NULL); + + kfree(res); + kfree(cell.name); + + return ret; +} + +static int qpnp_probe(struct spmi_device *sdev) +{ + struct device_node *root = sdev->dev.of_node; + struct device_node *child; + struct regmap *regmap; + + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + for_each_available_child_of_node(root, child) + qpnp_add_device(sdev, child); + + return 0; +} + +static void qpnp_remove(struct spmi_device *sdev) +{ + mfd_remove_devices(&sdev->dev); +} + +static const struct of_device_id qpnp_id_table[] = { + { .compatible = "qcom,qpnp-spmi", }, + { }, +}; +MODULE_DEVICE_TABLE(of, qpnp_id_table); + +static struct spmi_driver qpnp_driver = { + .probe = qpnp_probe, + .remove = qpnp_remove, + .driver = { + .name = "qpnp-spmi", + .of_match_table = qpnp_id_table, + }, +}; +module_spmi_driver(qpnp_driver);