On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:
> 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.
A hybrid solution came to my mind, define bank types in device tree once
and then reference them in banks. Something like:
pinctrl-bank-types {
bank_off: bank-off {
samsung,func-width = <4>;
samsung,pud-width = <2>;
samsung,drv-width = <2>;
samsung,conpdn-width = <2>;
samsung,pudpdn-width = <2>;
};
bank_alive: bank-alive {
samsung,func-width = <4>;
samsung,pud-width = <2>;
samsung,drv-width = <2>;
};
};
/* ... */
pinctrl@12345678 {
/* ... */
gpa0: gpa0 {
/* ... */
samsung,bank-type = <&bank_off>;
/* ... */
};
/* ... */
};
This would add the possibility to define new banks types quickly, but would
not add too much overhead.
What do you think?
> >>> 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.
All the definitions in device tree for pinctrl take lots of lines and so I
though it would make it more readable if pin groups would have its own
source file and so would pin banks.
Now that I think of it, they aren't going to be modified too much, so it
might be better indeed to put them together in a single file.
Best regards,
Tomasz Figa
--
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
