On 10/17/2018 09:51 PM, Rob Herring wrote: > 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? We can, but won't it be easier to just reuse a binding which is already used by multiple compatibles ? -- Best regards, Marek Vasut
