[PATCH 1/4] documentation: add palmas dts definition

[PATCH 1/4] documentation: add palmas dts definition

[J Keerthy]

From: Graeme Gregory <gg@slimlogic.co.uk>

Add the DTS definition for the palmas device including the MFD children.

Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
[j-keerthy@ti.com: changed the DT node property names to follow the
convention]
Signed-off-by: J Keerthy <j-keerthy@ti.com>
---
 Documentation/devicetree/bindings/mfd/palmas.txt |   67 ++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/palmas.txt

diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt
new file mode 100644
index 0000000..5fa922e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/palmas.txt
@@ -0,0 +1,67 @@
+Texas Instruments Palmas family
+
+The Palmas familly are Integrated Power Management Chips.
+These chips are connected to an i2c bus.
+
+
+Required properties:
+- compatible : Must be "ti,palmas";
+  For Integrated power-management in the palmas series, twl6035, twl6037,
+  tps65913
+- 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>;
+- 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.
+- mux-pad1 if a value is given it will be used for the pad1 mux
+- mux-pad2 if a value us given it will be used for the pad2 mux
+- power-ctrl if a value is given it will be written to the POWER_CTRL register
+
+Example:
+/*
+ * Integrated Power Management Chip Palmas
+ */
+palmas@48 {
+    compatible = "ti,palmas";
+    reg = <0x48>;
+    interrupts = <39>; /* IRQ_SYS_1N cascaded to 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 {
+		compatible = "ti,palmas_pmic";
+		regulators {
+			smps12_reg: smps12 {
+				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>;
+        reg = <0>;
+    };
+};
-- 
1.7.5.4

Re: [PATCH 1/4] documentation: add palmas dts definition

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On Wed, Feb 20, 2013 at 09:30:15AM +0530, J Keerthy wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
> 
> Add the DTS definition for the palmas device including the MFD children.
> 
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> [j-keerthy@ti.com: changed the DT node property names to follow the
> convention]
> Signed-off-by: J Keerthy <j-keerthy@ti.com>
> ---
>  Documentation/devicetree/bindings/mfd/palmas.txt |   67 ++++++++++++++++++++++
>  1 files changed, 67 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/palmas.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt
> new file mode 100644
> index 0000000..5fa922e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/palmas.txt
> @@ -0,0 +1,67 @@
> +Texas Instruments Palmas family
> +
> +The Palmas familly are Integrated Power Management Chips.
> +These chips are connected to an i2c bus.

I would expect this to enumerate the regulators that the user can select
but it doesn't appear to do that.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/4] documentation: add palmas dts definition

[J, KEERTHY]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Wednesday, February 20, 2013 4:57 PM
> To: J, KEERTHY
> Cc: devicetree-discuss@lists.ozlabs.org; linux-doc@vger.kernel.org;
> linux-kernel@vger.kernel.org; grant.likely@secretlab.ca;
> rob.herring@calxeda.com; rob@landley.net; gg@slimlogic.co.uk;
> sameo@linux.intel.com; liam.r.girdwood@intel.com
> Subject: Re: [PATCH 1/4] documentation: add palmas dts definition
> 
> On Wed, Feb 20, 2013 at 09:30:15AM +0530, J Keerthy wrote:
> > From: Graeme Gregory <gg@slimlogic.co.uk>
> >
> > Add the DTS definition for the palmas device including the MFD
> children.
> >
> > Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> > [j-keerthy@ti.com: changed the DT node property names to follow the
> > convention]
> > Signed-off-by: J Keerthy <j-keerthy@ti.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/palmas.txt |   67
> ++++++++++++++++++++++
> >  1 files changed, 67 insertions(+), 0 deletions(-)  create mode
> 100644
> > Documentation/devicetree/bindings/mfd/palmas.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt
> > b/Documentation/devicetree/bindings/mfd/palmas.txt
> > new file mode 100644
> > index 0000000..5fa922e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/palmas.txt
> > @@ -0,0 +1,67 @@
> > +Texas Instruments Palmas family
> > +
> > +The Palmas familly are Integrated Power Management Chips.
> > +These chips are connected to an i2c bus.
> 
> I would expect this to enumerate the regulators that the user can
> select but it doesn't appear to do that.


Mark,

Thanks for reviewing the patch.

I went through the DT Documentation files in the regulator/ folder.
So shall I add a list of regulators which are in the Palmas device
Under a separate file under Documentation/devicetree/bindings/regulator ?
Is that what you are looking for?

Regards,
Keerthy

Re: [PATCH 1/4] documentation: add palmas dts definition

[Stephen Warren]

On 02/20/2013 06:49 AM, J, KEERTHY wrote:
> Mark Brown wrote at Wednesday, February 20, 2013 4:57 PM:
>> On Wed, Feb 20, 2013 at 09:30:15AM +0530, J Keerthy wrote:
>>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>>
>>> 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.
>>
>> I would expect this to enumerate the regulators that the user can
>> select but it doesn't appear to do that.
> 
> Mark,
> 
> Thanks for reviewing the patch.
> 
> I went through the DT Documentation files in the regulator/ folder.
> So shall I add a list of regulators which are in the Palmas device
> Under a separate file under Documentation/devicetree/bindings/regulator ?
> Is that what you are looking for?

I believe what Mark wants is something like the following in this file
(take from Documentation/devicetree/bindings/regulator/tps6586x.txt):

> - regulators: A node that houses a sub-node for each regulator within the
>   device. Each sub-node is identified using the node's name (or the deprecated
>   regulator-compatible property if present), with valid values listed below.
>   The content of each sub-node is defined by the standard binding for
>   regulators; see regulator.txt.
>   sys, sm[0-2], ldo[0-9] and ldo_rtc

Re: [PATCH 1/4] documentation: add palmas dts definition

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

On Wed, Feb 27, 2013 at 11:16:57AM -0700, Stephen Warren wrote:

> I believe what Mark wants is something like the following in this file
> (take from Documentation/devicetree/bindings/regulator/tps6586x.txt):

> > - regulators: A node that houses a sub-node for each regulator within the
> >   device. Each sub-node is identified using the node's name (or the deprecated
> >   regulator-compatible property if present), with valid values listed below.
> >   The content of each sub-node is defined by the standard binding for
> >   regulators; see regulator.txt.
> >   sys, sm[0-2], ldo[0-9] and ldo_rtc

Yes, exactly.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH 1/4] documentation: add palmas dts definition

[J, KEERTHY]

> -----Original Message-----
> From: J, KEERTHY
> Sent: Wednesday, February 20, 2013 7:19 PM
> To: 'Mark Brown'
> Cc: devicetree-discuss@lists.ozlabs.org; linux-doc@vger.kernel.org;
> linux-kernel@vger.kernel.org; grant.likely@secretlab.ca;
> rob.herring@calxeda.com; rob@landley.net; gg@slimlogic.co.uk;
> sameo@linux.intel.com; liam.r.girdwood@intel.com
> Subject: RE: [PATCH 1/4] documentation: add palmas dts definition
> 
> 
> 
> > -----Original Message-----
> > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> > Sent: Wednesday, February 20, 2013 4:57 PM
> > To: J, KEERTHY
> > Cc: devicetree-discuss@lists.ozlabs.org; linux-doc@vger.kernel.org;
> > linux-kernel@vger.kernel.org; grant.likely@secretlab.ca;
> > rob.herring@calxeda.com; rob@landley.net; gg@slimlogic.co.uk;
> > sameo@linux.intel.com; liam.r.girdwood@intel.com
> > Subject: Re: [PATCH 1/4] documentation: add palmas dts definition
> >
> > On Wed, Feb 20, 2013 at 09:30:15AM +0530, J Keerthy wrote:
> > > From: Graeme Gregory <gg@slimlogic.co.uk>
> > >
> > > Add the DTS definition for the palmas device including the MFD
> > children.
> > >
> > > Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> > > [j-keerthy@ti.com: changed the DT node property names to follow the
> > > convention]
> > > Signed-off-by: J Keerthy <j-keerthy@ti.com>
> > > ---
> > >  Documentation/devicetree/bindings/mfd/palmas.txt |   67
> > ++++++++++++++++++++++
> > >  1 files changed, 67 insertions(+), 0 deletions(-)  create mode
> > 100644
> > > Documentation/devicetree/bindings/mfd/palmas.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt
> > > b/Documentation/devicetree/bindings/mfd/palmas.txt
> > > new file mode 100644
> > > index 0000000..5fa922e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/palmas.txt
> > > @@ -0,0 +1,67 @@
> > > +Texas Instruments Palmas family
> > > +
> > > +The Palmas familly are Integrated Power Management Chips.
> > > +These chips are connected to an i2c bus.
> >
> > I would expect this to enumerate the regulators that the user can
> > select but it doesn't appear to do that.
> 
> 
> Mark,
> 
> Thanks for reviewing the patch.
> 
> I went through the DT Documentation files in the regulator/ folder.
> So shall I add a list of regulators which are in the Palmas device
> Under a separate file under Documentation/devicetree/bindings/regulator
> ?
> Is that what you are looking for?

Any inputs on this Mark? Sorry I did not understand the comment.

> 
> Regards,
> Keerthy

Regards,
Keerthy

[PATCH 1/4] documentation: add palmas dts definition

[J Keerthy]

From: Graeme Gregory <gg@slimlogic.co.uk>

Add the DTS definition for the palmas device including the MFD children.

Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
[j-keerthy@ti.com: changed the DT node property names to follow the
convention]
Signed-off-by: J Keerthy <j-keerthy@ti.com>
---
 Documentation/devicetree/bindings/mfd/palmas.txt |   67 ++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/palmas.txt

diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt
new file mode 100644
index 0000000..5fa922e
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/palmas.txt
@@ -0,0 +1,67 @@
+Texas Instruments Palmas family
+
+The Palmas familly are Integrated Power Management Chips.
+These chips are connected to an i2c bus.
+
+
+Required properties:
+- compatible : Must be "ti,palmas";
+  For Integrated power-management in the palmas series, twl6035, twl6037,
+  tps65913
+- 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>;
+- 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.
+- mux-pad1 if a value is given it will be used for the pad1 mux
+- mux-pad2 if a value us given it will be used for the pad2 mux
+- power-ctrl if a value is given it will be written to the POWER_CTRL register
+
+Example:
+/*
+ * Integrated Power Management Chip Palmas
+ */
+palmas@48 {
+    compatible = "ti,palmas";
+    reg = <0x48>;
+    interrupts = <39>; /* IRQ_SYS_1N cascaded to 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 {
+		compatible = "ti,palmas_pmic";
+		regulators {
+			smps12_reg: smps12 {
+				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>;
+        reg = <0>;
+    };
+};
-- 
1.7.5.4

Re: [PATCH 1/4] documentation: add palmas dts definition

[Stephen Warren]

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>;
> +    };
> +};

Re: [PATCH 1/4] documentation: add palmas dts definition

[Laxman Dewangan]

On Thursday 28 February 2013 12:02 AM, Stephen Warren wrote:
> On 02/17/2013 10:11 PM, J Keerthy wrote:
> +- 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?


I dont think that child IP block can be used outside of the palma 
although other mfd device may have same IP.

The child driver very much used the palma's API for register access and 
they can not be separated untill driver is write completely independent 
of palmas API. Currently, child driver include the palma header, uses 
palma mfd stcruture and plama's api for accessing registers.

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

Stephen,
Just curios, why do we require the palma_pmic node at all, We can start 
with regulator node directly. Is it not too much nested here?



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

The interrupt goes to the chip-internal irq, not to external of chip.  
We have only one int line from chip which can be connected to processor/GIC.
yes, interrupt parent need to be define here to get the proper interrupt 
number.

Re: [PATCH 1/4] documentation: add palmas dts definition

[Graeme Gregory]

On 28/02/13 08:52, Laxman Dewangan wrote:
> On Thursday 28 February 2013 12:02 AM, Stephen Warren wrote:
>> On 02/17/2013 10:11 PM, J Keerthy wrote:
>> +- 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?
>
>
> I dont think that child IP block can be used outside of the palma
> although other mfd device may have same IP.
>
> The child driver very much used the palma's API for register access
> and they can not be separated untill driver is write completely
> independent of palmas API. Currently, child driver include the palma
> header, uses palma mfd stcruture and plama's api for accessing registers.
>
I wonder why break good software principles of keeping data and code
localised? Just because there is no current case where a block is
re-used does not mean it shall not be so in the future. The original
information I got from TI when designing this was blocks may be re-used
in future products.

This structure also makes it much neater when dealing with palmas
varients with different IP blocks which already exist.

I also do not see an issue with working like the internal MFD structure,
I think it is a good design.

>>> +    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.
>
> Stephen,
> Just curios, why do we require the palma_pmic node at all, We can
> start with regulator node directly. Is it not too much nested here?
>
>
>
>>
>> +
>> +    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?
>
> The interrupt goes to the chip-internal irq, not to external of chip. 
> We have only one int line from chip which can be connected to
> processor/GIC.
> yes, interrupt parent need to be define here to get the proper
> interrupt number.
Those interrupt lines are un-needed in the newer version of driver, I
forgot to remove them. The regmap-irq takes care of this for us without
needing this information in the DT at all.

But actually the OF handles this without requiring a parent in this
case. These interrupts are fed to the child nodes inside io_resource
entries.

Graeme

Re: [PATCH 1/4] documentation: add palmas dts definition

[Laxman Dewangan]

On Thursday 28 February 2013 03:28 PM, Graeme Gregory wrote:
> On 28/02/13 08:52, Laxman Dewangan wrote:
>> On Thursday 28 February 2013 12:02 AM, Stephen Warren wrote:
>>> On 02/17/2013 10:11 PM, J Keerthy wrote:
>>> +- 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?
>>
>> I dont think that child IP block can be used outside of the palma
>> although other mfd device may have same IP.
>>
>> The child driver very much used the palma's API for register access
>> and they can not be separated untill driver is write completely
>> independent of palmas API. Currently, child driver include the palma
>> header, uses palma mfd stcruture and plama's api for accessing registers.
>>
> I wonder why break good software principles of keeping data and code
> localised? Just because there is no current case where a block is
> re-used does not mean it shall not be so in the future. The original
> information I got from TI when designing this was blocks may be re-used
> in future products.
>
> This structure also makes it much neater when dealing with palmas
> varients with different IP blocks which already exist.
>
> I also do not see an issue with working like the internal MFD structure,
> I think it is a good design.


I did not get how the register access will be happen from IP driver.
suppose we have RTC driver which is common IP for device 1 and device2. 
Device1 and device2 are registered as separate MFD driver which has 
different set of chip structure and initialisation.

When I write the RTC register then how do  I call register access? 
Currently RTC driver is saying device1_reg_read() or device2_reg_read() 
etc on which register address passed along with dev or chip structure.

Re: [PATCH 1/4] documentation: add palmas dts definition

[Graeme Gregory]

On 28/02/13 10:27, Laxman Dewangan wrote:
> On Thursday 28 February 2013 03:28 PM, Graeme Gregory wrote:
>> On 28/02/13 08:52, Laxman Dewangan wrote:
>>> On Thursday 28 February 2013 12:02 AM, Stephen Warren wrote:
>>>> On 02/17/2013 10:11 PM, J Keerthy wrote:
>>>> +- 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?
>>>
>>> I dont think that child IP block can be used outside of the palma
>>> although other mfd device may have same IP.
>>>
>>> The child driver very much used the palma's API for register access
>>> and they can not be separated untill driver is write completely
>>> independent of palmas API. Currently, child driver include the palma
>>> header, uses palma mfd stcruture and plama's api for accessing
>>> registers.
>>>
>> I wonder why break good software principles of keeping data and code
>> localised? Just because there is no current case where a block is
>> re-used does not mean it shall not be so in the future. The original
>> information I got from TI when designing this was blocks may be re-used
>> in future products.
>>
>> This structure also makes it much neater when dealing with palmas
>> varients with different IP blocks which already exist.
>>
>> I also do not see an issue with working like the internal MFD structure,
>> I think it is a good design.
>
>
> I did not get how the register access will be happen from IP driver.
> suppose we have RTC driver which is common IP for device 1 and
> device2. Device1 and device2 are registered as separate MFD driver
> which has different set of chip structure and initialisation.
>
> When I write the RTC register then how do  I call register access?
> Currently RTC driver is saying device1_reg_read() or
> device2_reg_read() etc on which register address passed along with dev
> or chip structure.
>
Since I originally wrote the driver it is now possible to get your
parents regmap without knowledge of the parent.

All that is then needed is a method to pass an offset (possibly re-use
IO_RESOURCE).

The final but of information that would be needed is some method to pass
down product_id/design_rev and for a lot of the IP blocks they could be
made independent of the actual parent.

This is theoretical at the moment because I would not do this work
unless it became neccessary. But this was in my head when I was
originally designing the driver.

The RTC is a good point, the same RTC IP block is used in most tps6591X
and tps800XX devices. My dream would be to make them all one driver!

Graeme

Re: [PATCH 1/4] documentation: add palmas dts definition

[Graeme Gregory]

On 28/02/13 10:57, Graeme Gregory wrote:
> On 28/02/13 10:27, Laxman Dewangan wrote:
>> On Thursday 28 February 2013 03:28 PM, Graeme Gregory wrote:
>>> On 28/02/13 08:52, Laxman Dewangan wrote:
>>>> On Thursday 28 February 2013 12:02 AM, Stephen Warren wrote:
>>>>> On 02/17/2013 10:11 PM, J Keerthy wrote:
>>>>> +- 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?
>>>> I dont think that child IP block can be used outside of the palma
>>>> although other mfd device may have same IP.
>>>>
>>>> The child driver very much used the palma's API for register access
>>>> and they can not be separated untill driver is write completely
>>>> independent of palmas API. Currently, child driver include the palma
>>>> header, uses palma mfd stcruture and plama's api for accessing
>>>> registers.
>>>>
>>> I wonder why break good software principles of keeping data and code
>>> localised? Just because there is no current case where a block is
>>> re-used does not mean it shall not be so in the future. The original
>>> information I got from TI when designing this was blocks may be re-used
>>> in future products.
>>>
>>> This structure also makes it much neater when dealing with palmas
>>> varients with different IP blocks which already exist.
>>>
>>> I also do not see an issue with working like the internal MFD structure,
>>> I think it is a good design.
>>
>> I did not get how the register access will be happen from IP driver.
>> suppose we have RTC driver which is common IP for device 1 and
>> device2. Device1 and device2 are registered as separate MFD driver
>> which has different set of chip structure and initialisation.
>>
>> When I write the RTC register then how do  I call register access?
>> Currently RTC driver is saying device1_reg_read() or
>> device2_reg_read() etc on which register address passed along with dev
>> or chip structure.
>>
> Since I originally wrote the driver it is now possible to get your
> parents regmap without knowledge of the parent.
>
> All that is then needed is a method to pass an offset (possibly re-use
> IO_RESOURCE).
>
> The final but of information that would be needed is some method to pass
> down product_id/design_rev and for a lot of the IP blocks they could be
> made independent of the actual parent.
>
> This is theoretical at the moment because I would not do this work
> unless it became neccessary. But this was in my head when I was
> originally designing the driver.
>
> The RTC is a good point, the same RTC IP block is used in most tps6591X
> and tps800XX devices. My dream would be to make them all one driver!
>
And on the OS agnostic side of things, if you use hierarchies in DT then
the OS does not have to choose to use them.

But if you make everything flat the OS is pretty much forced to follow.

So in my mind trying to make everything flat leads to less choice for
the implementer which is bad IMHO!

Graeme
 

Re: [PATCH 1/4] documentation: add palmas dts definition

[Stephen Warren]

On 02/28/2013 03:57 AM, Graeme Gregory wrote:
...
> The final but of information that would be needed is some method to pass
> down product_id/design_rev and for a lot of the IP blocks they could be
> made independent of the actual parent.

That should probably be represented in DT itself as differing compatible
values.

I could see the argument that this is SW-probe-able since there's a
register that defines the value. However, any global ID register would
apply to the top-level device, and not the version of any child IP
blocks if there's the possibility of mixing/matching IP blocks. If
there's a dedicated version register for a child IP block, then
presumably it's already part of the child IP block's register space, and
so the driver can read it, and then there perhaps wouldn't be a need to
represent this using different compatible values.

Re: [PATCH 1/4] documentation: add palmas dts definition

[Stephen Warren]

On 02/28/2013 02:58 AM, Graeme Gregory wrote:
> On 28/02/13 08:52, Laxman Dewangan wrote:
>> On Thursday 28 February 2013 12:02 AM, Stephen Warren wrote:
>>> On 02/17/2013 10:11 PM, J Keerthy wrote:
>>> > +- 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?
...
> I wonder why break good software principles of keeping data and code
> localised? Just because there is no current case where a block is
> re-used does not mean it shall not be so in the future. The original
> information I got from TI when designing this was blocks may be re-used
> in future products.
> 
> This structure also makes it much neater when dealing with palmas
> varients with different IP blocks which already exist.

Given the current existence of chips that mix/match IP blocks and the
guidance you quote from TI about future chips doing so too, having these
child nodes in the DT makes sense to me.

>>> +    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?
>>
>> The interrupt goes to the chip-internal irq, not to external of chip. 
>> We have only one int line from chip which can be connected to
>> processor/GIC.
>> yes, interrupt parent need to be define here to get the proper
>> interrupt number.
>
> Those interrupt lines are un-needed in the newer version of driver, I
> forgot to remove them. The regmap-irq takes care of this for us without
> needing this information in the DT at all.
> 
> But actually the OF handles this without requiring a parent in this
> case. These interrupts are fed to the child nodes inside io_resource
> entries.

That doesn't make sense.

The driver for the child node should parse the DT to extract the IRQ
number/handle, and IRQ must be fully represented in DT in the standard
fashion. Put another way, the driver for the child node should not
expect the top-level driver to have created platform-device style
resources for it when instantiated from DT.

If you don't do this, there will be no possibility for the driver for
the top-level Palmas node to completely generically support arbitrary IP
blocks being mixed/matched into it, and hence there will be no point at
all representing the child IP blocks as separate DT nodes.

Even ignoring any of that, the DT content still needs to correctly
represent the HW, using the existing DT standards for interrupts, and it
doesn't as written.

Re: [PATCH 1/4] documentation: add palmas dts definition

[Stephen Warren]

On 02/28/2013 01:52 AM, Laxman Dewangan wrote:
> On Thursday 28 February 2013 12:02 AM, Stephen Warren wrote:
>> On 02/17/2013 10:11 PM, J Keerthy wrote:
>> +- 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?
> 
> 
> I dont think that child IP block can be used outside of the palma
> although other mfd device may have same IP.

That sounds like pretty much the definition of re-using the IP block...

> The child driver very much used the palma's API for register access and
> they can not be separated untill driver is write completely independent
> of palmas API. Currently, child driver include the palma header, uses
> palma mfd stcruture and plama's api for accessing registers.

The DT binding and compatible values should not be influenced by
OS-specific driver implementation details. DT bindings are supposed to
be (as near as possible) a pure HW description, which (many different)
OSs parse, and map to their internal driver structure as appropriate.

The above is of course just a comment on how DT is supposed to work; I'm
not saying anything here re: what's the most appropriate DT structure
for this device.

>>> +    palmas_pmic {
>> Just "pmic" seems simpler, although I dare say the node name isn't
>> really used for anything.
> 
> Stephen,
> Just curios, why do we require the palma_pmic node at all, We can start
> with regulator node directly. Is it not too much nested here?

That was the question I was asking in my original email. But I also
commented on the patch as written, in case the answer to my question was
that the child DT nodes made sense.

RE: [PATCH 1/4] documentation: add palmas dts definition

[J, KEERTHY]

Hello Stephen,

Thanks for a detailed review.

> -----Original Message-----
> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> Sent: Thursday, February 28, 2013 12:03 AM
> To: J, KEERTHY
> Cc: grant.likely@secretlab.ca; rob.herring@calxeda.com;
> rob@landley.net; devicetree-discuss@lists.ozlabs.org; linux-
> doc@vger.kernel.org; linux-kernel@vger.kernel.org; Cousson, Benoit;
> gg@slimlogic.co.uk
> Subject: Re: [PATCH 1/4] documentation: add palmas dts definition
> 
> 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.

I will correct this.

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

AFAIK there is no HW version.
 
> 
> > +  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.

Ok.

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

>From the register manual I see that only GPIO has the edge detect capability.
I agree.

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

I guess for now I will drop this patch and will be taken up once we
Finalize on the design.
 
> ...
> > +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>;
> > +    };
> > +};

Re: [PATCH 1/4] documentation: add palmas dts definition

[Stephen Warren]

On 02/28/2013 05:09 AM, J, KEERTHY wrote:
> Stephen Warren wrote at Thursday, February 28, 2013 12:03 AM:
>> 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

>>> +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?
> 
> AFAIK there is no HW version.

My point was more: might there be in the future. However, I guess we can
go with a first compatible value that has no version, and for any future
device we can add the version to its compatible value then.

>>> +- 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.
> 
> From the register manual I see that only GPIO has the edge detect capability.
> I agree.

I'm not sure if you're agreeing that #interrupt-cells should be 2 here
as I suggested, or with the original code. you say "only GPIO has the
edge detect capability" which would imply that IRQs don't, which would
imply no need for a flags cell in DT, so #interrupt-cells=<1> would be
fine... But then you say "I agree" after I suggested that
#interrupt-cells=<2> might be better.

BTW, your mailer completely mangled the line-wrapping of my email,
making the parts you quoted rather harder to read.

>>> +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?
> 
> I guess for now I will drop this patch and will be taken up once we
> Finalize on the design.

The DT binding has to be fully defined before the code, or how do you
know what binding you're writing the code for? Dropping this patch and
then moving forward with posting it (which is what your statement
implies) doesn't seem correct.

RE: [PATCH 1/4] documentation: add palmas dts definition

[J, KEERTHY]

Hi Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> Sent: Friday, March 01, 2013 12:37 AM
> To: J, KEERTHY
> Cc: grant.likely@secretlab.ca; rob.herring@calxeda.com;
> rob@landley.net; devicetree-discuss@lists.ozlabs.org; linux-
> doc@vger.kernel.org; linux-kernel@vger.kernel.org; Cousson, Benoit;
> gg@slimlogic.co.uk
> Subject: Re: [PATCH 1/4] documentation: add palmas dts definition
> 
> On 02/28/2013 05:09 AM, J, KEERTHY wrote:
> > Stephen Warren wrote at Thursday, February 28, 2013 12:03 AM:
> >> 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
> 
> >>> +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?
> >
> > AFAIK there is no HW version.
> 
> My point was more: might there be in the future. However, I guess we
> can go with a first compatible value that has no version, and for any
> future device we can add the version to its compatible value then.
> 

I got it. We can add versioning when we have the next versions available.

> >>> +- 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.
> >
> > From the register manual I see that only GPIO has the edge detect
> capability.
> > I agree.
> 
> I'm not sure if you're agreeing that #interrupt-cells should be 2 here
> as I suggested, or with the original code. you say "only GPIO has the
> edge detect capability" which would imply that IRQs don't, which would
> imply no need for a flags cell in DT, so #interrupt-cells=<1> would be
> fine... But then you say "I agree" after I suggested that #interrupt-
> cells=<2> might be better.

Sorry I did not give a detailed explanation earlier. There are 32 sources
Of interrupts from Palmas but only one physical line coming out. Out of 32
8 of them are from the GPIO module of palmas. The GPIO module interrupts
Are edge based. Hence I completely agreed to the point of using
#interrupt-cells=<2>.
 
> 
> BTW, your mailer completely mangled the line-wrapping of my email,
> making the parts you quoted rather harder to read.
> 

Sorry about that but not quite sure what happened there!

> >>> +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?
> >
> > I guess for now I will drop this patch and will be taken up once we
> > Finalize on the design.
> 
> The DT binding has to be fully defined before the code, or how do you
> know what binding you're writing the code for? Dropping this patch and
> then moving forward with posting it (which is what your statement
> implies) doesn't seem correct.

Since Graeme is planning to redesign a bit I am dropping this patch
And he plans to send the updated documentation patch. The original
Intent of this series was to add documentation since the code was
Defined before Documentation. To make it clear I am not posting
Any further patches before Documentation/DT binding is completely
Defined.

Regards,
Keerthy