On Tue, Oct 16, 2018 at 11:26:02AM +0200, Marek Vasut wrote: > On 10/16/2018 09:37 AM, Linus Walleij wrote: > > On Fri, Oct 12, 2018 at 5:12 PM Marek Vasut <marex@xxxxxxx> wrote: > >> On 10/11/2018 10:44 AM, Linus Walleij wrote: > >>> On Wed, Oct 10, 2018 at 9:23 PM Marek Vasut <marex@xxxxxxx> wrote: > >>> > >>>> Add DT bindings for the GPI / GPO block in the Altera SoCFPGA FPGA manager. > >>>> The GPIO block in the FPGA manager has two 32bit registers, one for setting > >>>> 32 GPOs and another one for reading 32 GPIs, both of which can be mapped to > >>>> separate physical pads. > >>>> > >>>> Signed-off-by: Marek Vasut <marex@xxxxxxx> > >>>> Cc: Rob Herring <robh+dt@xxxxxxxxxx> > >>>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx> > >>> (...) > >>> > >>>> +- gpio,syscon-dev: phandle/offset pair. The phandle to syscon used to > >>>> + access device state control registers and the offset of device's specific > >>>> + registers within device state control registers range. > >>> (...) > >>>> + gpio,syscon-dev = <&fpgamgr0 0x10>; > >>> > >>> I didn't see that before. > >>> > >>> It is usually not a good idea to encode register offsets into the > >>> device tree. > >>> > >>> I think the register offset should be in the driver and determined > >>> from the compatible-string. If that is not possible, the compatible > >>> strings do not really indicate compatibility, if you see what I mean. > >>> > >>> As for the name of the variable, why not just use: > >>> > >>> syscon = <&fpgamgr0>; > >>> > >>> It seems simple enough without any gpio,* prefix or explicitly > >>> suffixing it with a "-dev" - every node in the device tree is a device > >>> by definition so skip that. > >> > >> Isn't it better to just have one compatible string for all SoCFPGAs and > >> handle the possible difference in offset where the registers are in DT? > >> It's the same as "reg" property which we use to describe where a certain > >> block is in the address space. > > > > You have a point. > > > > What about: > > > > syscon = <&fpgamgr0>; > > reg = <0x10>; > > ? > > > > You can just parse out "reg" in the driver. > > > > I mean reg is intuitively for that, so... > > But drivers/gpio/gpio-syscon.c already parses the gpio,syscon-dev > binding, including the register offset. Why reinvent a new one if there > already is one which fits perfectly ? :) Maybe so, but it is undocumented. And if you look at the Rockchip addition to the driver, it was done a different way. Can't this be a child of the fpgamgr? Rob
