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

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

[Marek Vasut]

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>
---
 .../bindings/gpio/gpio-altera-fpga-mgr.txt    | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera-fpga-mgr.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera-fpga-mgr.txt b/Documentation/devicetree/bindings/gpio/gpio-altera-fpga-mgr.txt
new file mode 100644
index 000000000000..74a996a6b72f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-altera-fpga-mgr.txt
@@ -0,0 +1,24 @@
+Altera SoCFPGA FPGA manager GPIO controller.
+
+The Altera SoCFPGA fpgamgr block has a GPIO controller as a part of it.
+The controller has two sets of pins, 32 GPIs and 32 GPOs.
+
+Required properties:
+- compatible: Should contain "altr,fpga-mgr-gpio".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be 2. The first cell is the pin number and
+  the second cell is used to specify the gpio polarity:
+    0 = Active high,
+    1 = Active low.
+- 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.
+
+Example:
+
+	fpgamgr_gpio: gpio@ff706010 {
+		compatible = "altr,fpga-mgr-gpio";
+		gpio-controller;
+		#gpio-cells = <2>;
+		gpio,syscon-dev = <&fpgamgr0 0x10>;
+	};
-- 
2.18.0

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

[Linus Walleij]

Hi Marek,

thanks for the patch!

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.

Yours,
Linus Walleij

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

[Marek Vasut]

On 10/11/2018 10:44 AM, Linus Walleij wrote:
> Hi Marek,

Hi!

> thanks for the patch!

thanks for the feedback!

> 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.

-- 
Best regards,
Marek Vasut

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

[Linus Walleij]

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...

Yours,
Linus Walleij

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

[Marek Vasut]

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

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

[Rob Herring]

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?

Rob

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

[Marek Vasut]

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

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

[Rob Herring]

On Wed, Oct 17, 2018 at 4:37 PM Marek Vasut <marex@denx.de> wrote:
>
> 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 ?

The gpio-syscon driver will lookup by compatible (of the syscon),
phandle (gpio,syscon-dev) or parent device. So I don't see any
difference in reuse. Unless there is some compelling reason to put
this node elsewhere, the most logical spot for a sub-function is under
its parent.

Rob