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 ? :) -- Best regards, Marek Vasut
