lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
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