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. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |