On 02/17/2013 10:11 PM, J Keerthy wrote:
> Add the DTS definition for the palmas device including the MFD children.
...
> diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt
...
> +Texas Instruments Palmas family
> +
> +The Palmas familly are Integrated Power Management Chips.
> +These chips are connected to an i2c bus.
s/familly/family.
> +
> +Required properties:
> +- compatible : Must be "ti,palmas";
Do you need a version number there; will there be Palmas v1 HW, then
later Palmas v2 HW, and so on?
> + For Integrated power-management in the palmas series, twl6035, twl6037,
> + tps65913
If this binding represents multiple different chips, compatible should
contain both the most chip-specific value (e.g. ti,twl6035 I guess given
the above) /and/ the more generic "ti,palmas" value. This will allow any
device-specific quirks to be implemented if needed in the future,
without having to retrofit the device-specific compatible value into
.dts files after the fact.
> +- interrupts : This i2c device has an IRQ line connected to the main SoC
> +- interrupt-controller : Since the palmas support several interrupts internally,
> + it is considered as an interrupt controller cascaded to the SoC one.
> +- #interrupt-cells = <1>;
Why not 2; can't any IRQ flags be represented in DT? 1 seems limiting
here unless the HW truly can't support configuration of IRQ input
polarity of edge-vs-level sensitivity.
> +- interrupt-parent : The parent interrupt controller.
> +
> +Optional node:
> +- Child nodes contain in the palmas. The palmas family is made of several
> + variants that support a different number of features.
> + The child nodes will thus depend of the capability of the variant.
Are there DT bindings for those child nodes anywhere?
Representing each internal component as a separate DT node feels a
little like designing the DT bindings to model the Linux-internal MFD
structure. DT bindings should be driven by the HW design and OS-agnostic.
>From a DT perspective, is there any need at all to create a separate DT
node for each component? This would only be needed or useful if the
child IP blocks (and hence DT bindings for those blocks) could be
re-used in other top-level devices that aren't represented by this
top-level ti,palmas DT binding. Are the HW IP blocks here re-used
anywhere, or will they be?
...
> +Example:
> +/*
> + * Integrated Power Management Chip Palmas
> + */
> +palmas@48 {
There's a considerable mix of TAB and space indentation in this example.
> + compatible = "ti,palmas";
> + reg = <0x48>;
> + interrupts = <39>; /* IRQ_SYS_1N cascaded to gic */
If that's routed to a regular ARM GIC, then I think you need extra cells
there; #interrupt-cells=<3> for the ARM GIC.
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + interrupt-parent = <&gic>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ti,mux-pad1 = <0x00>;
> + ti,mux-pad2 = <0x00>;
> + ti,power-ctrl = <0x03>;
> +
> + palmas_pmic {
Just "pmic" seems simpler, although I dare say the node name isn't
really used for anything.
> + compatible = "ti,palmas_pmic";
Using _ in compatible values isn't common. "ti,palmas-pmic" instead?
> + regulators {
> + smps12_reg: smps12 {
As I mentioned elsewhere, this binding (or a separate binding doc for
"ti,palmas_pmic") should contain a list of valid values for these node
names.
> + regulator-min-microvolt = < 600000>;
> + regulator-max-microvolt = <1500000>;
> + regulator-always-on;
> + regulator-boot-on;
> + ti,warm-sleep = <0>;
> + ti,roof-floor = <0>;
> + ti,mode-sleep = <0>;
> + ti,warm-reset = <0>;
> + ti,tstep = <0>;
> + ti,vsel = <0>;
> + };
> + };
> + ti,ldo6-vibrator = <0>;
> + };
> +
> + palmas_rtc {
> + compatible = "ti,palmas_rtc";
> + interrupts = <8 9>;
Are the interrupt outputs of the RTC fed directly to the GIC interrupt
mentioned in the top-level Palmas node, or do these interrupts feed into
a top-level IRQ controller in the Palmas device, which then feeds into
the external IRQ controller?
If these feed into an on-chip IRQ controller, then you'd need an
interrupt-parent property here to indicate that.
If these feed directly into an external IRQ controller, it's almost
certain that IRQ controller's binding uses #interrupt-cells = <3> it
is's the ARM GIC, and hence you need some extra cells here.
> + reg = <0>;
> + };
> +};
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
