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:

quoted

From: Josh Cartwright <redacted>

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 <redacted>
Signed-off-by: Stanimir Varbanov <redacted>
Acked-by: Lee Jones <redacted>
---
 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.

(...)

quoted

+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?

quoted

+};

(...)

quoted

+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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html