[PATCHv3 1/2] Add device tree bindings for Altera FPGA Manager GPIO

[PATCHv3 1/2] Add device tree bindings for Altera FPGA Manager GPIO

[Bernd Edlinger]

These are the bindings for the gpio-altera-fpgamgr driver.

Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
---
 .../bindings/gpio/gpio-altera-fpgamgr.txt          | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt b/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
new file mode 100644
index 0000000..7e9434f
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
@@ -0,0 +1,45 @@
+Altera FPGA Manager GPIO controller bindings
+
+Required controller properties:
+- #address-cells : Should be 1
+- #size-cells : Should be 0
+- compatible:
+  - "altr,fpgamgr-gpio"
+- reg: Physical base address and length of the controller's registers.
+- status : "okay" or "disabled".
+
+The FPGA Manager has two 32-bit ports, one for input and one for output.
+
+Port properties:
+- compatible:
+  - "altr,fpgamgr-gpio-output"
+  - "altr,fpgamgr-gpio-input"
+- #gpio-cells : Should be 2
+  - The first cell is the gpio offset number.
+  - The second cell is reserved and is currently unused.
+- gpio-controller : Marks the device node as a GPIO controller.
+- reg : Port number, 0 for output, 1 for input.
+
+Example:
+
+gpio3: gpio@ff706010 {
+  #address-cells = <1>;
+  #size-cells = <0>;
+  compatible = "altr,fpgamgr-gpio";
+  reg = <0xff706010 0x8>;
+  status = "okay";
+
+  portd: gpio-controller@0 {
+    compatible = "altr,fpgamgr-gpio-output";
+    gpio-controller;
+    #gpio-cells = <2>;
+    reg = <0>;
+  };
+
+  porte: gpio-controller@1 {
+    compatible = "altr,fpgamgr-gpio-input";
+    gpio-controller;
+    #gpio-cells = <2>;
+    reg = <1>;
+  };
+};
-- 
1.9.1

Re: [PATCHv3 1/2] Add device tree bindings for Altera FPGA Manager GPIO

[Rob Herring]

On Sun, Oct 08, 2017 at 11:30:27AM +0000, Bernd Edlinger wrote:
> These are the bindings for the gpio-altera-fpgamgr driver.

Bindings are for h/w devices, not drivers.

> 
> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
> ---
>  .../bindings/gpio/gpio-altera-fpgamgr.txt          | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt b/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
> new file mode 100644
> index 0000000..7e9434f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
> @@ -0,0 +1,45 @@
> +Altera FPGA Manager GPIO controller bindings
> +
> +Required controller properties:
> +- #address-cells : Should be 1
> +- #size-cells : Should be 0
> +- compatible:
> +  - "altr,fpgamgr-gpio"

Seems kind of generic. Only 1 implementation of h/w?

> +- reg: Physical base address and length of the controller's registers.
> +- status : "okay" or "disabled".

No need to document status.

> +
> +The FPGA Manager has two 32-bit ports, one for input and one for output.

This sounds more like a system control/status register to tie off all 
the leftover signals than a GPIO block. Linus likely has some opinions 
on that.

> +
> +Port properties:
> +- compatible:
> +  - "altr,fpgamgr-gpio-output"
> +  - "altr,fpgamgr-gpio-input"
> +- #gpio-cells : Should be 2
> +  - The first cell is the gpio offset number.
> +  - The second cell is reserved and is currently unused.
> +- gpio-controller : Marks the device node as a GPIO controller.
> +- reg : Port number, 0 for output, 1 for input.
> +
> +Example:
> +
> +gpio3: gpio@ff706010 {
> +  #address-cells = <1>;
> +  #size-cells = <0>;
> +  compatible = "altr,fpgamgr-gpio";
> +  reg = <0xff706010 0x8>;
> +  status = "okay";
> +
> +  portd: gpio-controller@0 {
> +    compatible = "altr,fpgamgr-gpio-output";
> +    gpio-controller;
> +    #gpio-cells = <2>;
> +    reg = <0>;
> +  };
> +
> +  porte: gpio-controller@1 {
> +    compatible = "altr,fpgamgr-gpio-input";
> +    gpio-controller;
> +    #gpio-cells = <2>;
> +    reg = <1>;
> +  };

These child nodes don't really add anything. Can't you just define the 
controller has 64 lines with the 1st 32 being outputs.

Rob

Re: [PATCHv3 1/2] Add device tree bindings for Altera FPGA Manager GPIO

[Bernd Edlinger]

On 10/13/17 22:06, Rob Herring wrote:
> On Sun, Oct 08, 2017 at 11:30:27AM +0000, Bernd Edlinger wrote:
>> These are the bindings for the gpio-altera-fpgamgr driver.
> 
> Bindings are for h/w devices, not drivers.
> 

I should drop that sentence then?
The first line is already:
"Add device tree bindings for Altera FPGA Manager GPIO"

>>
>> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
>> ---
>>   .../bindings/gpio/gpio-altera-fpgamgr.txt          | 45 ++++++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt b/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
>> new file mode 100644
>> index 0000000..7e9434f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
>> @@ -0,0 +1,45 @@
>> +Altera FPGA Manager GPIO controller bindings
>> +
>> +Required controller properties:
>> +- #address-cells : Should be 1
>> +- #size-cells : Should be 0
>> +- compatible:
>> +  - "altr,fpgamgr-gpio"
> 
> Seems kind of generic. Only 1 implementation of h/w?

I guess, there are lots of Altera devices with similar peripherals.

> 
>> +- reg: Physical base address and length of the controller's registers.
>> +- status : "okay" or "disabled".
> 
> No need to document status.
> 

Ok, will remove here and in the Example.

>> +
>> +The FPGA Manager has two 32-bit ports, one for input and one for output.
> 
> This sounds more like a system control/status register to tie off all
> the leftover signals than a GPIO block. Linus likely has some opinions
> on that.
> 

I don't think so, the only difference is that the signals are not at the
external boundary, instead they connect internally the FPGA logic.
What these signals control depends entirely on the user's logic design.

>> +
>> +Port properties:
>> +- compatible:
>> +  - "altr,fpgamgr-gpio-output"
>> +  - "altr,fpgamgr-gpio-input"
>> +- #gpio-cells : Should be 2
>> +  - The first cell is the gpio offset number.
>> +  - The second cell is reserved and is currently unused.
>> +- gpio-controller : Marks the device node as a GPIO controller.
>> +- reg : Port number, 0 for output, 1 for input.
>> +
>> +Example:
>> +
>> +gpio3: gpio@ff706010 {
>> +  #address-cells = <1>;
>> +  #size-cells = <0>;
>> +  compatible = "altr,fpgamgr-gpio";
>> +  reg = <0xff706010 0x8>;
>> +  status = "okay";
>> +
>> +  portd: gpio-controller@0 {
>> +    compatible = "altr,fpgamgr-gpio-output";
>> +    gpio-controller;
>> +    #gpio-cells = <2>;
>> +    reg = <0>;
>> +  };
>> +
>> +  porte: gpio-controller@1 {
>> +    compatible = "altr,fpgamgr-gpio-input";
>> +    gpio-controller;
>> +    #gpio-cells = <2>;
>> +    reg = <1>;
>> +  };
> 
> These child nodes don't really add anything. Can't you just define the
> controller has 64 lines with the 1st 32 being outputs.
> 

Other device bindings can refer to the single bits in each port
as "<&portd 1 0>" or "<&porte 2 0>" which is more mnemonic,
but it is possible that this makes no difference to the user mode.

The bindings are structured similar to what's in
Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
where the bindings are structured in the same way.


Bernd.

Re: [PATCHv3 1/2] Add device tree bindings for Altera FPGA Manager GPIO

[Rob Herring]

On Mon, Oct 16, 2017 at 12:04 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
> On 10/13/17 22:06, Rob Herring wrote:
>> On Sun, Oct 08, 2017 at 11:30:27AM +0000, Bernd Edlinger wrote:
>>> These are the bindings for the gpio-altera-fpgamgr driver.
>>
>> Bindings are for h/w devices, not drivers.
>>
>
> I should drop that sentence then?

Yes, but you need some commit msg besides just the subject.

> The first line is already:
> "Add device tree bindings for Altera FPGA Manager GPIO"
>
>>>
>>> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
>>> ---
>>>   .../bindings/gpio/gpio-altera-fpgamgr.txt          | 45 ++++++++++++++++++++++
>>>   1 file changed, 45 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt b/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
>>> new file mode 100644
>>> index 0000000..7e9434f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-altera-fpgamgr.txt
>>> @@ -0,0 +1,45 @@
>>> +Altera FPGA Manager GPIO controller bindings
>>> +
>>> +Required controller properties:
>>> +- #address-cells : Should be 1
>>> +- #size-cells : Should be 0
>>> +- compatible:
>>> +  - "altr,fpgamgr-gpio"
>>
>> Seems kind of generic. Only 1 implementation of h/w?
>
> I guess, there are lots of Altera devices with similar peripherals.
>
>>
>>> +- reg: Physical base address and length of the controller's registers.
>>> +- status : "okay" or "disabled".
>>
>> No need to document status.
>>
>
> Ok, will remove here and in the Example.
>
>>> +
>>> +The FPGA Manager has two 32-bit ports, one for input and one for output.
>>
>> This sounds more like a system control/status register to tie off all
>> the leftover signals than a GPIO block. Linus likely has some opinions
>> on that.
>>
>
> I don't think so, the only difference is that the signals are not at the
> external boundary, instead they connect internally the FPGA logic.
> What these signals control depends entirely on the user's logic design.


>>> +
>>> +Port properties:
>>> +- compatible:
>>> +  - "altr,fpgamgr-gpio-output"
>>> +  - "altr,fpgamgr-gpio-input"
>>> +- #gpio-cells : Should be 2
>>> +  - The first cell is the gpio offset number.
>>> +  - The second cell is reserved and is currently unused.
>>> +- gpio-controller : Marks the device node as a GPIO controller.
>>> +- reg : Port number, 0 for output, 1 for input.
>>> +
>>> +Example:
>>> +
>>> +gpio3: gpio@ff706010 {
>>> +  #address-cells = <1>;
>>> +  #size-cells = <0>;
>>> +  compatible = "altr,fpgamgr-gpio";
>>> +  reg = <0xff706010 0x8>;
>>> +  status = "okay";
>>> +
>>> +  portd: gpio-controller@0 {
>>> +    compatible = "altr,fpgamgr-gpio-output";
>>> +    gpio-controller;
>>> +    #gpio-cells = <2>;
>>> +    reg = <0>;
>>> +  };
>>> +
>>> +  porte: gpio-controller@1 {
>>> +    compatible = "altr,fpgamgr-gpio-input";
>>> +    gpio-controller;
>>> +    #gpio-cells = <2>;
>>> +    reg = <1>;
>>> +  };
>>
>> These child nodes don't really add anything. Can't you just define the
>> controller has 64 lines with the 1st 32 being outputs.
>>
>
> Other device bindings can refer to the single bits in each port
> as "<&portd 1 0>" or "<&porte 2 0>" which is more mnemonic,
> but it is possible that this makes no difference to the user mode.
>
> The bindings are structured similar to what's in
> Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> where the bindings are structured in the same way.

The DW GPIO was a bit of an oddball IIRC, so I don't think I'd copy

>
>
> Bernd.