From mboxrd@z Thu Jan 1 00:00:00 1970 From: svarbanov@mm-sol.com (Stanimir Varbanov) Date: Thu, 31 Jul 2014 11:48:00 +0300 Subject: [PATCH v3 1/4] mfd: pm8xxx-spmi: add support for Qualcomm SPMI PMICs In-Reply-To: <53D8182A.5050204@codeaurora.org> References: <1406205921-7452-1-git-send-email-svarbanov@mm-sol.com> <1406205921-7452-2-git-send-email-svarbanov@mm-sol.com> <53D8182A.5050204@codeaurora.org> Message-ID: <53DA02C0.2050606@mm-sol.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >> >> 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 >> Signed-off-by: Stanimir Varbanov >> Acked-by: Lee Jones >> --- >> 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. -- regards, Stan