| Message ID | 20181010192315.26163-1-marex@denx.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dt-bindings: gpio: altera-fpga-mgr: Add Altera FPGA manager GPIO bindings | expand |
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
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.
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
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 ? :)
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
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 ?
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
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>; + };
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