Re: [PATCH v3 1/4] mfd: pm8xxx-spmi: add support for Qualcomm SPMI PMICs

[David Collins]

On 07/24/2014 05:45 AM, Stanimir Varbanov wrote:
> From: Josh Cartwright <joshc@xxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Stanimir Varbanov <svarbanov@xxxxxxxxxx>
> Acked-by: Lee Jones <lee.jones@xxxxxxxxxx>
> ---
>  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

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/