On Wed, 2016-12-14 at 10:46 -0600, Rob Herring wrote:

quoted

quoted

quoted

+                   compatible = "aspeed,g5-pinctrl";

There's no register range for pinctrl?

This may be a mistake on my part; when I wrote this I had no experience
with writing devicetree bindings (and still don't have a lot).

The SCU does have register regions for pinctrl but on reflection I feel
neither the mfd nor syscon bindings describe how children's resources
should be treated in general. The example in the mfd bindings is for
hardware that is register-bit-led,compatible, whose bindings use the
'offset' property rather than 'reg', which still describes where, but
not using the reg property. Given my uncertainty with reg in an mfd
child, I wrote the pinctrl/pinmux driver using offsets from the base of
the SCU's syscon rather than describing the exact region(s) of the
syscon that should be used.

The issue you raise here occurred to me when writing the LPC Host
Controller bindings, but there I wasn't convinced using the ranges
property to give offsets was the right thing to do either.

Regardless, whilst there are two dedicated regions of pinmux registers,
the mux state also depends on bits in SCU registers outside of these
regions. Assuming we define an appropriate ranges property for the SCU
syscon the pinctrl reg property would look like:

    reg = <0x2c 0x1>, <0x3c 0x1>, <0x48 0x1>, <0x70 0x1>, <0x7c 0x1>, <0x80 0x18>, <0xa0 0x10>, <0xd0 0x1>;

This is the list of registers affecting the mux taken from the pinctrl-
aspeed.h.

With other registers in the holes, right? 

Yes.

If it is sparse like that,
then yes you probably just want to have reg in the parent for the
whole block.

Alright, we will leave it as-is.

quoted

What action do you recommend here? The pinctrl dts patches for the
Aspeed SoCs are yet to be applied, so changing the bindings to require
a reg property can't break any existing in-tree users as there are
none. The pinctrl driver can be patched to respect the reg property
after the fact, though actually using the region descriptions might be
interesting.

quoted

quoted

+                   aspeed,external-nodes = <&gfx, &lpchc>;

Did you have feedback on this approach? I queried you about it in the
previous revision, but never received a reply:

It seems okay. At least, I don't have a better suggestion.

Thanks.

Andrew