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

[Bernd Edlinger <bernd.edlinger@xxxxxxxxxx>]

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@xxxxxxxxxx>
>> ---
>>   .../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.
��.n��������+%������w��{.n�����{��
b���ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f