Message-ID: <20140424181829.GA19254@joshc.qualcomm.com>
Date:	Thu, 24 Apr 2014 13:18:29 -0500
From:	Josh Cartwright <joshc@...eaurora.org>
To:	Courtney Cavin <courtney.cavin@...ymobile.com>
Cc:	Samuel Ortiz <sameo@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	David Collins <collinsd@...eaurora.org>
Subject: Re: [PATCH 1/2] mfd: pm8x41: add support for Qualcomm 8x41 PMICs

On Wed, Apr 23, 2014 at 04:36:22PM -0700, Courtney Cavin wrote:
> On Wed, Apr 23, 2014 at 11:46:26PM +0200, Josh Cartwright wrote:
> > On Tue, Apr 22, 2014 at 05:31:49PM -0700, Courtney Cavin wrote:
[..]
> > One thing that I had meant to do is rename this thing.  Nothing about
> > this is PM8841/PM8941 specific at all.  It should apply equally to all
> > Qualcomm's PMICs which implement QPNP.
> > 
> > Perhaps a better name would be "qcom-pmic-qpnp".
>
> What's a QPNP?  Really.  I've heard you speak about it before as being a
> definition of the register layout for interrupts, but I have no
> documentation on it.

QPNP is effectively (as I explained before) a partitioning scheme for
dividing the SPMI extended register space up into logical pieces, and
set of fixed register locations/definitions within these regions, with
some of these regions specifically used for interrupt handling.

> I would argue here from my understanding that this driver isn't specific
> to QPNP either.  With that in mind we could just go with
> "qcom-pmic-spmi".  In fact just "spmi-ext" would not be incorrect, as
> this driver has little to do with PMICs at all.

I'm actually not opposed to either of those suggested names.

> My point here is that we can easily make this into something very
> generic, but that only causes problems in the future when it's not
> "generic enough", and we have to add quirks.

Yes, this is why I'd still like to require having the specific PMIC
listed in the slave node's compatible string in front of a "generic"
one.  Without a perfect crystal ball, it's the best we have.

> If in the future Qualcomm releases a pm8A41, and it's qpnp, but not
> spmi, or spmi, but not 'ext', then we need to either change this
> driver dramatically, or write a new one.  I like keeping this driver
> name specific to what we know it supports.  We can rename it in the
> future if deemed appropriate, but I'd rather not make it something
> that which turns out to be wrong at some later point.

I don't necessarily disagree with the strategy, however, if you take a
look at the downstream msm-3.10 tree[1], you'll see that there are
already quite a few other PMICs that could be made to leverage this
driver with likely no changes (downstream the equivalent is a dt node
tagged spmi-slave-container):

	$ git grep spmi-slave-container arch/arm/boot/dts
	arch/arm/boot/dts/qcom/msm-pm8019.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8019.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8110.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8110.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8226.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8226.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8841.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8841.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8916.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8916.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8941.dtsi:	spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pm8941.dtsi:	spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pma8084.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pma8084.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pmd9635.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pmd9635.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pmi8962.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pmi8962.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pmiplutonium.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pmiplutonium.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pmplutonium.dtsi:		spmi-slave-container;
	arch/arm/boot/dts/qcom/msm-pmplutonium.dtsi:		spmi-slave-container;

[..]
> > > +static const struct of_device_id pm8x41_id_table[] = {
> > > +	{ .compatible = "qcom,pm8841", },
> > > +	{ .compatible = "qcom,pm8941", },
> > > +	{},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, pm8x41_id_table);
> > 
> > I'm thinking we should probably have a generic compatible entry as well,
> > "qcom,pmic-qpnp" or similar.  We should still specify in the binding
> > that PMIC slaves specify a version-specific string as well as the
> > generic string.  That is, a slave should have:
> > 
> > 	compatible = "qcom,pm8841", "qcom,pmic-qpnp";
> > 
> > ...in case we would ever need to differentiate in the future.
> > 
> > (I recall that in a previous version I had done this, but I don't
> > remember why I had changed it..)
>
> I gave this some thought but came to the conclusion that there is no
> benefit of adding a generic compatible to a new binding.  Please clarify
> a use-case where this would be ... useful.

Having a generic compatible entry allows for easily supporting new PMICs
without having to add yet another vacuous entry in the ID table.  In
this case I think it's perfectly acceptable given that this driver isn't
really defining a programming model for a specific device, but rather
acting much more like a bus.

Requiring a specific PMIC listed before a generic one allows us an
escape hatch in the future if for some reason we need to add a quirk for
a specific PMIC.

  Josh

[1]: git://codeaurora.org/quic/la/kernel/msm-3.10#msm-3.10

-- 
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists