Re: [PATCH] dt-bindings: gpio: altera-fpga-mgr: Add Altera FPGA manager GPIO bindings

[Marek Vasut <marex@denx.de>]

On 10/16/2018 09:37 AM, Linus Walleij wrote:
> On Fri, Oct 12, 2018 at 5:12 PM Marek Vasut <marex@denx.de> wrote:
>> On 10/11/2018 10:44 AM, Linus Walleij wrote:
>>> On Wed, Oct 10, 2018 at 9:23 PM Marek Vasut <marex@denx.de> 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@denx.de>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> (...)
>>>
>>>> +- 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