| Message ID | 1363964122-19201-2-git-send-email-ian@slimlogic.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/22/2013 08:55 AM, Ian Lartey wrote: > From: Graeme Gregory <gg@slimlogic.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. > 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. > + For ti,palmas-pmic - smps12, smps123, smps3 depending on OTP, > + smps45, smps457, smps7 depending on varient, smps6, smps[8-10], > + ldo[1-9], ldoln, ldousb > + For ti,palmas-charger-pmic - smps12, smps123, smps3 depending on OTP, > + smps[6-9], boost, ldo[1-14], ldoln, ldousb The list of legal compatible values for this node above doesn't include both ti,palmas-pmic and ti,palmas-charger-pmic. Should it? This node should describe this PMIC block in a completely standalone fashion, without the need to go look at the top-level node to see if it's a "charger" variant or not. > + optional chip specific regulator fields :- > + ti,warm-reset - maintain voltage during warm reset > + ti,roof-floor - control voltage selection by pin I assume those are Boolean not integer. It's worth mentioning the type of each property.
On Mon, Mar 25, 2013 at 11:59:11AM -0600, Stephen Warren wrote: > On 03/22/2013 08:55 AM, Ian Lartey wrote: > > From: Graeme Gregory <gg@slimlogic.co.uk> > > +Optional nodes: > > +- regulators : should contain the constrains and init information for the > > + regulators. It should contain a subnode per regulator from the > > + list. > > + For ti,palmas-pmic - smps12, smps123, smps3 depending on OTP, > > + smps45, smps457, smps7 depending on varient, smps6, smps[8-10], > > + ldo[1-9], ldoln, ldousb > > + For ti,palmas-charger-pmic - smps12, smps123, smps3 depending on OTP, > > + smps[6-9], boost, ldo[1-14], ldoln, ldousb > The list of legal compatible values for this node above doesn't include > both ti,palmas-pmic and ti,palmas-charger-pmic. Should it? This node > should describe this PMIC block in a completely standalone fashion, > without the need to go look at the top-level node to see if it's a > "charger" variant or not. The latter was removed from the code in this series, only palmas-pmic is present now. Just as a general thing there seems to be an awful lot of stuff here about the boilerplate for the generic properties like the interrupt and GPIO controller stuff - we probably need to spin round and look at factoring this out to make life easier. There seems to be a lot of boiler plate in the bindings that is factored out well by the frameworks in the code so people don't even need to think about it.
On 03/25/2013 11:29 PM, Stephen Warren wrote: > On 03/22/2013 08:55 AM, Ian Lartey wrote: >> From: Graeme Gregory <gg@slimlogic.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. Graeme, Can i send a separate Documentation patch for palmas-core and palmas-regulators outside this series since the drivers are already part of mainline kernel. This will ensure at least the bare minimal DT support for palmas family will be available in the mainline kernel? > >> 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. >> + For ti,palmas-pmic - smps12, smps123, smps3 depending on OTP, >> + smps45, smps457, smps7 depending on varient, smps6, smps[8-10], >> + ldo[1-9], ldoln, ldousb >> + For ti,palmas-charger-pmic - smps12, smps123, smps3 depending on OTP, >> + smps[6-9], boost, ldo[1-14], ldoln, ldousb > > The list of legal compatible values for this node above doesn't include > both ti,palmas-pmic and ti,palmas-charger-pmic. Should it? This node > should describe this PMIC block in a completely standalone fashion, > without the need to go look at the top-level node to see if it's a > "charger" variant or not. > >> + optional chip specific regulator fields :- >> + ti,warm-reset - maintain voltage during warm reset >> + ti,roof-floor - control voltage selection by pin > > I assume those are Boolean not integer. It's worth mentioning the type > of each property. Reagrds, Keerthy
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@slimlogic.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. 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.
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: > > > > On 03/25/2013 11:29 PM, Stephen Warren wrote: > > > >> On 03/22/2013 08:55 AM, Ian Lartey wrote: > >>> From: Graeme Gregory <gg@slimlogic.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. Regards, Keerthy
On 2013-06-03 07:55, J, KEERTHY wrote: > 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: >> > >> > On 03/25/2013 11:29 PM, Stephen Warren wrote: >> > >> >> On 03/22/2013 08:55 AM, Ian Lartey wrote: >> >>> From: Graeme Gregory <gg@slimlogic.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
diff --git a/Documentation/devicetree/bindings/clock/palmas-clk.txt b/Documentation/devicetree/bindings/clock/palmas-clk.txt new file mode 100644 index 0000000..26fbc9f --- /dev/null +++ b/Documentation/devicetree/bindings/clock/palmas-clk.txt @@ -0,0 +1,27 @@ +* palmas and palmas-charger resource clock IP block devicetree bindings + +Required properties: +- compatible : Should be from the list + ti,twl6035-clk + ti,twl6036-clk + ti,twl6037-clk + ti,tps65913-clk + ti,tps65914-clk + ti,tps80036-clk +and also the generic series names + ti,palmas-clk + +Optional properties: +- ti,clk32g-mode-sleep - mode to adopt in pmic sleep 0 - off, 1 - on +- ti,clkg32kgaudio-mode-sleep - see above + +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"; +}; diff --git a/Documentation/devicetree/bindings/gpio/gpio-palmas.txt b/Documentation/devicetree/bindings/gpio/gpio-palmas.txt new file mode 100644 index 0000000..688eebb --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-palmas.txt @@ -0,0 +1,35 @@ +* palmas and palmas charger GPIO IP block devicetree bindings + +Required properties: +- compatible : Should be from the list + ti,twl6035-gpio + ti,twl6036-gpio + ti,twl6037-gpio + ti,tps65913-gpio + ti,tps65914-gpio + ti,tps80036-gpio + +and also the generic series names + + ti,palmas-gpio + +- gpio-controller: mark the device as a GPIO controller +- gpio-cells = <1>: GPIO lines are provided. +- 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 +- interrupt-parent : The parent interrupt controller. + +Example: + +gpio { + compatible = "ti,twl6035-gpio", "ti,palmas-gpio"; + + gpio-controller; + #gpio-cells = <1>; + interrupt-parent = <&palmas>; + interrupt-controller; + #interrupt-cells = <2>; +}; diff --git a/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt b/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt new file mode 100644 index 0000000..722ca94 --- /dev/null +++ b/Documentation/devicetree/bindings/input/palmas-pwrbutton.txt @@ -0,0 +1,26 @@ +* palmas and palmas-charger Button IP block devicetree bindings + +Required properties: +- compatible : Should be from the list + ti,twl6035-pwrbutton + ti,twl6036-pwrbutton + ti,twl6037-pwrbutton + ti,tps65913-pwrbutton + ti,tps65914-pwrbutton + ti,tps80036-pwrbutton +and also the generic series names + ti,palmas-pwrbutton + +- interrupts: the interrupt outputs of the controller. +- interrupt-names : Should be the name of irq resource. Each interrupt + binds its interrupt-name. +- interrupt-parent : The parent interrupt controller. + +Example: + +pwrbutton { + compatible = "ti,twl6035-pwrbutton", "ti,palmas-pwrbutton"; + interrupt-parent = <&palmas>; + interrupts = <1 0>; + interrupt-names = "pwron-irq"; +}; diff --git a/Documentation/devicetree/bindings/leds/leds-palmas.txt b/Documentation/devicetree/bindings/leds/leds-palmas.txt new file mode 100644 index 0000000..a825f43 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-palmas.txt @@ -0,0 +1,36 @@ +* palmas and palmas-charger LED IP block devicetree bindings + +Required properties: +- compatible : Should be from the list + ti,twl6035-leds + ti,twl6036-leds + ti,twl6037-leds + ti,tps65913-leds + ti,tps65914-leds + ti,tps80036-leds +and also the generic series names + ti,palmas-leds + +Optional properties: +-ti,led1-current - sink current setting 0 - 0mA, 1 - 25mA, 2 - 5mA, + 3 - 0mA, 4 - 5mA, 5 - 5mA, 6 - 10.0mA, 7 - 0mA +-ti,led2-current - see above +-ti,led3-current - see above +-ti,led4-current - see above +-ti,chrg-led-mode - only valid for charger - mode for charging led operation + 0 - Charging indicator + 1 - controlled as a general purpose LED +-ti,chrg-led-vbat-low - only valid for charger - blinking of low battery led + 0 - blinking is enabled, + 1 - blinking is disabled + +Example: +leds { + compatible = "ti,twl6035-leds", "ti,palmas-leds"; + ti,led1-current = <0>; + ti,led2-current = <0>; + ti,led3-current = <0>; + ti,led4-current = <0>; + ti,chrg-led-mode = <0>; + ti,chrg-led-vbat-low = <0>; +}; diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt new file mode 100644 index 0000000..84e5c2e --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/palmas.txt @@ -0,0 +1,81 @@ +* palmas and palmas-charger device tree bindings + +The TI palmas family current members :- +twl6035 (palmas) +twl6036 (palmas-charger) +twl6037 (palmas) +tps65913 (palmas) +tps65914 (palmas) +tps80036 (palmas-charger) + +Required properties: +- compatible : Should be from the list + ti,twl6035 + ti,twl6036 + ti,twl6037 + ti,tps65913 + ti,tps65914 + ti,tps80036 +and also the generic series names + ti,palmas + ti,palmas-charger +- 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 +- interrupt-parent : The parent interrupt controller. + +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: + +palmas { + compatible = "ti,twl6035", "ti,palmas"; + reg = <0x48> + interrupt-parent = <&intc>; + interrupt-controller; + #interrupt-cells = <2>; + + ti,mux-pad1 = <0>; + ti,mux-pad2 = <0>; + + #address-cells = <1>; + #size-cells = <0>; + + pmic { + compatible = "ti,twl6035-pmic", "ti,palmas-pmic"; + .... + } + + gpio { + compatible = "ti,twl6035-gpio", "ti,palmas-gpio"; + .... + }; + + wdt { + compatible = "ti,twl6035-wdt", "ti,palmas-wdt"; + .... + }; + + rtc { + compatible = "ti,twl6035-rtc", "ti,palmas-rtc"; + .... + }; + + pwrbutton { + compatible = "ti,twl6035-pwrbutton", "ti,palmas-pwrbutton"; + .... + }; + + leds { + compatible = "ti,twl6035-leds", "ti-palmas-leds"; + } + + clk { + compatible = "ti,twl6035-clk", "ti,palmas-clk"; + .... + }; +} diff --git a/Documentation/devicetree/bindings/regulator/palmas-pmic.txt b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt new file mode 100644 index 0000000..8097616 --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/palmas-pmic.txt @@ -0,0 +1,168 @@ +* palmas and palmas-charger regulator IP block devicetree bindings + +Required properties: +- compatible : Should be from the list + ti,twl6035-pmic + ti,twl6036-pmic + ti,twl6037-pmic + ti,tps65913-pmic + ti,tps65914-pmic + ti,tps80036-pmic +and also the generic series names + ti,palmas-pmic + +Optional properties: +- ti,ldo6-vibrator : ldo6 is in vibrator mode + +Optional nodes: +- regulators : should contain the constrains and init information for the + regulators. It should contain a subnode per regulator from the + list. + For ti,palmas-pmic - smps12, smps123, smps3 depending on OTP, + smps45, smps457, smps7 depending on varient, smps6, smps[8-10], + ldo[1-9], ldoln, ldousb + For ti,palmas-charger-pmic - smps12, smps123, smps3 depending on OTP, + smps[6-9], boost, ldo[1-14], ldoln, ldousb + + optional chip specific regulator fields :- + ti,warm-reset - maintain voltage during warm reset + ti,roof-floor - control voltage selection by pin + ti,sleep-mode - mode to adopt in pmic sleep 0 - off, 1 - auto, + 2 - eco, 3 - forced pwm + ti,tstep - slope control 0 - Jump, 1 10mV/us, 2 5mV/us, 3 2.5mV/us + ti,smps-range - OTP has the wrong range set for the hardware so override + 0 - low range, 1 - high range + +Example: + +pmic@0 { + compatible = "ti,twl6035-pmic", "ti,palmas-pmic"; + interrupt-parent = <&palmas>; + interrupts = <14 0>; + interrupt-name = "short-irq"; + + ti,ldo6_vibrator; + + regulators { + smps12_reg : smps12 { + regulator-name = "smps12"; + regulator-min-microvolt = < 600000>; + regulator-max-microvolt = <1500000>; + regulator-always-on; + regulator-boot-on; + ti,warm-reset; + ti,roof-floor; + ti,mode-sleep = <0>; + ti,tstep = <0>; + ti,smps-range = <1>; + }; + + smps3_reg: smps3 { + regulator-name = "smps3"; + regulator-min-microvolt = < 600000>; + regulator-max-microvolt = <1310000>; + }; + + smps45_reg: smps45 { + regulator-name = "smps45"; + regulator-min-microvolt = < 600000>; + regulator-max-microvolt = <1310000>; + }; + + smps6_reg: smps6 { + regulator-name = "smps6"; + regulator-min-microvolt = <1200000>; + regulator-max-microvolt = <1200000>; + }; + + smps7_reg: smps7 { + regulator-name = "smps7"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + + smps8_reg: smps8 { + regulator-name = "smps8"; + regulator-min-microvolt = < 600000>; + regulator-max-microvolt = <1310000>; + }; + + smps9_reg: smps9 { + regulator-name = "smps9"; + regulator-min-microvolt = <2100000>; + regulator-max-microvolt = <2100000>; + }; + + smps10_reg: smps10 { + regulator-name = "smps10"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + }; + + ldo1_reg: ldo1 { + regulator-name = "ldo1"; + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + }; + + ldo2_reg: ldo2 { + regulator-name = "ldo2"; + regulator-min-microvolt = <2900000>; + regulator-max-microvolt = <2900000>; + }; + + ldo3_reg: ldo3 { + regulator-name = "ldo3"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3000000>; + }; + + ldo4_reg: ldo4 { + regulator-name = "ldo4"; + regulator-min-microvolt = <2200000>; + regulator-max-microvolt = <2200000>; + }; + + ldo5_reg: ldo5 { + regulator-name = "ldo5"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + + ldo6_reg: ldo6 { + regulator-name = "ldo6"; + regulator-min-microvolt = <1500000>; + regulator-max-microvolt = <1500000>; + }; + + ldo7_reg: ldo7 { + regulator-name = "ldo7"; + regulator-min-microvolt = <1500000>; + regulator-max-microvolt = <1500000>; + }; + + ldo8_reg: ldo8 { + regulator-name = "ldo8"; + regulator-min-microvolt = <1500000>; + regulator-max-microvolt = <1500000>; + }; + + ldo9_reg: ldo9 { + regulator-name = "ldo9"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + + ldoln_reg: ldoln { + regulator-name = "ldoln"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + }; + + ldousb_reg: ldousb { + regulator-name = "ldousb"; + regulator-min-microvolt = <3250000>; + regulator-max-microvolt = <3250000>; + }; + }; +}; diff --git a/Documentation/devicetree/bindings/rtc/palmas-rtc.txt b/Documentation/devicetree/bindings/rtc/palmas-rtc.txt new file mode 100644 index 0000000..f405b36 --- /dev/null +++ b/Documentation/devicetree/bindings/rtc/palmas-rtc.txt @@ -0,0 +1,21 @@ +* palmas and palmas-charger RTC IP block devicetree bindings + +Required properties: +- compatible : Should be from the list + ti,twl6035-rtc + ti,twl6036-rtc + ti,twl6037-rtc + ti,tps65913-rtc + ti,tps65914-rtc + ti,tps80036-rtc +and also the generic series names + ti,palmas-rtc + +Examples: + +rtc { + compatible = "ti,twl6035-rtc", "ti,palmas-rtc"; + interrupt-parent = <&palmas>; + interrupts = <8 0 9 0>; + interrupt-name = "alarm-irq", "timer-irq"; +}; diff --git a/Documentation/devicetree/bindings/watchdog/palmas-wdt.txt b/Documentation/devicetree/bindings/watchdog/palmas-wdt.txt new file mode 100644 index 0000000..1553a0d --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/palmas-wdt.txt @@ -0,0 +1,21 @@ +* palmas and palmas-charger Watchdog IP block devicetree bindings + +Required properties: +- compatible : Should be from the list + ti,twl6035-wdt + ti,twl6036-wdt + ti,twl6037-wdt + ti,tps65913-wdt + ti,tps65914-wdt + ti,tps80036-wdt +and also the generic series names + ti,palmas-wdt + +Examples: + +wdt { + compatible = "ti,twl6035-wdt", "ti,palmas-wdt"; + interrupt-parent = <&palmas>; + interrupts = <10 0>; + interrupt-name = "watchdog-irq"; +};