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

[Marek Vasut <marex@denx.de>]

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@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 ? :)
> 
> 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