[PATCH 01/16] ARM: dts: exynos4210: Replace legacy GPIO bank nodes with pinctrl bank nodes

On Wednesday 10 of October 2012 09:26:51 Linus Walleij wrote:

On Mon, Oct 8, 2012 at 10:39 AM, Tomasz Figa [off-list ref] wrote:

quoted

Seuqential patches from this series introduce SoC-specific data parsing
from device tree.

This patch removes legacy GPIO bank nodes from exynos4210.dtsi and
replaces them with nodes and properties required for these patches.

So to be clear:

quoted

+       pinctrl-bank-types {
+               bank_off: bank-off {
+                       samsung,reg-names = "func", "dat", "pud",
+                                               "drv", "conpdn",
"pudpdn"; +                       samsung,reg-params = <0x00 4>, <0x04
1>, <0x08 2>, +                                               <0x0C
2>, <0x10 2>, <0x14 2>; +               };

This is starting to look like a firmware language, I have mixed
feelings about this. Shall this be read:

"Poke 4 into 0x00, poke 1 into 0x04, poke 2 into 0x08" etc?

I'm not sure if I understood you correctly, so let me explain how this 
works.

Each specifier defines register offset inside bank registers and how many 
bits are used for one pin in this register to specify configuration value.

E.g. func register is available at offset 0x00 and pin 0 occupies bits 0-3, 
pin 1 bit 4-7, etc.

We really need to discuss this, Grant has already NACK:ed
such approaches once.

If you're still going to do this, it is mandatory
to NOT use magic hex numbers anymore, because Stephen has
merged preprocessor support to the DTC compiler so you
can use #defined macros.

See commit:
cd296721a9645f9f28800a072490fa15458d1fb7

That's definitely nice. I have seen the work going on this before, but 
haven't followed it recently. Good to know that now it can be used.
 

quoted

+       pinctrl at 11400000 {
+               gpa0: gpa0 {
+                       gpio-controller;
+                       samsung,pctl-offset = <0x000>;
+                       samsung,pin-count = <8>;
+                       samsung,bank-type = <&bank_off>;
+                       #gpio-cells = <2>;

This part is OK.

quoted

+
+                       interrupt-controller;
+                       samsung,eint-offset = <0x00>;

This property is *NOT* OK. IMHO the driver should know these
offsets, not the device tree. The driver only needs the offset to
the register range, what registers there are and their names
should be #defined.

This is an offset inside of EINT register group. EINT registers are 
organized in groups as following:

EINT_CON_0
EINT_CON_1
...
EINT_CON_N

...

EINT_MASK_0
EINT_MASK_1
...
EINT_MASK_N

...

EINT_PEND_0
EINT_PEND_1
...
EINT_PEND_N

With arbitrary order of particular groups, arbitrary space between groups 
and arbitrary mapping of particular registers to pin banks, although the 
mapping is the same for all groups of registers, that's why there is only 
one eint-offset property. Also holes (reserved/unused registers) inside 
groups might exist.

So if we want to access EINT_MASK register of bank A0 (of pinctrl 0), we 
must construct the address as following:

eint_mask_a0 = pinctrl_0_base + pinctrl_0_geint_mask + bank_a0_eint_offset

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center