Re: [RFC 6/6] ARM: dts: exynos4210: Add platform-specific descriptions for pin controllers

On 09/21/2012 01:54 PM, Tomasz Figa wrote:

On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:

quoted

On 09/20/2012 02:53 AM, Tomasz Figa wrote:

quoted

The patch "pinctrl: samsung: Parse pin banks from DT" introduced
platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device
tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

quoted

quoted

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

The properties above represent the width of the fields. Must all fields
always be packed together into say the LSB of the registers? What if
there are gaps between the fields in a future SoC variant, or the order
of the fields in the register changes? I think you want to add either a
samsung,func-bit/samsung,func-position property for each of the fields,
or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC,
the generic pinctrl binding used a mask for this purpose.

What if a future SoC variant adds more fields to the register? At that
point, you'd need to edit the driver anyway in order to define a new DT
property to represent the new field. Perhaps instead of having an
explicit named property per field in the register, you want a simple
list of fields:

samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn";
samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;

That would allow a completely arbitrary number of fields and layouts to
be described.

I wonder if for absolute generality you want a samsung,pin-stride
property to represent the difference in register address per pin. I
assume that's hard-coded as 4 right now.

Hmm, considering so many different possible changes, maybe a more 
conservative solution would be better, like reducing the amount of 
information held in DT to bank type, e.g.

	samsung,bank-type = "exynos4";

or maybe

	compatible = "samsung,exynos4-pin-bank*;

and then define supported bank types in the driver. SoC-specific data would 
remain in DT, i.e. pctl-offset, pin-bank, pin-count, eint-offset, etc.

Yes, removing much of the data from DT and putting it into a tiny table
in the driver makes sense to me in this case.

quoted

quoted

diff --git a/arch/arm/boot/dts/exynos4210.dtsi

b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644

--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi

@@ -59,6 +59,10 @@

 		reg = <0x11400000 0x1000>;
 		interrupts = <0 47 0>;
 		interrupt-controller;

+		samsung,geint-con = <0x700>;
+		samsung,geint-mask = <0x900>;
+		samsung,geint-pend = <0xA00>;
+		samsung,svc = <0xB08>;

I assume those new properties represent register addresses within the
block. If not, I don't understand what they are.

Yes, they do.

quoted

It's unclear to me why those properties aren't all part of
exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where
the register addresses for the pinctrl registers are the same (hence can
be in a shared exynos4210-pinctrl-banks.dtsi), yet the register
addresses for the geint registers are different (hence must be in
non-shared exynos4210.dtsi)?

Exynos4210-pincstrl-banks.dtsi isn't shared, it's specific to Exynos4210. 
Other SoCs are going to have their own whatever-pinctrl-banks.dtsi.

OK, so my point here is: why not put all the pinctrl-related properties
into a single file, rather than spreading them across different files.
Having separate files makes sense if they can be re-used in different
places, but not if they're single-use.