| Message ID | 20180828112721.28178-3-Eugeniy.Paltsev@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | GPIO: add single-register GPIO via CREG driver | expand |
On Tue, Aug 28, 2018 at 02:27:21PM +0300, Eugeniy Paltsev wrote: > This patch adds documentation of device tree bindings for the Synopsys > GPIO via CREG driver. > > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> > --- > .../devicetree/bindings/gpio/snps,creg-gpio.txt | 49 ++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > > diff --git a/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > new file mode 100644 > index 000000000000..eb022d44ccda > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > @@ -0,0 +1,49 @@ > +GPIO via CREG (Control REGisers) driver REGisters? Bindings don't describe drivers. > + > +This is is single-register MMIO GPIO driver to control such strangely mapped > +outputs: > + > +31 11 8 7 5 0 < bit number > +| | | | | | > +[ not used | gpio-1 | shift-1 | gpio-0 | shift-0 ] < 32 bit MMIO register > + ^ ^ > + | | > + | write 0x2 == set output to "1" (on) > + | write 0x3 == set output to "0" (off) > + | > + write 0x1 == set output to "1" (on) > + write 0x4 == set output to "0" (off) What kind of crazy h/w designer designed this? > +Required properties: > +- compatible : "snps,creg-gpio" > +- reg : Exactly one register range with length 0x4. > +- #gpio-cells : Should be one - the pin number. > +- gpio-controller : Marks the device node as a GPIO controller. > +- snps,ngpios: Number of GPIO pins. > +- snps,bit-per-line: Number of bits per each gpio line (see picture). > + Array the size of "snps,ngpios" > +- snps,shift: Shift (in bits) of the each GPIO field from the previous one in > + register (see picture). Array the size of "snps,ngpios" > +- snps,on-val: Value should be set in corresponding field to set > + output to "1" (see picture). Array the size of "snps,ngpios" > +- snps,off-val: Value should be set in corresponding field to set > + output to "0" (see picture). Array the size of "snps,ngpios" Convince me we need to parameterize all this. We try to avoid describing h/w like this. > + > +Optional properties: > +- snps,default-val: default output field values. Array the size of "snps,ngpios" > + > +Example (see picture): > + > +gpio: gpio@f00014b0 { > + compatible = "snps,creg-gpio"; > + reg = <0xf00014b0 0x4>; > + gpio-controller; > + #gpio-cells = <1>; > + snps,ngpios = <2>; > + snps,shift = <5 1>; > + snps,bit-per-line = <2 3>; > + snps,on-val = <2 1>; > + snps,off-val = <3 4>; > + snps,default-val = <2 1>; > +}; > -- > 2.14.4 >
On Tue, Aug 28, 2018 at 1:27 PM Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> wrote: > +++ b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > @@ -0,0 +1,49 @@ > +GPIO via CREG (Control REGisers) driver Speling Also should be "Synopsys GPIO via CREG" as this is likely just for Synopsys and not general purpose. > +This is is single-register MMIO GPIO driver to control such strangely mapped > +outputs: > + > +31 11 8 7 5 0 < bit number > +| | | | | | > +[ not used | gpio-1 | shift-1 | gpio-0 | shift-0 ] < 32 bit MMIO register > + ^ ^ > + | | > + | write 0x2 == set output to "1" (on) > + | write 0x3 == set output to "0" (off) > + | > + write 0x1 == set output to "1" (on) > + write 0x4 == set output to "0" (off) Move this documentation into the driver instead. > +Required properties: > +- compatible : "snps,creg-gpio" > +- reg : Exactly one register range with length 0x4. > +- #gpio-cells : Should be one - the pin number. > +- gpio-controller : Marks the device node as a GPIO controller. OK > +- snps,ngpios: Number of GPIO pins. Use the existing ngpios attribute for this, see gpio.txt > +- snps,bit-per-line: Number of bits per each gpio line (see picture). > + Array the size of "snps,ngpios" > +- snps,shift: Shift (in bits) of the each GPIO field from the previous one in > + register (see picture). Array the size of "snps,ngpios" > +- snps,on-val: Value should be set in corresponding field to set > + output to "1" (see picture). Array the size of "snps,ngpios" > +- snps,off-val: Value should be set in corresponding field to set > + output to "0" (see picture). Array the size of "snps,ngpios" Move this into a lookup table in the driver instead, and match the lookup table to the compatible string. The format of the register is known for a certain compatible, right? > +Optional properties: > +- snps,default-val: default output field values. Array the size of "snps,ngpios" Default values for different lines can be achieved by hogs if it's OK to tie them up perpetually, else work on creating generic inialization values in gpio.txt and implement that in gpiolib-of.c for everyone. This discussion comes up from time to time. Yours, Linus Walleij
On Tue, 2018-08-28 at 20:02 -0500, Rob Herring wrote: > On Tue, Aug 28, 2018 at 02:27:21PM +0300, Eugeniy Paltsev wrote: > > This patch adds documentation of device tree bindings for the Synopsys > > GPIO via CREG driver. > > > > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> > > --- > > .../devicetree/bindings/gpio/snps,creg-gpio.txt | 49 ++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > > > > diff --git a/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > > new file mode 100644 > > index 000000000000..eb022d44ccda > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > > @@ -0,0 +1,49 @@ > > +GPIO via CREG (Control REGisers) driver > > Bindings don't describe drivers. > > > + > > +This is is single-register MMIO GPIO driver to control such strangely mapped > > +outputs: > > + > > +31 11 8 7 5 0 < bit number > > +| | | | | | > > +[ not used | gpio-1 | shift-1 | gpio-0 | shift-0 ] < 32 bit MMIO register > > + ^ ^ > > + | | > > + | write 0x2 == set output to "1" (on) > > + | write 0x3 == set output to "0" (off) > > + | > > + write 0x1 == set output to "1" (on) > > + write 0x4 == set output to "0" (off) > > What kind of crazy h/w designer designed this? Actually this fields in register controls some multiplexers, which we want to use as IO port, see the example: /| / | | |------ some internal line IO PIN -------| |------ logic 0 | |------ logic 1 | |------ not used \ | |\| | CREG field > > +Required properties: > > +- compatible : "snps,creg-gpio" > > +- reg : Exactly one register range with length 0x4. > > +- #gpio-cells : Should be one - the pin number. > > +- gpio-controller : Marks the device node as a GPIO controller. > > +- snps,ngpios: Number of GPIO pins. > > +- snps,bit-per-line: Number of bits per each gpio line (see picture). > > + Array the size of "snps,ngpios" > > +- snps,shift: Shift (in bits) of the each GPIO field from the previous one in > > + register (see picture). Array the size of "snps,ngpios" > > +- snps,on-val: Value should be set in corresponding field to set > > + output to "1" (see picture). Array the size of "snps,ngpios" > > +- snps,off-val: Value should be set in corresponding field to set > > + output to "0" (see picture). Array the size of "snps,ngpios" > > Convince me we need to parameterize all this. We try to avoid describing > h/w like this. Well, I going to use this driver on 3 already upstreamed platforms and one upcoming. They all have such CREG 'GPIOs' differently mapped with different IO lines number, different enable/disable value, etc... So I really want to create some generic and configurable driver to handle both existing and upcoming platforms. > > + > > +Optional properties: > > +- snps,default-val: default output field values. Array the size of "snps,ngpios" > > + > > +Example (see picture): > > + > > +gpio: gpio@f00014b0 { > > + compatible = "snps,creg-gpio"; > > + reg = <0xf00014b0 0x4>; > > + gpio-controller; > > + #gpio-cells = <1>; > > + snps,ngpios = <2>; > > + snps,shift = <5 1>; > > + snps,bit-per-line = <2 3>; > > + snps,on-val = <2 1>; > > + snps,off-val = <3 4>; > > + snps,default-val = <2 1>; > > +}; > > -- > > 2.14.4 > >
On Thu, 2018-08-30 at 10:43 +0200, Linus Walleij wrote: > On Tue, Aug 28, 2018 at 1:27 PM Eugeniy Paltsev > <Eugeniy.Paltsev@synopsys.com> wrote: > > > +++ b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt > > @@ -0,0 +1,49 @@ > > +GPIO via CREG (Control REGisers) driver [snip] > > +- snps,ngpios: Number of GPIO pins. > > Use the existing ngpios attribute for this, see gpio.txt Ok > > +- snps,bit-per-line: Number of bits per each gpio line (see picture). > > + Array the size of "snps,ngpios" > > +- snps,shift: Shift (in bits) of the each GPIO field from the previous one in > > + register (see picture). Array the size of "snps,ngpios" > > +- snps,on-val: Value should be set in corresponding field to set > > + output to "1" (see picture). Array the size of "snps,ngpios" > > +- snps,off-val: Value should be set in corresponding field to set > > + output to "0" (see picture). Array the size of "snps,ngpios" > > Move this into a lookup table in the driver instead, and match > the lookup table to the compatible string. The format of the > register is known for a certain compatible, right? Actually I really don't want to hardcode this values into lookup table as I going to use this driver on 3 already upstreamed platforms and at least one upcoming. They all have such CREG pseudo-'GPIOs' differently mapped with different IO lines number, different enable/disable value, etc... Is it really a problem to have this values configured via device tree? If we read them from DT we are able to use this generic and configurable driver to handle both existing and upcoming platforms without the need of patching the driver on every new platform upstreaming. > > Yours, > Linus Walleij
On Thu, Aug 30, 2018 at 8:16 PM Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> wrote: > On Thu, 2018-08-30 at 10:43 +0200, Linus Walleij wrote: > > > +- snps,bit-per-line: Number of bits per each gpio line (see picture). > > > + Array the size of "snps,ngpios" > > > +- snps,shift: Shift (in bits) of the each GPIO field from the previous one in > > > + register (see picture). Array the size of "snps,ngpios" > > > +- snps,on-val: Value should be set in corresponding field to set > > > + output to "1" (see picture). Array the size of "snps,ngpios" > > > +- snps,off-val: Value should be set in corresponding field to set > > > + output to "0" (see picture). Array the size of "snps,ngpios" > > > > Move this into a lookup table in the driver instead, and match > > the lookup table to the compatible string. The format of the > > register is known for a certain compatible, right? > > Actually I really don't want to hardcode this values into lookup table as I going to use > this driver on 3 already upstreamed platforms and at least one upcoming. > > They all have such CREG pseudo-'GPIOs' differently mapped with different IO lines number, > different enable/disable value, etc... So each of them will have their own compatible, and table entry, so what's the problem? If they don't have their own compatible, they should be added, because they are per definition not compatible if they need different values into different parts of the register. > Is it really a problem to have this values configured via device tree? Yes because the DT maintainers do not like that we use the device tree as a data dumping ground. pinctrl-single.c and some other real big pin controllers are dumping data into the device tree, but it is a dubious practice. Two wrongs doesn't make one right. > If we read them from DT we are able to use this generic and configurable driver to handle > both existing and upcoming platforms without the need of patching the driver on every new > platform upstreaming. But you will have to patch the driver to add a new compatible for each platform you're upstreaming anyway, so this isn't going to make things easier. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt new file mode 100644 index 000000000000..eb022d44ccda --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt @@ -0,0 +1,49 @@ +GPIO via CREG (Control REGisers) driver + +This is is single-register MMIO GPIO driver to control such strangely mapped +outputs: + +31 11 8 7 5 0 < bit number +| | | | | | +[ not used | gpio-1 | shift-1 | gpio-0 | shift-0 ] < 32 bit MMIO register + ^ ^ + | | + | write 0x2 == set output to "1" (on) + | write 0x3 == set output to "0" (off) + | + write 0x1 == set output to "1" (on) + write 0x4 == set output to "0" (off) + + +Required properties: +- compatible : "snps,creg-gpio" +- reg : Exactly one register range with length 0x4. +- #gpio-cells : Should be one - the pin number. +- gpio-controller : Marks the device node as a GPIO controller. +- snps,ngpios: Number of GPIO pins. +- snps,bit-per-line: Number of bits per each gpio line (see picture). + Array the size of "snps,ngpios" +- snps,shift: Shift (in bits) of the each GPIO field from the previous one in + register (see picture). Array the size of "snps,ngpios" +- snps,on-val: Value should be set in corresponding field to set + output to "1" (see picture). Array the size of "snps,ngpios" +- snps,off-val: Value should be set in corresponding field to set + output to "0" (see picture). Array the size of "snps,ngpios" + +Optional properties: +- snps,default-val: default output field values. Array the size of "snps,ngpios" + +Example (see picture): + +gpio: gpio@f00014b0 { + compatible = "snps,creg-gpio"; + reg = <0xf00014b0 0x4>; + gpio-controller; + #gpio-cells = <1>; + snps,ngpios = <2>; + snps,shift = <5 1>; + snps,bit-per-line = <2 3>; + snps,on-val = <2 1>; + snps,off-val = <3 4>; + snps,default-val = <2 1>; +};
This patch adds documentation of device tree bindings for the Synopsys GPIO via CREG driver. Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> --- .../devicetree/bindings/gpio/snps,creg-gpio.txt | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/snps,creg-gpio.txt