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

[Stephen Warren <swarren@wwwdotorg.org>]

On 09/21/2012 01:54 PM, Tomasz Figa wrote:
> On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:
>> On 09/20/2012 02:53 AM, Tomasz Figa wrote:
>>> 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

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

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