Hi Stephen,

-----Original Message-----
From: Stephen Warren [mailto:swarren@wwwdotorg.org]
Sent: Friday, May 31, 2013 4:34 AM
To: J, KEERTHY
Cc: gg@slimlogic.co.uk; Ian Lartey; linux-kernel@vger.kernel.org;
linux-doc@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
leds@vger.kernel.org; linux-watchdog@vger.kernel.org; devicetree-
discuss@lists.ozlabs.org; grant.likely@secretlab.ca;
broonie@opensource.wolfsonmicro.com; rob.herring@calxeda.com;
rob@landley.net; mturquette@linaro.org; linus.walleij@linaro.org;
cooloney@gmail.com; sfr@canb.auug.org.au; rpurdie@rpsys.net;
akpm@linux-foundation.org; sameo@linux.intel.com; wim@iguana.be;
lgirdwood@gmail.com; ldewangan@nvidia.com; Kristo, Tero
Subject: Re: [PATCH v10 01/12] mfd: DT bindings for the palmas family
MFD

On 05/30/2013 05:33 AM, keerthy wrote:

quoted

On 03/25/2013 11:29 PM, Stephen Warren wrote:

quoted

On 03/22/2013 08:55 AM, Ian Lartey wrote:

quoted

From: Graeme Gregory <redacted>

Add the various binding files for the palmas family of chips. There
is a top level MFD binding then a seperate binding for IP blocks on

chips.

quoted

quoted

quoted

diff --git a/Documentation/devicetree/bindings/clock/palmas-clk.txt

b/Documentation/devicetree/bindings/clock/palmas-clk.txt

Where is the reg property; if you're instantiating the clk device as
a standalone DT node and driver, it should be possible to retrieve
the address of this IP block instance from DT, not using driver-

internal APIs.

quoted

quoted

This same comment likely applies everywhere, so I won't repeat it.

quoted

+Example:
+
+clk {
+    compatible = "ti,twl6035-clk", "ti,palmas-clk";
+    ti,clk32kg-mode-sleep = <0>;
+    ti,clk32kgaudio-mode-sleep = <0>;

quoted

+    #clock-cells = <1>;
+    clock-frequency = <32000000>;
+    clock-names = "clk32kg", "clk32kgaudio";

The binding description itself should describe what clocks this node
provides and consumes.

What is clock-frequency; which clock does it affect?

The presence of #clock-cells implies this is a clock provider. This
binding should define the format of the clock cells that this
property implies. I assume it's just the ID of the output clocks,

but

quoted

quoted

what values correspond to which output clocks? That mapping table
needs to be listed here.

The presence of clock-names implies this is a clock consumer.
However, there is no clocks property in the example. Is clks missing
from the example, or should this property be clock-output-names
instead? If this node is a clock consumer, the list of which clocks
it requires should be documented.

quoted

diff --git a/Documentation/devicetree/bindings/gpio/gpio-palmas.txt

b/Documentation/devicetree/bindings/gpio/gpio-palmas.txt

quoted

+- gpio-controller: mark the device as a GPIO controller
+- gpio-cells = <1>:  GPIO lines are provided.

That's #gpio-cells not gpio-cells.

I assume that cell simply contains the GPIO ID/number. That should

be

quoted

quoted

documented for clarity.

Surely gpio-cells should be 2 not 1, so there is space for flags. At
least an "inverted" or "active-low" flag should be included; GPIO
consumers would typically implement that flag in SW, so there' no
requirement that the flag only be supported if the HW supports the

feature.

quoted

quoted

quoted

+- interrupt-controller : palmas has its own internal IRQs
+- #interrupt-cells : should be set to 2 for IRQ number and flags
+  The first cell is the IRQ number.
+  The second cell is the flags, encoded as the trigger masks from
+  Documentation/devicetree/bindings/interrupts.txt

You need "/interrupt-controller" before the filename there.

quoted

+- interrupt-parent : The parent interrupt controller.

If this IP block has interrupt outputs, it should require an
"interrupts" property too. This is the same concept that drives the
need for a reg property. This same comment likely applies

everywhere,

quoted

quoted

so I won't repeat it.

quoted

diff --git
a/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt
b/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt

quoted

+- interrupts: the interrupt outputs of the controller.

How many entries are there, what do they mean, and in what order

must

quoted

quoted

they appear? (Note that the binding of a node must define the order
of the interrupts property, since historically it's been accessed by
index, not by name, and all bindings must allow that access method

to be used).

quoted

quoted

quoted

+- interrupt-names : Should be the name of irq resource. Each
+interrupt
+  binds its interrupt-name.

The binding needs to define standard names for the entries in this
property, or define that interrupts are only retrieved by index, in
which case interrupt-names shouldn't be present. This same comment
likely applies everywhere, so I won't repeat it.

quoted

diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt

b/Documentation/devicetree/bindings/mfd/palmas.txt

quoted

+Required properties:

...

I believe the Palmas devices have many separate I2C addresses, even
buses, which are I think are at least partially independently SW
configurable. In that case, the reg property for this node should
probably enumerate all the I2C addresses that this chip responds to,
so that they can each be passed down to the child nodes.

Stephen,

The palmas devices do have multiple I2C slave IDs. From OMAP5 as the
master all the palmas slave devices are connected via I2C1 bus.

I did not understand the SW configurable part. It is more board
dependent. Correct me if i understood it wrongly.

IIRC (and I may not sine it's been a while since I looked at this),
there are SW registers that can modify the I2C address that the chip
will respond to, so you could access the main I2C address, then program
which other I2C addresses get used.

Ok. I guess you are referring to the I2C_SPI register of Palmas.
This register is indeed SW configurable and I tried changing the
Slave IDs on the fly and I could change them. AFAIK these are OTP
And never changed through software on the fly.

Or, perhaps I was just thinking of the fact that there are strapping
pins on the chip that affect both the main I2C address, and some/all of
the other I2C addresses, so the driver needs to be told each and every
I2C address, not just one single overall I2C address.

Looking at the register spec there seem to be 2 possible combinations:
0x48, 0x49, 0x4A or 0x58, 0x59, 0x5A. Again these are OTP. By default
It is 0x48, 0x49, 0x4A. So passing 0x48 or 0x58 as the main I2C
Address seems sufficient here.

Regards,
Keerthy