Message-ID: <6af100d1d4ba82de24f96561b5108072@slimlogic.co.uk>
Date:	Tue, 04 Jun 2013 07:24:36 +0100
From:	gg@...mlogic.co.uk
To:	"J, KEERTHY" <j-keerthy@...com>
Cc:	Stephen Warren <swarren@...dotorg.org>,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-leds@...r.kernel.org,
	linux-watchdog@...r.kernel.org,
	devicetree-discuss@...ts.ozlabs.org, grant.likely@...retlab.ca,
	broonie@...nsource.wolfsonmicro.com, rob.herring@...xeda.com,
	rob@...dley.net, mturquette@...aro.org, linus.walleij@...aro.org,
	cooloney@...il.com, sfr@...b.auug.org.au, rpurdie@...ys.net,
	akpm@...ux-foundation.org, sameo@...ux.intel.com, wim@...ana.be,
	lgirdwood@...il.com, ldewangan@...dia.com,
	"Kristo, Tero" <t-kristo@...com>
Subject: RE: [PATCH v10 01/12] mfd: DT bindings for the palmas family MFD

On 2013-06-03 07:55, J, KEERTHY wrote:
> Hi Stephen,
> 
>> -----Original Message-----
>> From: Stephen Warren [mailto:swarren@...dotorg.org]
>> Sent: Friday, May 31, 2013 4:34 AM
>> To: J, KEERTHY
>> Cc: gg@...mlogic.co.uk; Ian Lartey; linux-kernel@...r.kernel.org;
>> linux-doc@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; 
>> linux-
>> leds@...r.kernel.org; linux-watchdog@...r.kernel.org; devicetree-
>> discuss@...ts.ozlabs.org; grant.likely@...retlab.ca;
>> broonie@...nsource.wolfsonmicro.com; rob.herring@...xeda.com;
>> rob@...dley.net; mturquette@...aro.org; linus.walleij@...aro.org;
>> cooloney@...il.com; sfr@...b.auug.org.au; rpurdie@...ys.net;
>> akpm@...ux-foundation.org; sameo@...ux.intel.com; wim@...ana.be;
>> lgirdwood@...il.com; ldewangan@...dia.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:
>> >
>> > On 03/25/2013 11:29 PM, Stephen Warren wrote:
>> >
>> >> On 03/22/2013 08:55 AM, Ian Lartey wrote:
>> >>> From: Graeme Gregory <gg@...mlogic.co.uk>
>> >>>
>> >>> 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.
>> >>
>> >>> 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.
>> >>
>> >> This same comment likely applies everywhere, so I won't repeat it.
>> >>
>> >>> +Example:
>> >>> +
>> >>> +clk {
>> >>> +    compatible = "ti,twl6035-clk", "ti,palmas-clk";
>> >>> +    ti,clk32kg-mode-sleep = <0>;
>> >>> +    ti,clk32kgaudio-mode-sleep = <0>;
>> >>
>> >>> +    #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
>> >> 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.
>> >>
>> >>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-palmas.txt
>> >>> b/Documentation/devicetree/bindings/gpio/gpio-palmas.txt
>> >>
>> >>> +- 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
>> >> 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.
>> >>
>> >>> +- 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.
>> >>
>> >>> +- 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,
>> >> so I won't repeat it.
>> >>
>> >>> diff --git
>> >>> a/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt
>> >>> b/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt
>> >>
>> >>> +- interrupts: the interrupt outputs of the controller.
>> >>
>> >> How many entries are there, what do they mean, and in what order
>> must
>> >> 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).
>> >>
>> >>> +- 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.
>> >>
>> >>> diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt
>> >>> b/Documentation/devicetree/bindings/mfd/palmas.txt
>> >>
>> >>> +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.
> 
The I2C addresses are set in OTP, I do not think they should ever be 
changed
on the fly.

Graeme

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