| Message ID | 1406205921-7452-2-git-send-email-svarbanov@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/24/2014 05:45 AM, Stanimir Varbanov wrote: > From: Josh Cartwright <joshc@codeaurora.org> > > The Qualcomm SPMI 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> > Acked-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/mfd/Kconfig | 16 +++++++++++ > drivers/mfd/Makefile | 1 + > drivers/mfd/pm8xxx-spmi.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ Would it be possible to rename this driver: qcom-spmi-pmic.c? The driver will be supporting several PMICs that do not fit the pm8xxx naming scheme. One of which is even specified in the compatible list of this driver (pma8084). There is presently downstream support for the following PMICs: PM8019, PM8110, PM8226, PM8841, PM8916, PM8941, PM8994, PMA8084, PMD9635, PMI8962, and PMI8994 [1]. Four of these do not fit the "PM8XXX" template. If we can agree on changing the file name for the driver, then all other instances of "pm8xxx" would need to be renamed in this patch series (e.g. config options, function names, struct names, DT binding documentation, etc.) It would probably be good to rename pm8xxx-ssbi.c to qcom-ssbi-pmic.c as well in order to maintain consistency. (...) > +static const struct regmap_config pm8xxx_regmap_config = { > + .reg_bits = 16, > + .val_bits = 8, > + .max_register = 0xffff, Can you please add the following line here? .fast_io = true; This will cause a spinlock to be held during SPMI transactions instead of a mutex lock. This is needed because several downstream peripheral drivers need to make SPMI read and write calls from atomic context. I have commented on this point in a previous thread with specific examples [2]. > +}; (...) > +static const struct of_device_id pm8xxx_id_table[] = { > + { .compatible = "qcom,pm8941" }, > + { .compatible = "qcom,pm8841" }, > + { .compatible = "qcom,pma8084" }, Would it be possible to add a generic compatible string as well? Perhaps something like "qcom,spmi-pmic" could be used. This driver is not doing anything with the PMIC specific compatible strings. The generic compatible string could be specified in device tree in conjunction a PMIC specific string that is not present in this list. That way, this driver file would not need to be touched as new PMIC chips are introduced unless some weird workaround is needed. In that case, the PMIC specific compatible string could be added to the list along with whatever special function is needed to handle it. Take care, David Collins [1]: https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/arch/arm/boot/dts/qcom?h=msm-3.10 [2]: https://lkml.org/lkml/2014/4/25/720
Hi David, Thanks for the comments! On 07/30/2014 12:54 AM, David Collins wrote: > On 07/24/2014 05:45 AM, Stanimir Varbanov wrote: >> From: Josh Cartwright <joshc@codeaurora.org> >> >> The Qualcomm SPMI 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> >> Acked-by: Lee Jones <lee.jones@linaro.org> >> --- >> drivers/mfd/Kconfig | 16 +++++++++++ >> drivers/mfd/Makefile | 1 + >> drivers/mfd/pm8xxx-spmi.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ > > Would it be possible to rename this driver: qcom-spmi-pmic.c? The driver > will be supporting several PMICs that do not fit the pm8xxx naming scheme. > One of which is even specified in the compatible list of this driver > (pma8084). There is presently downstream support for the following PMICs: > PM8019, PM8110, PM8226, PM8841, PM8916, PM8941, PM8994, PMA8084, PMD9635, > PMI8962, and PMI8994 [1]. Four of these do not fit the "PM8XXX" template. I haven't strong opinion on the file names. The qcom prefix is the one which annoying me. If you look at /drivers/mfd the company name prefixes are very few. The *compatible* strings are the important thing here. So If MFD maintainer is fine with this name I'm fine too. > > If we can agree on changing the file name for the driver, then all other > instances of "pm8xxx" would need to be renamed in this patch series (e.g. > config options, function names, struct names, DT binding documentation, etc.) > > It would probably be good to rename pm8xxx-ssbi.c to qcom-ssbi-pmic.c as > well in order to maintain consistency. > > (...) >> +static const struct regmap_config pm8xxx_regmap_config = { >> + .reg_bits = 16, >> + .val_bits = 8, >> + .max_register = 0xffff, > > Can you please add the following line here? > > .fast_io = true; > > This will cause a spinlock to be held during SPMI transactions instead of > a mutex lock. This is needed because several downstream peripheral > drivers need to make SPMI read and write calls from atomic context. I > have commented on this point in a previous thread with specific examples [2]. OK, I understand the need of atomic context, but pmic_arb_read_cmd() and pmic_arb_write_cmd() functions use raw_spin_lock_irqsave already. Isn't those locks enough? > >> +}; > > (...) >> +static const struct of_device_id pm8xxx_id_table[] = { >> + { .compatible = "qcom,pm8941" }, >> + { .compatible = "qcom,pm8841" }, >> + { .compatible = "qcom,pma8084" }, > > Would it be possible to add a generic compatible string as well? Perhaps > something like "qcom,spmi-pmic" could be used. This driver is not doing > anything with the PMIC specific compatible strings. The generic > compatible string could be specified in device tree in conjunction a PMIC > specific string that is not present in this list. That way, this driver > file would not need to be touched as new PMIC chips are introduced unless > some weird workaround is needed. In that case, the PMIC specific > compatible string could be added to the list along with whatever special > function is needed to handle it. OK, I'm fine with this suggestion.
On 07/31/2014 01:48 AM, Stanimir Varbanov wrote: > Hi David, > > Thanks for the comments! > > On 07/30/2014 12:54 AM, David Collins wrote: >> On 07/24/2014 05:45 AM, Stanimir Varbanov wrote: >>> From: Josh Cartwright <joshc@codeaurora.org> >>> >>> The Qualcomm SPMI 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. (...) >>> +static const struct regmap_config pm8xxx_regmap_config = { >>> + .reg_bits = 16, >>> + .val_bits = 8, >>> + .max_register = 0xffff, >> >> Can you please add the following line here? >> >> .fast_io = true; >> >> This will cause a spinlock to be held during SPMI transactions instead of >> a mutex lock. This is needed because several downstream peripheral >> drivers need to make SPMI read and write calls from atomic context. I >> have commented on this point in a previous thread with specific examples [2]. > > OK, I understand the need of atomic context, but pmic_arb_read_cmd() and > pmic_arb_write_cmd() functions use raw_spin_lock_irqsave already. Isn't > those locks enough? No, that isn't sufficient. The problem is that the peripheral driver would already be in atomic context at the time that it needs to perform an SPMI read or write via regmap_read() or regmap_write() respectively. These regmap calls would take a mutex lock if fast_io == false. This is not allowed since calling a sleepable function in atomic context can lead to deadlock. Take care, David
On 07/31/2014 11:48 AM, Stanimir Varbanov wrote: > Hi David, > > Thanks for the comments! > > On 07/30/2014 12:54 AM, David Collins wrote: >> On 07/24/2014 05:45 AM, Stanimir Varbanov wrote: >>> From: Josh Cartwright <joshc@codeaurora.org> >>> >>> The Qualcomm SPMI 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> >>> Acked-by: Lee Jones <lee.jones@linaro.org> >>> --- >>> drivers/mfd/Kconfig | 16 +++++++++++ >>> drivers/mfd/Makefile | 1 + >>> drivers/mfd/pm8xxx-spmi.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ >> >> Would it be possible to rename this driver: qcom-spmi-pmic.c? The driver >> will be supporting several PMICs that do not fit the pm8xxx naming scheme. >> One of which is even specified in the compatible list of this driver >> (pma8084). There is presently downstream support for the following PMICs: >> PM8019, PM8110, PM8226, PM8841, PM8916, PM8941, PM8994, PMA8084, PMD9635, >> PMI8962, and PMI8994 [1]. Four of these do not fit the "PM8XXX" template. > > I haven't strong opinion on the file names. The qcom prefix is the one > which annoying me. If you look at /drivers/mfd the company name prefixes > are very few. > > The *compatible* strings are the important thing here. So If MFD > maintainer is fine with this name I'm fine too. Lee, are you OK with suggested names qcom-spmi-pmic and qcom-ssbi-pmic?
On Fri, 01 Aug 2014, Stanimir Varbanov wrote: > On 07/31/2014 11:48 AM, Stanimir Varbanov wrote: > > On 07/30/2014 12:54 AM, David Collins wrote: > >> On 07/24/2014 05:45 AM, Stanimir Varbanov wrote: > >>> From: Josh Cartwright <joshc@codeaurora.org> > >>> > >>> The Qualcomm SPMI 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> > >>> Acked-by: Lee Jones <lee.jones@linaro.org> > >>> --- > >>> drivers/mfd/Kconfig | 16 +++++++++++ > >>> drivers/mfd/Makefile | 1 + > >>> drivers/mfd/pm8xxx-spmi.c | 65 +++++++++++++++++++++++++++++++++++++++++++++ > >> > >> Would it be possible to rename this driver: qcom-spmi-pmic.c? The driver > >> will be supporting several PMICs that do not fit the pm8xxx naming scheme. > >> One of which is even specified in the compatible list of this driver > >> (pma8084). There is presently downstream support for the following PMICs: > >> PM8019, PM8110, PM8226, PM8841, PM8916, PM8941, PM8994, PMA8084, PMD9635, > >> PMI8962, and PMI8994 [1]. Four of these do not fit the "PM8XXX" template. > > > > I haven't strong opinion on the file names. The qcom prefix is the one > > which annoying me. If you look at /drivers/mfd the company name prefixes > > are very few. > > > > The *compatible* strings are the important thing here. So If MFD > > maintainer is fine with this name I'm fine too. > > Lee, are you OK with suggested names qcom-spmi-pmic and qcom-ssbi-pmic? Sounds fine to me.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 6cc4b6a..122e2d9 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -524,6 +524,22 @@ 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_PM8XXX_SPMI + tristate "Qualcomm PM8XXX SPMI PMICs" + depends on ARCH_QCOM || COMPILE_TEST + depends on OF + depends on SPMI + select MFD_PM8XXX + select REGMAP_SPMI + help + This enables support for the Qualcomm PM8XXX 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 PM8XXX SPMI PMIC + series as a module. The module will be called "pm8xxx-spmi". + config MFD_RDC321X tristate "RDC R-321x southbridge" select MFD_CORE diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 8afedba..93d82cc 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_PM8XXX_SPMI) += pm8xxx-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/pm8xxx-spmi.c b/drivers/mfd/pm8xxx-spmi.c new file mode 100644 index 0000000..16335a1 --- /dev/null +++ b/drivers/mfd/pm8xxx-spmi.c @@ -0,0 +1,65 @@ +/* + * 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_platform.h> + +static const struct regmap_config pm8xxx_regmap_config = { + .reg_bits = 16, + .val_bits = 8, + .max_register = 0xffff, +}; + +static int pm8xxx_probe(struct spmi_device *sdev) +{ + struct device_node *root = sdev->dev.of_node; + struct regmap *regmap; + + regmap = devm_regmap_init_spmi_ext(sdev, &pm8xxx_regmap_config); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + + return of_platform_populate(root, NULL, NULL, &sdev->dev); +} + +static void pm8xxx_remove(struct spmi_device *sdev) +{ + of_platform_depopulate(&sdev->dev); +} + +static const struct of_device_id pm8xxx_id_table[] = { + { .compatible = "qcom,pm8941" }, + { .compatible = "qcom,pm8841" }, + { .compatible = "qcom,pma8084" }, + { } +}; +MODULE_DEVICE_TABLE(of, pm8xxx_id_table); + +static struct spmi_driver pm8xxx_driver = { + .probe = pm8xxx_probe, + .remove = pm8xxx_remove, + .driver = { + .name = "pm8xxx-spmi", + .of_match_table = pm8xxx_id_table, + }, +}; +module_spmi_driver(pm8xxx_driver); + +MODULE_DESCRIPTION("PM8XXX SPMI PMIC driver"); +MODULE_ALIAS("spmi:pm8xxx-spmi"); +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Josh Cartwright <joshc@codeaurora.org>"); +MODULE_AUTHOR("Stanimir Varbanov <svarbanov@mm-sol.com>");