RE: [PATCH v2] mfd: DT bindings for the palmas family MFD

[J, KEERTHY]

Hi Stephen,

Thanks for the quick review.

> -----Original Message-----
> From: Stephen Warren [mailto:swarren@xxxxxxxxxxxxx]
> Sent: Wednesday, June 05, 2013 10:44 PM
> To: J, KEERTHY
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx;
> devicetree-discuss@xxxxxxxxxxxxxxxx;
> broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx; rob.herring@xxxxxxxxxxx;
> rob@xxxxxxxxxxx; sameo@xxxxxxxxxxxxxxx; wim@xxxxxxxxx;
> lgirdwood@xxxxxxxxx; gg@xxxxxxxxxxxxxxx; Kristo, Tero;
> lee.jones@xxxxxxxxxx; Ian Lartey
> Subject: Re: [PATCH v2] mfd: DT bindings for the palmas family MFD
> 
> On 06/04/2013 02:41 AM, J Keerthy wrote:
> > From: Graeme Gregory <gg@xxxxxxxxxxxxxxx>
> >
> > Add the various binding files for the palmas family of chips. There
> is
> > a top level MFD binding then a seperate binding for regulators IP
> blocks on chips.
> 
> > diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt
> > b/Documentation/devicetree/bindings/mfd/palmas.txt
> 
> > +Optional properties:
> > +  ti,mux_padX : set the pad register X (1-2) to the correct muxing
> for the
> > +  		hardware, if not set will use muxing in OTP.
> > +
> > +Example:
> ...
> > +	ti,mux-pad1 = <0>;
> > +	ti,mux-pad2 = <0>;
> 
> Use of - vs. _ is inconsistent there. It should be -.
> 

Oops. I will fix this.

> > diff --git
> > a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> > b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt
> 
> > +Optional nodes:
> > +- regulators : should contain the constrains and init information
> for the
> > +	       regulators. It should contain a subnode per regulator from
> the
> > +	       list.
> 
> I would re-phrase that as:
> 
> Must contain a sub-node per regulator from the list below. Each sub-
> node should contain the constraints and initialization information for
> that regulator. See regulator.txt for a description of standard
> properties for these sub-nodes. Additional custom properties  are
> listed below.
> 
> > +	       For ti,palmas-pmic - smps12, smps123, smps3 depending on
> OTP,
> > +	       smps45, smps457, smps7 depending on varient, smps6,
> > +smps[8-10],
> 
> typo: s/varient/variant/.

I will fix this.

> 
> > +	       ldo[1-9], ldoln, ldousb
> 
> nit: s/$/./ ?
> 

Ok.

> > +
> > +	       optional chip specific regulator fields :-
> 
> Perhaps "Optional sub-node properties:"?

Ok.

> 
> > +pmic {
> > +	compatible = "ti,twl6035-pmic", "ti,palmas-pmic";
> > +	interrupt-parent = <&palmas>;
> > +	interrupts = <14 IRQ_TYPE_NONE>;
> > +	interrupt-name = "short-irq";
> 
> If those are required, shouldn't they be listed in a "Required
> properties" section above? In particular, the order of entries in the
> interrupts property must be defined, as well as the expected nameds in
> the interrupt-name property.
> 
> Oh, and it's interrupt-names not interrupt-name.

Ok.

> 
> Oh, one question though: How does the regulator driver determine the
> register address of the regulator sub-device within the overall PMIC?
> Presumably if these are pluggable independent modules, that could
> change depending on which overall chip the PMIC device is plugged into.
> don't you need a reg property to specify that?

The variants have identical register addresses. These are not pluggable
Independent modules. All the variants come with all the regulators
Listed above in general. The driver today has a statically defined
Array of all the above mentioned regulators with their addresses.
 
drivers/regulator/palmas-regulator.c

Line 38.

> 
> Aside from those comments, this all looks reasonable to me.

Once again thanks for the comprehensive feedback.

Regards,
Keerthy
--
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/