On Monday 10 June 2013 10:27:05 Srinivas KANDAGATLA wrote:
> + soc {
> + pin-controller-sbc {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "st,stih416-pinctrl", "simple-bus";
Why is this both its own device with a compatible string and a
"simple-bus" at the same time? Wouldn't it be simpler to just
scan the child device nodes from the "st,stih416-pinctrl"
driver instead of having a separate platform_driver for them?
> + st,retime-in-delay = <0 300 500 750 1000 1250 1500
> 1750 2000 2250 2500 2750 3000 3250>;
> + st,retime-out-delay = <0 300 500 750 1000 1250 1500
> 1750 2000 2250 2500 2750 3000 3250>;
> + st,syscfg = <&syscfg_sbc>;
> + st,syscfg-offsets = <0 40 50 60 100>;
> + ranges;
> + PIO0: pinctrl@fe610000 {
> + #gpio-cells = <1>;
> + compatible = "st,stixxxx-gpio";
> + gpio-controller;
> + reg = <0xfe610000 0x100>;
> + st,bank-name = "PIO0";
> + st,retime-pin-mask = <0xff>;
> + };
> + PIO1: pinctrl@fe611000 {
> + #gpio-cells = <1>;
> + compatible = "st,stixxxx-gpio";
> + gpio-controller;
> + reg = <0xfe611000 0x100>;
> + st,bank-name = "PIO1";
> + st,retime-pin-mask = <0xff>;
> + };
What is in the ranges between these registers? It seems you have
256 bytes for each pinctrl node, with 4kb spacing. I wonder if
it would make sense to declare the entire range to belong to a single
pinctrl device. At least since all of the registers are in a single
range, you could add a property like
ranges = <0 0xfe610000 0x10000>;
and use relative addresses in the sub-nodes.
Please don't use identifiers with 'xxx' in them. Instead use numbers
of actual chips, ideally using the first one that this is compatible
with.
> + syscfg_sbc:syscfg@fe600000{
> + compatible = "st,stih416-syscfg";
> + reg = <0xfe600000 0x1000>;
> + syscfg-range = <0 999>;
> + syscfg-name = "SYSCFG_SBC";
> + };
> + syscfg_front:syscfg@fee10000{
> + compatible = "st,stih416-syscfg";
> + reg = <0xfee10000 0x1000>;
> + syscfg-range = <1000 999>;
> + syscfg-name = "SYSCFG_FRONT";
> + };
Did you mean to declare ranges excluding 1000 and 2000 here?
Normally I would expect inclusive ranges like syscfg-range=<0 1000>;
What is the idea of the 'syscfg-name'? If the nodes are all different,
I would expect them to have distinct "compatible" values and not
need them.
Arnd
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
