gpio: lpc32xx: add new LPC32xx GPIO controller driver
Subsystems (3)
Vladimir Zapolskiy
maintainercover gpio: lpc32xx: add new LPC32xx GPIO controller driver
The changeset updates NXP LPC32xx GPIO controller driver on basis of the new LPC32xx irqchip driver.
Unfortunately the old driver has too many hacks inherited from the ancient legacy NXP BSP and I find it can not be maintained, the new driver has compatible interface to device tree clients, so moving to the new driver should not impact any GPIO consumers.
The changeset has dependencies on the recent updates to LPC32xx DTS:
* http://permalink.gmane.org/gmane.linux.ports.arm.kernel/456304
* Recent LPC32xx CCF series (no link at the moment)
* Recent LPC32xx irqchip series (no link at the moment)
Vladimir Zapolskiy (4):
dt-bindings: gpio: update desription of LPC32xx GPIO controller
arm: dts: lpc32xx: extend description of gpio controller node
gpio: lpc32xx: remove legacy LPC32xx GPIO driver
gpio: lpc32xx: add new LPC32xx GPIO controller driver
.../devicetree/bindings/gpio/gpio_lpc32xx.txt | 121 ++- arch/arm/boot/dts/lpc32xx.dtsi | 85 +- drivers/gpio/Kconfig | 8 + drivers/gpio/Makefile | 2 +- drivers/gpio/gpio-lpc32xx.c | 961 +++++++++++---------- 5 files changed, 735 insertions(+), 442 deletions(-)
Signature · 1 line
2.1.4
Vladimir Zapolskiy
maintainerPATCH 1/4 dt-bindings: gpio: update desription of LPC32xx GPIO controller
For the purpose of better description of NXP LPC32xx GPIO controller hardware in device tree format, extend the existing description with device tree subnodes, which represent 6 GPIO banks within the controller.
Note, client interface to the GPIO controller is untouched.
--- .../devicetree/bindings/gpio/gpio_lpc32xx.txt | 121 ++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +120 -1
@@ -15,7 +15,43 @@ Required properties: 2) pin number 3) optional parameters: - bit 0 specifies polarity (0 for normal, 1 for inverted) -- reg: Index of the GPIO group +- #address-cells: should be 2, which stands for GPIO bank id and + physical base address of this GPIO bank. +- #size-cells: should be 1, total size of GPIO bank registers. + +The NXP LPC32xx SoC GPIO controller device node must contain a list +of device nodes representing GPIO banks and their descriptions. + +The format of subnodes should follow the description below. + +Required properties: +- reg: should contain 3 integer values: + 1) GPIO bank id from 0 to 5, + 2) physical base address of this GPIO bank, + 3) total size of the GPIO bank registers. + +Optional properties: +- gpio-bank-name: human readable name of a GPIO bank, +- gpio-no-output-state: property of P2 bank, which has special, + mapping of its control registers, +- gpio-offset: property of P3/GPIO bank, offset of bits representing + GPIO lines in output and direction registers, +- gpios: number of GPIO lines per GPIO bank, if this property is + omitted, then gpio-input-mask must be present, +- gpio-input-mask: should contain two bitmasks, the first bitmask is + the mapping of GPIO lines to input status register, the second + bitmask should be a subset of the first bitmask and it represents + input GPIO lines, which may serve as an interrupt source, + if gpio-input-mask roperty is omitted, gpios property should be + present, +- interrupts: list of parent interrupts mapped to input GPIO lines, +- interrupts-extended: list of parent interrupts mapped to input GPIO + lines, used if parent interrupts are provided by more than one + interrupt controller, this option is used by GPI bank, +- interrupt-controller: indicates that GPIO bank may serve as an + interrupt controller, +- #interrupt-cells: if interrupt-controller property is present, + it should be 2, interrupt id and its flags. Example:
@@ -24,6 +60,89 @@ Example: reg = <0x40028000 0x1000>; gpio-controller; #gpio-cells = <3>; /* bank, pin, flags */ + + ranges = <0 0x0 0x40028000 0x00001000>, + <1 0x0 0x40028000 0x00001000>, + <2 0x0 0x40028000 0x00001000>, + <3 0x0 0x40028000 0x00001000>, + <4 0x0 0x40028000 0x00001000>, + <5 0x0 0x40028000 0x00001000>; + #address-cells = <2>; + #size-cells = <1>; + + gpio_p0: gpio-controller at 0 { + reg = <0 0x40 0x1C>; + gpio-bank-name = "p0"; + gpios = <8>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + }; + + gpio_p1: gpio-controller at 1 { + reg = <1 0x60 0x1C>; + gpio-bank-name = "p1"; + gpios = <24>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + }; + + gpio_p2: gpio-controller at 2 { + reg = <2 0x10 0x18>; + gpio-bank-name = "p2"; + gpios = <13>; + gpio-no-output-state; + }; + + gpio_gpio: gpio-controller at 3 { + reg = <3 0x00 0x1C>; + gpio-bank-name = "gpio"; + gpio-offset = <25>; + gpio-input-mask = <0x01007c00>, <0x01007c00>; + + interrupt-parent = <&sic2>; + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>, + <1 IRQ_TYPE_LEVEL_HIGH>, + <2 IRQ_TYPE_LEVEL_HIGH>, + <3 IRQ_TYPE_LEVEL_HIGH>, + <4 IRQ_TYPE_LEVEL_HIGH>, + <5 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + gpio_gpi: gpio-controller at 4 { + reg = <4 0x00 0x04>; + gpio-bank-name = "gpi"; + gpio-input-only; + gpio-input-mask = <0x1aff83ff>, <0x100803ff>; + + interrupts-extended = + <&sic2 22 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 23 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 24 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 25 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 26 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 27 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 28 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 15 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 9 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 10 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 11 IRQ_TYPE_LEVEL_HIGH>, + <&sic1 4 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + gpio_gpo: gpio-controller at 5 { + reg = <5 0x00 0x10>; + gpio-bank-name = "gpo"; + gpios = <24>; + gpio-output-only; + }; }; leds {
Signature · 1 line
2.1.4
Rob Herring
maintainerPATCH 1/4 dt-bindings: gpio: update desription of LPC32xx GPIO controller
Quoted 17 lines · Vladimir Zapolskiy
For the purpose of better description of NXP LPC32xx GPIO controller hardware in device tree format, extend the existing description with device tree subnodes, which represent 6 GPIO banks within the controller.
Note, client interface to the GPIO controller is untouched.
Signed-off-by Vladimir Zapolskiy <[email protected]>--- .../devicetree/bindings/gpio/gpio_lpc32xx.txt | 121 ++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +0
-0
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 19
Quoted 6 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +2
-1@@ -15,7 +15,43 @@ Required properties: 2) pin number 3) optional parameters: - bit 0 specifies polarity (0 for normal, 1 for inverted) -- reg: Index of the GPIO group +- #address-cells: should be 2, which stands for GPIO bank id and + physical base address of this GPIO bank.
Now you need special code to do address translation. I'd really think twice about doing this.
Why do you need the bank number?
Quoted 9 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +9
-0+- #size-cells: should be 1, total size of GPIO bank registers. + +The NXP LPC32xx SoC GPIO controller device node must contain a list +of device nodes representing GPIO banks and their descriptions. + +The format of subnodes should follow the description below. + +Required properties: +- reg: should contain 3 integer values:
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 38
Quoted 10 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +10
-0+ 1) GPIO bank id from 0 to 5, + 2) physical base address of this GPIO bank, + 3) total size of the GPIO bank registers. + +Optional properties: +- gpio-bank-name: human readable name of a GPIO bank, +- gpio-no-output-state: property of P2 bank, which has special, + mapping of its control registers, +- gpio-offset: property of P3/GPIO bank, offset of bits representing + GPIO lines in output and direction registers,
Seems like nr-gpios should have been a mask instead...
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 40
Quoted 2 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +2
-0+- gpios: number of GPIO lines per GPIO bank, if this property is + omitted, then gpio-input-mask must be present,
"gpios" is already the property name for the client interface.
Quoted 18 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +14
-0+- gpio-input-mask: should contain two bitmasks, the first bitmask is + the mapping of GPIO lines to input status register, the second + bitmask should be a subset of the first bitmask and it represents + input GPIO lines, which may serve as an interrupt source, + if gpio-input-mask roperty is omitted, gpios property should be + present, +- interrupts: list of parent interrupts mapped to input GPIO lines, +- interrupts-extended: list of parent interrupts mapped to input GPIO + lines, used if parent interrupts are provided by more than one + interrupt controller, this option is used by GPI bank, +- interrupt-controller: indicates that GPIO bank may serve as an + interrupt controller, +- #interrupt-cells: if interrupt-controller property is present, + it should be 2, interrupt id and its flags. Example:
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Example: · line 62
Quoted 3 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +0
-0@@ -24,6 +60,89 @@ Example: reg = <0x40028000 0x1000>; gpio-controller; #gpio-cells = <3>; /* bank, pin, flags */
Can't bank and pin be encoded into one cell as the gpio core binding suggests.
Quoted 27 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +27
-0+ + ranges = <0 0x0 0x40028000 0x00001000>, + <1 0x0 0x40028000 0x00001000>, + <2 0x0 0x40028000 0x00001000>, + <3 0x0 0x40028000 0x00001000>, + <4 0x0 0x40028000 0x00001000>, + <5 0x0 0x40028000 0x00001000>; + #address-cells = <2>; + #size-cells = <1>; + + gpio_p0: gpio-controller at 0 { + reg = <0 0x40 0x1C>; + gpio-bank-name = "p0"; + gpios = <8>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + }; + + gpio_p1: gpio-controller at 1 { + reg = <1 0x60 0x1C>; + gpio-bank-name = "p1"; + gpios = <24>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + };
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Example: · line 99
Quoted 10 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +10
-0+ + gpio_p2: gpio-controller at 2 { + reg = <2 0x10 0x18>; + gpio-bank-name = "p2"; + gpios = <13>; + gpio-no-output-state; + }; + + gpio_gpio: gpio-controller at 3 { + reg = <3 0x00 0x1C>;
This overlaps with bank 2.
Quoted 52 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +46
-0+ gpio-bank-name = "gpio"; + gpio-offset = <25>; + gpio-input-mask = <0x01007c00>, <0x01007c00>; + + interrupt-parent = <&sic2>; + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>, + <1 IRQ_TYPE_LEVEL_HIGH>, + <2 IRQ_TYPE_LEVEL_HIGH>, + <3 IRQ_TYPE_LEVEL_HIGH>, + <4 IRQ_TYPE_LEVEL_HIGH>, + <5 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + gpio_gpi: gpio-controller at 4 { + reg = <4 0x00 0x04>; + gpio-bank-name = "gpi"; + gpio-input-only; + gpio-input-mask = <0x1aff83ff>, <0x100803ff>; + + interrupts-extended = + <&sic2 22 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 23 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 24 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 25 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 26 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 27 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 28 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 15 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 9 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 10 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 11 IRQ_TYPE_LEVEL_HIGH>, + <&sic1 4 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + gpio_gpo: gpio-controller at 5 { + reg = <5 0x00 0x10>; + gpio-bank-name = "gpo"; + gpios = <24>; + gpio-output-only; + }; }; leds {Signature · 1 line
2.1.4
Vladimir Zapolskiy
maintainerPATCH 1/4 dt-bindings: gpio: update desription of LPC32xx GPIO controller
Quoted 13 lines · Rob Herring
Quoted 12 lines · Vladimir Zapolskiy
For the purpose of better description of NXP LPC32xx GPIO controller hardware in device tree format, extend the existing description with device tree subnodes, which represent 6 GPIO banks within the controller.
Note, client interface to the GPIO controller is untouched.
Signed-off-by Vladimir Zapolskiy <[email protected]>--- .../devicetree/bindings/gpio/gpio_lpc32xx.txt | 121 ++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-)
Quoted 14 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 19
Quoted 11 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +2
-1@@ -15,7 +15,43 @@ Required properties: 2) pin number 3) optional parameters: - bit 0 specifies polarity (0 for normal, 1 for inverted) -- reg: Index of the GPIO group +- #address-cells: should be 2, which stands for GPIO bank id and + physical base address of this GPIO bank.Now you need special code to do address translation. I'd really think twice about doing this.
Correct, address translation code is needed here...
Quoted 1 line
Why do you need the bank number?
Only one reason -- backward compatibility in sense of referencing a GPIO line on client's side. This API design is broken, I agree.
Honestly I would prefer to get rid of this "feature", new code allows to reference on client's side either a parent GPIO controller device node, or bank nodes, probably the improvement can be done in a few steps?
- this change,
- convert clients to reference a GPIO bank directly,
- remove root GPIO controller (e.g. make it "simple-bus") and convert
GPIO banks to "gpio-controller"s.
Can an evolution like this happen?
Quoted 44 lines
Quoted 9 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +9
-0+- #size-cells: should be 1, total size of GPIO bank registers. + +The NXP LPC32xx SoC GPIO controller device node must contain a list +of device nodes representing GPIO banks and their descriptions. + +The format of subnodes should follow the description below. + +Required properties: +- reg: should contain 3 integer values:Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 38
Quoted 10 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +10
-0+ 1) GPIO bank id from 0 to 5, + 2) physical base address of this GPIO bank, + 3) total size of the GPIO bank registers. + +Optional properties: +- gpio-bank-name: human readable name of a GPIO bank, +- gpio-no-output-state: property of P2 bank, which has special, + mapping of its control registers, +- gpio-offset: property of P3/GPIO bank, offset of bits representing + GPIO lines in output and direction registers,Seems like nr-gpios should have been a mask instead...
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 40
Quoted 2 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +2
-0+- gpios: number of GPIO lines per GPIO bank, if this property is + omitted, then gpio-input-mask must be present,"gpios" is already the property name for the client interface.
Quoted 17 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +14
-0+- gpio-input-mask: should contain two bitmasks, the first bitmask is + the mapping of GPIO lines to input status register, the second + bitmask should be a subset of the first bitmask and it represents + input GPIO lines, which may serve as an interrupt source, + if gpio-input-mask roperty is omitted, gpios property should be + present, +- interrupts: list of parent interrupts mapped to input GPIO lines, +- interrupts-extended: list of parent interrupts mapped to input GPIO + lines, used if parent interrupts are provided by more than one + interrupt controller, this option is used by GPI bank, +- interrupt-controller: indicates that GPIO bank may serve as an + interrupt controller, +- #interrupt-cells: if interrupt-controller property is present, + it should be 2, interrupt id and its flags. Example:
Quoted 7 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Example: · line 62
Quoted 4 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +0
-0@@ -24,6 +60,89 @@ Example: reg = <0x40028000 0x1000>; gpio-controller; #gpio-cells = <3>; /* bank, pin, flags */Can't bank and pin be encoded into one cell as the gpio core binding suggests.
Please see the comment above, the proposed change does not modify this legacy part.
Quoted 31 lines
Quoted 31 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +31
-0+ + ranges = <0 0x0 0x40028000 0x00001000>, + <1 0x0 0x40028000 0x00001000>, + <2 0x0 0x40028000 0x00001000>, + <3 0x0 0x40028000 0x00001000>, + <4 0x0 0x40028000 0x00001000>, + <5 0x0 0x40028000 0x00001000>; + #address-cells = <2>; + #size-cells = <1>; + + gpio_p0: gpio-controller at 0 { + reg = <0 0x40 0x1C>; + gpio-bank-name = "p0"; + gpios = <8>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + }; + + gpio_p1: gpio-controller at 1 { + reg = <1 0x60 0x1C>; + gpio-bank-name = "p1"; + gpios = <24>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + }; + + gpio_p2: gpio-controller at 2 { + reg = <2 0x10 0x18>; + gpio-bank-name = "p2";
Quoted 8 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Example: · line 99
Quoted 6 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +6
-0+ gpios = <13>; + gpio-no-output-state; + }; + + gpio_gpio: gpio-controller at 3 { + reg = <3 0x00 0x1C>;This overlaps with bank 2.
Yes, it is. Thousand thanks to hardware designers.
--
With best wishes,
Vladimir
Rob Herring
maintainerPATCH 1/4 dt-bindings: gpio: update desription of LPC32xx GPIO controller
Quoted 29 lines · Vladimir Zapolskiy
Quoted 13 lines · Rob Herring
Quoted 12 lines · Vladimir Zapolskiy
For the purpose of better description of NXP LPC32xx GPIO controller hardware in device tree format, extend the existing description with device tree subnodes, which represent 6 GPIO banks within the controller.
Note, client interface to the GPIO controller is untouched.
Signed-off-by Vladimir Zapolskiy <[email protected]>--- .../devicetree/bindings/gpio/gpio_lpc32xx.txt | 121 ++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-)Quoted 14 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 19
Quoted 11 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +2
-1@@ -15,7 +15,43 @@ Required properties: 2) pin number 3) optional parameters: - bit 0 specifies polarity (0 for normal, 1 for inverted) -- reg: Index of the GPIO group +- #address-cells: should be 2, which stands for GPIO bank id and + physical base address of this GPIO bank.Now you need special code to do address translation. I'd really think twice about doing this.
Quoted 6 lines
Correct, address translation code is needed here...
Quoted 1 line
Why do you need the bank number?
Only one reason -- backward compatibility in sense of referencing a GPIO line on client's side. This API design is broken, I agree.
What client exactly? Between the dtb and kernel or kernel and userspace or ...?
Quoted 10 lines
Honestly I would prefer to get rid of this "feature", new code allows to reference on client's side either a parent GPIO controller device node, or bank nodes, probably the improvement can be done in a few steps?
- this change,
- convert clients to reference a GPIO bank directly,
- remove root GPIO controller (e.g. make it "simple-bus") and convert
GPIO banks to "gpio-controller"s.Can an evolution like this happen?
You generally don't want bindings to evolve.
Quoted 12 lines
Quoted 11 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Example: · line 99
Quoted 9 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +9
-0+ gpio_p2: gpio-controller at 2 { + reg = <2 0x10 0x18>; + gpio-bank-name = "p2"; + gpios = <13>; + gpio-no-output-state; + }; + + gpio_gpio: gpio-controller at 3 { + reg = <3 0x00 0x1C>;This overlaps with bank 2.
Quoted 1 line
Yes, it is. Thousand thanks to hardware designers.
Then you might want to split these into 2 regions. The problem is request_resource does not work with overlapping resources. Or just don't do subnodes. If there is not a lot of variations in the subnode data, then just leave that information in the kernel.
Rob
PATCH 1/4 dt-bindings: gpio: update desription of LPC32xx GPIO controller
In reply to Vladimir ZapolskiyQuoted 8 lines · Vladimir Zapolskiy
For the purpose of better description of NXP LPC32xx GPIO controller hardware in device tree format, extend the existing description with device tree subnodes, which represent 6 GPIO banks within the controller.
Note, client interface to the GPIO controller is untouched.
Signed-off-by Vladimir Zapolskiy <[email protected]>
(...)
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 31
Quoted 6 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +6
-0+ +Required properties: +- reg: should contain 3 integer values: + 1) GPIO bank id from 0 to 5, + 2) physical base address of this GPIO bank, + 3) total size of the GPIO bank registers.
If each bank has a unique physical address, it should be a unique device with a compatible string.
Below: rethink this. All are named gpio-* which is the generic GPIO namespace and should be documented in Documentation/devicetree/bindings/gpio/gpio.txt
so figure out if what you're adding is generic or not.
Quoted 2 lines
+Optional properties:
+- gpio-bank-name: human readable name of a GPIO bank,
The node name is unique enough and is what we use in device trees. Get rid of this.
Quoted 2 lines
+- gpio-no-output-state: property of P2 bank, which has special, + mapping of its control registers,
Looks like it should be nxp,no-output-state
Quoted 2 lines
+- gpio-offset: property of P3/GPIO bank, offset of bits representing + GPIO lines in output and direction registers,
Explain more. I think this should probably be generic and described together with ngpios.
Quoted 2 lines
+- gpios: number of GPIO lines per GPIO bank, if this property is + omitted, then gpio-input-mask must be present,
Use the generic ngpios, also one does not exclude the other, e.g if there is 32 gpios, offset it 8 ngpios is 8, we know that bits 8..15 are GPIO lines.
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 46
Quoted 6 lines
+- gpio-input-mask: should contain two bitmasks, the first bitmask is
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +5
-0+ the mapping of GPIO lines to input status register, the second + bitmask should be a subset of the first bitmask and it represents + input GPIO lines, which may serve as an interrupt source, + if gpio-input-mask roperty is omitted, gpios property should be + present,
This is really hopeless to understand. This document needs a detailed description about how this GPIO block works so we have the background for this.
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 54
Quoted 8 lines
+- interrupts: list of parent interrupts mapped to input GPIO lines, +- interrupts-extended: list of parent interrupts mapped to input GPIODocumentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +6
-0+ lines, used if parent interrupts are provided by more than one + interrupt controller, this option is used by GPI bank, +- interrupt-controller: indicates that GPIO bank may serve as an + interrupt controller, +- #interrupt-cells: if interrupt-controller property is present, + it should be 2, interrupt id and its flags.
You need to add a description of how the block maps IRQs to GPIO lines. It seems like it is doing some kind of phone-exchange type of thing and just latches the GPIO line out to an IRQ line on the interrupt controller, and that is why even setting up trigger type has to percolate up to the parent? Explain this in this document.
Quoted 1 line
+ gpio-output-only;
You forgot to documen this property.
Yours,
Linus Walleij
PATCH 1/4 dt-bindings: gpio: update desription of LPC32xx GPIO controller
Hi Linus,
Quoted 13 lines · Linus Walleij
Quoted 8 lines · Vladimir Zapolskiy
For the purpose of better description of NXP LPC32xx GPIO controller hardware in device tree format, extend the existing description with device tree subnodes, which represent 6 GPIO banks within the controller.
Note, client interface to the GPIO controller is untouched.
Signed-off-by Vladimir Zapolskiy <[email protected]>(...)
Quoted 9 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 31
Quoted 6 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +6
-0+ +Required properties: +- reg: should contain 3 integer values: + 1) GPIO bank id from 0 to 5, + 2) physical base address of this GPIO bank, + 3) total size of the GPIO bank registers.If each bank has a unique physical address, it should be a unique device with a compatible string.
thank you for review, this is the most broken GPIO controller I've ever seen, so many common generalized concepts present in the kernel are not applicable, unfortunately.
Here is just a short description of "the best hw design practices" from the SoC documentation:
* 6 GPIO banks (similar properties of GPIO lines within a bank): P0 (8 lines), P1 (24 lines), P2 (13 lines), GPIO (6 lines), GPO (24 lines, output only), GPI (22 lines, input only),
- gpio-output-only property is for GPO,
- gpio-input-only property is for GPI,
* but 4 mixed register spaces: P0 (holds controls of P0), P1 (controls of P1), P2 (controls of P2 and GPIO), P3 (controls of GPIO, GPO and GPI),
Here is the first answer to your questions, not each bank has a unique physical address, GPIO bank controls are in two register spaces and one register space contains controls of 3 banks.
So there must be another way to address banks, I reused an existing one from the legacy code, banks are enumerated from 0 to 5.
Also this "bank address" has been already used by the existing GPIO consumers, I'd be glad to have one device per bank (and that's actually in my plans for future), but this requires an update of all GPIO clients in DTS files, I deliberately separate this task from the GPIO controller driver update task.
* only P2 bank has controls to set output value, but has no register to read out the set output value,
- gpio-no-output-state property is for P2,
* due to the fact that P2 register space has GPIO bank controls, one more property is introduced:
- gpio-offset property for GPIO,
* GPIO names in GPI bank are discrete (that caused a problem you've recently applied a fix for), all other banks has continuous GPIO names,
* GPIO - 6 lines, each is capable to produce an interrupt, interrupt on GPIO line can be IRQ_TYPE_EDGE_BOTH if the driver reads a line state out, but IRQ chip itself does not support edge-both interrupts, this bank potentially can be converted to use GPIOLIB_IRQCHIP, but please see the next point,
* GPI - 22 lines, but only 12 can produce an interrupt, so GPIOLIB_IRQCHIP helper can not be used here, to keep the code smaller (one less exception for the banks) I've shared the same mechanism of assigning GPIO lines to IRQs from GPI bank to GPIO bank.
- gpio-input-mask property of GPIO and GPI banks, mapping between lines and
interrupts
Quoted 4 lines
Below: rethink this. All are named gpio-* which is the generic GPIO namespace and should be documented in
Documentation/devicetree/bindings/gpio/gpio.txt
so figure out if what you're adding is generic or not.
You may see that adding of 5 bank specific properties mentioned above allow to generalize all 6 banks, any of the banks now can be converted to a separate GPIO controller device with the same compatible in one step, but this is postponed at the moment.
Quoted 5 lines
Quoted 2 lines
+Optional properties:
+- gpio-bank-name: human readable name of a GPIO bank,The node name is unique enough and is what we use in device trees. Get rid of this.
The node name in most cases and in the example below is "gpio-controller", at least in this particular case I find it insufficiently informative, P0 bank is sensibly different from P2, as well as it is different from GPI, GPIO or GPO. A good mechanism to understand in userspace what particular bank is involved is wanted here, probably I can add a "label" property? Or should I replace gpio-controller at xxx device node names with p0 at xxx etc.?
Quoted 4 lines
Quoted 2 lines
+- gpio-no-output-state: property of P2 bank, which has special, + mapping of its control registers,Looks like it should be nxp,no-output-state
Agree.
Quoted 5 lines
Quoted 2 lines
+- gpio-offset: property of P3/GPIO bank, offset of bits representing + GPIO lines in output and direction registers,Explain more. I think this should probably be generic and described together with ngpios.
The necessity comes from the fact that there is an intersection of register spaces for banks P2 and GPIO, when it concerns GPIO, DIR_CLR/DIR_STATE registers has a bit offset to control GPIO bank lines.
Quoted 6 lines
Quoted 2 lines
+- gpios: number of GPIO lines per GPIO bank, if this property is + omitted, then gpio-input-mask must be present,Use the generic ngpios, also one does not exclude the other, e.g if there is 32 gpios, offset it 8 ngpios is 8, we know that bits 8..15 are GPIO lines.
Ok, will use ngpios. Offset feature won't help, because there is no uniform offset (except 0) for any of the banks...
Quoted 10 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 46
Quoted 6 lines
+- gpio-input-mask: should contain two bitmasks, the first bitmask is
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +5
-0+ the mapping of GPIO lines to input status register, the second + bitmask should be a subset of the first bitmask and it represents + input GPIO lines, which may serve as an interrupt source, + if gpio-input-mask roperty is omitted, gpios property should be + present,This is really hopeless to understand. This document needs a detailed description about how this GPIO block works so we have the background for this.
I'll add more info, shortly if you once find LPC32xx User's Manual (it is public), this property contains two values, the first one repeats mapping of P3_INP_STATE bits to bank specific lines, the second one (subset of the first) lists input lines, which can produce an interrupt.
Quoted 9 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 54
Quoted 8 lines
+- interrupts: list of parent interrupts mapped to input GPIO lines, +- interrupts-extended: list of parent interrupts mapped to input GPIODocumentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +6
-0+ lines, used if parent interrupts are provided by more than one + interrupt controller, this option is used by GPI bank, +- interrupt-controller: indicates that GPIO bank may serve as an + interrupt controller, +- #interrupt-cells: if interrupt-controller property is present, + it should be 2, interrupt id and its flags.
Quoted 6 lines
You need to add a description of how the block maps IRQs to GPIO lines. It seems like it is doing some kind of phone-exchange type of thing and just latches the GPIO line out to an IRQ line on the interrupt controller, and that is why even setting up trigger type has to percolate up to the parent? Explain this in this document.
Will extend this description. Generally your understanding is correct, a requested GPIO interrupt is propagated to an IRQ chip interrupt, the handled IRQ chip interrupt is cascaded to the GPIO interrupt, but some specific handling of both edges type of interrupt is done on GPIO interrupt controller side, because IRQ chip interrupt can not support edge-both type of interrupts.
Quoted 3 lines
Quoted 1 line
+ gpio-output-only;
You forgot to documen this property.
Ok.
Thanks for review. If you find that conceptually any other scheme of device tree node properties or bank layout is more applicable in this particular case, please let me know.
--
With best wishes,
Vladimir
PATCH 1/4 dt-bindings: gpio: update desription of LPC32xx GPIO controller
Quoted 10 lines · Vladimir Zapolskiy
[Me]
Quoted 2 lines
The node name is unique enough and is what we use in device trees. Get rid of this.
The node name in most cases and in the example below is "gpio-controller", at least in this particular case I find it insufficiently informative, P0 bank is sensibly different from P2, as well as it is different from GPI, GPIO or GPO. A good mechanism to understand in userspace what particular bank is involved is wanted here, probably I can add a "label" property? Or should I replace gpio-controller at xxx device node names with p0 at xxx etc.?
Userspace is supposed to use the whole filepath in sysfs to identify a device. That should be enough, no matter what name the chip has.
Also: see the ongoing discussion and proposed patch for a new userspace ABI based on a character device. We're not too happy about new userspace usecases for GPIO right now, we need to fix the ABI.
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
+- gpio-offset: property of P3/GPIO bank, offset of bits representing + GPIO lines in output and direction registers,Explain more. I think this should probably be generic and described together with ngpios.
The necessity comes from the fact that there is an intersection of register spaces for banks P2 and GPIO, when it concerns GPIO, DIR_CLR/DIR_STATE registers has a bit offset to control GPIO bank lines.
So do we think that other hardware will have this property too or is it an nxp,* thing?
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
+- gpios: number of GPIO lines per GPIO bank, if this property is + omitted, then gpio-input-mask must be present,Use the generic ngpios, also one does not exclude the other, e.g if there is 32 gpios, offset it 8 ngpios is 8, we know that bits 8..15 are GPIO lines.
Ok, will use ngpios. Offset feature won't help, because there is no uniform offset (except 0) for any of the banks...
OK let's look closer at it.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
+- gpio-input-mask: should contain two bitmasks, the first bitmask is
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +5
-0+ the mapping of GPIO lines to input status register, the second + bitmask should be a subset of the first bitmask and it represents + input GPIO lines, which may serve as an interrupt source, + if gpio-input-mask roperty is omitted, gpios property should be + present,
Quoted 8 lines
Quoted 3 lines
This is really hopeless to understand. This document needs a detailed description about how this GPIO block works so we have the background for this.
I'll add more info, shortly if you once find LPC32xx User's Manual (it is public), this property contains two values, the first one repeats mapping of
P3_INP_STATEbits to bank specific lines, the second one (subset of the first) lists input lines, which can produce an interrupt.
OK, and should it be an nxp,* property?
Quoted 16 lines
Quoted 9 lines
Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt Required properties: · line 54
Quoted 8 lines
+- interrupts: list of parent interrupts mapped to input GPIO lines, +- interrupts-extended: list of parent interrupts mapped to input GPIODocumentation/devicetree/bindings/gpio/gpio_lpc32xx.txt +6
-0+ lines, used if parent interrupts are provided by more than one + interrupt controller, this option is used by GPI bank, +- interrupt-controller: indicates that GPIO bank may serve as an + interrupt controller, +- #interrupt-cells: if interrupt-controller property is present, + it should be 2, interrupt id and its flags.Quoted 6 lines
You need to add a description of how the block maps IRQs to GPIO lines. It seems like it is doing some kind of phone-exchange type of thing and just latches the GPIO line out to an IRQ line on the interrupt controller, and that is why even setting up trigger type has to percolate up to the parent? Explain this in this document.
Quoted 6 lines
Will extend this description. Generally your understanding is correct, a requested GPIO interrupt is propagated to an IRQ chip interrupt, the handled IRQ chip interrupt is cascaded to the GPIO interrupt, but some specific handling of both edges type of interrupt is done on GPIO interrupt controller side, because IRQ chip interrupt can not support edge-both type of interrupts.
OMG that sounds so weird.
Yours,
Linus Walleij
Vladimir Zapolskiy
maintainerPATCH 2/4 arm: dts: lpc32xx: extend description of gpio controller node
In reply to Vladimir ZapolskiyThe change adds detailed description of NXP LPC32xx GPIO controller node.
Note, in future possibly P0 and P1 banks can be converted to interrupt controllers, but this requires pinmux configuration, which is missing at the moment.
The change does not affect any GPIO controller clients.
--- arch/arm/boot/dts/lpc32xx.dtsi | 85 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-)
arch/arm/boot/dts/lpc32xx.dtsi +84 -1
@@ -398,11 +398,94 @@ clocks = <&clk LPC32XX_CLK_RTC>; }; - gpio: gpio at 40028000 { + gpio: gpio-controller at 40028000 { compatible = "nxp,lpc3220-gpio"; reg = <0x40028000 0x1000>; gpio-controller; #gpio-cells = <3>; /* bank, pin, flags */ + + ranges = <0 0x0 0x40028000 0x00001000>, + <1 0x0 0x40028000 0x00001000>, + <2 0x0 0x40028000 0x00001000>, + <3 0x0 0x40028000 0x00001000>, + <4 0x0 0x40028000 0x00001000>, + <5 0x0 0x40028000 0x00001000>; + #address-cells = <2>; + #size-cells = <1>; + + gpio_p0: gpio-controller at 0 { + reg = <0 0x40 0x1C>; + gpio-bank-name = "p0"; + gpios = <8>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + }; + + gpio_p1: gpio-controller at 1 { + reg = <1 0x60 0x1C>; + gpio-bank-name = "p1"; + gpios = <24>; + + interrupt-parent = <&sic2>; + interrupts = <8 IRQ_TYPE_LEVEL_HIGH>; + }; + + gpio_p2: gpio-controller at 2 { + reg = <2 0x10 0x18>; + gpio-bank-name = "p2"; + gpios = <13>; + gpio-no-output-state; + }; + + gpio_gpio: gpio-controller at 3 { + reg = <3 0x00 0x1C>; + gpio-bank-name = "gpio"; + gpio-offset = <25>; + gpio-input-mask = <0x01007c00>, <0x01007c00>; + + interrupt-parent = <&sic2>; + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>, + <1 IRQ_TYPE_LEVEL_HIGH>, + <2 IRQ_TYPE_LEVEL_HIGH>, + <3 IRQ_TYPE_LEVEL_HIGH>, + <4 IRQ_TYPE_LEVEL_HIGH>, + <5 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + gpio_gpi: gpio-controller at 4 { + reg = <4 0x00 0x04>; + gpio-bank-name = "gpi"; + gpio-input-only; + gpio-input-mask = <0x1aff83ff>, <0x100803ff>; + + interrupts-extended = + <&sic2 22 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 23 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 24 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 25 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 26 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 27 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 28 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 15 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 9 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 10 IRQ_TYPE_LEVEL_HIGH>, + <&sic2 11 IRQ_TYPE_LEVEL_HIGH>, + <&sic1 4 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + gpio_gpo: gpio-controller at 5 { + reg = <5 0x00 0x10>; + gpio-bank-name = "gpo"; + gpios = <24>; + gpio-output-only; + }; }; timer4: timer at 4002C000 {
Signature · 1 line
2.1.4
Vladimir Zapolskiy
maintainerPATCH 3/4 gpio: lpc32xx: remove legacy LPC32xx GPIO driver
In reply to Vladimir ZapolskiyLegacy GPIO controller driver for NXP LPC32xx platforms depends on hardcoded hardware interrupt values, which makes it unmaintainable with newer interrupt controller features like custom platform irq handler, dynamic allocation of virtual irqs and sparse irqs.
The legacy LPX32xx interrupt controller and as a result this legacy GPIO controller driver are broken since commit 76ba59f8366f2 ("genirq:
Add irq_domain-aware core IRQ handler"), the driver will be replaced by a new version compatible with current GPIO client device tree interface.
--- drivers/gpio/Makefile | 1 - drivers/gpio/gpio-lpc32xx.c | 577 -------------------------------------------- 2 files changed, 578 deletions(-) delete mode 100644 drivers/gpio/gpio-lpc32xx.c
drivers/gpio/Makefile +0 -1
@@ -50,7 +50,6 @@ obj-$(CONFIG_GPIO_INTEL_MID) += gpio-intel-mid.o obj-$(CONFIG_GPIO_LOONGSON) += gpio-loongson.o obj-$(CONFIG_GPIO_LP3943) += gpio-lp3943.o obj-$(CONFIG_GPIO_LPC18XX) += gpio-lpc18xx.o -obj-$(CONFIG_ARCH_LPC32XX) += gpio-lpc32xx.o obj-$(CONFIG_GPIO_LYNXPOINT) += gpio-lynxpoint.o obj-$(CONFIG_GPIO_MAX730X) += gpio-max730x.o obj-$(CONFIG_GPIO_MAX7300) += gpio-max7300.o
drivers/gpio/gpio-lpc32xx.c deleted +0 -577
577 lines not on this page · raw message
Signature · 1 line
2.1.4
Vladimir Zapolskiy
maintainerPATCH 4/4 gpio: lpc32xx: add new LPC32xx GPIO controller driver
In reply to Vladimir ZapolskiyThe change adds a replacement to a legacy unmaintainable LPC32xx GPIO controller driver.
The driver gets all information to configure a GPIO bank from its device tree node, the driver is capable to assign interrupt controller roles to its GPIO banks and it has no hardcoded dependencies on hardware interrupt numbers, control register addresses, mapping of GPIO lines to interrupts or bitfields.
The new driver is compatible with device tree clients of the legacy driver, no changes on GPIO client side are needed.
--- drivers/gpio/Kconfig | 8 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-lpc32xx.c | 660 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 669 insertions(+) create mode 100644 drivers/gpio/gpio-lpc32xx.c
drivers/gpio/Kconfig +8 -0
@@ -246,6 +246,14 @@ config GPIO_LPC18XX Select this option to enable GPIO driver for NXP LPC18XX/43XX devices. +config GPIO_LPC32XX + bool "NXP LPC32 GPIO support" + default y if ARCH_LPC32XX + depends on OF_GPIO && (ARCH_LPC32XX || COMPILE_TEST) + help + Select this option to enable GPIO driver for + NXP LPC32XX devices. + config GPIO_LYNXPOINT tristate "Intel Lynxpoint GPIO support" depends on ACPI && X86
drivers/gpio/Makefile +1 -0
@@ -50,6 +50,7 @@ obj-$(CONFIG_GPIO_INTEL_MID) += gpio-intel-mid.o obj-$(CONFIG_GPIO_LOONGSON) += gpio-loongson.o obj-$(CONFIG_GPIO_LP3943) += gpio-lp3943.o obj-$(CONFIG_GPIO_LPC18XX) += gpio-lpc18xx.o +obj-$(CONFIG_GPIO_LPC32XX) += gpio-lpc32xx.o obj-$(CONFIG_GPIO_LYNXPOINT) += gpio-lynxpoint.o obj-$(CONFIG_GPIO_MAX730X) += gpio-max730x.o obj-$(CONFIG_GPIO_MAX7300) += gpio-max7300.o
drivers/gpio/gpio-lpc32xx.c new file +660 -0
660 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH 4/4 gpio: lpc32xx: add new LPC32xx GPIO controller driver
Quoted 9 lines · Vladimir Zapolskiy
The change adds a replacement to a legacy unmaintainable LPC32xx GPIO controller driver.
The driver gets all information to configure a GPIO bank from its device tree node, the driver is capable to assign interrupt controller roles to its GPIO banks and it has no hardcoded dependencies on hardware interrupt numbers, control register addresses, mapping of GPIO lines to interrupts or bitfields.
Quoted 4 lines
The new driver is compatible with device tree clients of the legacy driver, no changes on GPIO client side are needed.
Signed-off-by Vladimir Zapolskiy <[email protected]>
(...)
Overall question: why are you not creating one device and thus one gpio_chip per bank?
That would make this driver a lot simpler I think. Please motivate if you want to keep it like this.
Doing that would probably also enable you to useGPIOLIB_IRQCHIP which would be a big win.
Quoted 1 line
+#define
pr_fmt(fmt)"%s: " fmt,__func__
Usually a driver shall use dev_*(dev,) and not need quirks like this to modify prints. Why is this needed?
drivers/gpio/gpio-lpc32xx.c line 21
Quoted 6 lines
drivers/gpio/gpio-lpc32xx.c new file +6
-0+#include <linux/interrupt.h> +#include <linux/of_address.h> +#include <linux/of_gpio.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> +#include <linux/slab.h>
This file will need
#include <linux/bitops.h>
You should also include
#include <linux/gpio/driver.h>
Even if this gets in implicitly.
Quoted 1 line
+#define
to_lpc32xx_gpio_chip(c)container_of(chip, struct lpc32xx_gpio_chip, c)
Rewrite this into a static inline function. Defines are ugly.
drivers/gpio/gpio-lpc32xx.c line 45
Quoted 8 lines
drivers/gpio/gpio-lpc32xx.c new file +8
-0+struct lpc32xx_gpio_chip ; + +struct lpc32xx_irq_data { +struct lpc32xx_gpio_chip *bank; +u32 idx; +u32 parent; +u32 gpio; +};
This needs some kerneldoc, and I think it looks very weird, like you are reimplementing irqdomain or something.
Some circular dependency of one device pointing
back to the other, why? Is this necessary?
Quoted 7 lines
drivers/gpio/gpio-lpc32xx.c new file +7
-0+struct lpc32xx_gpio_chip { +struct gpio_chip chip; + +struct device *dev; +void __iomem *base; +void __iomem *input; +void __iomem *output;
drivers/gpio/gpio-lpc32xx.c line 63
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+void __iomem *dir; + +bool input_only; +bool output_only; +u32 *output_states; +u32 offset; +u32 input_mask; +u32 interrupt_mask; + +const char **gpio_names;
The gpio_chip has a field named .names for this,
why are you reimplementing this in your driver?
drivers/gpio/gpio-lpc32xx.c line 68
Quoted 4 lines
drivers/gpio/gpio-lpc32xx.c new file +4
-0+u32 nirq; +struct resource *irq_res; +struct lpc32xx_irq_data *irqs; +};
Kerneldoc this struct so I understand it, too.
Review
Quoted 2 lines
(quoted diff) +2
-0+ +static struct lpc32xx_gpio_chip *lpc32xx_gpio_chips[LPC32XX_GPIO_BANKS];
I don't see why you are doing a local array of these but I guess I will find out...
drivers/gpio/gpio-lpc32xx.c line 82
Quoted 11 lines
drivers/gpio/gpio-lpc32xx.c new file +11
-0+/* Count set bits in mask, i.e. get max relative bit position */ +static int bit_count (u32 mask) +{ +int count =0 ; + +for (; mask; mask >>=1 ) +if (mask &0x1 ) + count++; + +return count; +}
Nope. This is already in the kernel, sometimes assembly-optimized for certain architectures.
Just use
#include <linux/bitops.h>
hweight32(mask);
Quoted 3 lines
+/*
+ * If mask has less or equal than "rel" set bits, i.e.
+ * relative bit is set, find its abosulute bit position
Spelling.
Quoted 37 lines
+ */
drivers/gpio/gpio-lpc32xx.c new file +36
-0+static int bit_abs (u32 rel,u32 mask) +{ +int idx, val =0 ; + +for (idx =0 ; idx <= rel; val++, mask >>=1 ) { +if (mask &0x1 ) + idx++; +if (!mask) +return -EINVAL ; + } + val--; + +return val; +} + +/* +* If mask has set bit on absolute position, +* find relative bit number in set bits +*/ +static int bit_rel (u32 abs,u32 mask) +{ +int val =0 ; + +if (!(BIT (abs) & mask)) +return -EINVAL ; + +for (; abs; abs--, mask >>=1 ) { +if (mask &0x1 ) + val++; + } +return val; +} + +/* +* Composition of bit_abs() and bit_rel(), +* mapping of one relative bit position into another
drivers/gpio/gpio-lpc32xx.c line 133
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+*/ +static int bit_map (u32 rel,u32 from,u32 to) +{ +int ret; + + ret =bit_abs (rel, from); +if (ret <0 ) +return ret; +return bit_rel (ret, to); +}
I suspect these are reiplementations of generic concepts as well. Please study <linux/bitops.h> and <asm/bitops.h>
We have all kinds of operations for finding first set bit, last set bit, hamming weight, modifying bitmasks etc etc.
Quoted 1 line
+static int
lpc32xx_gpio_get(struct gpio_chip *chip, unsigned gpio)
Rename parameter "gpio" to "offset", as this is not a global number but chip-local.
drivers/gpio/gpio-lpc32xx.c line 164
Quoted 12 lines
drivers/gpio/gpio-lpc32xx.c new file +12
-0+{ +struct lpc32xx_gpio_chip *c =to_lpc32xx_gpio_chip (chip); +u32 val, direction =lpc32xx_gpio_dir_get (chip, gpio); +int ret; + +if (direction ==GPIOF_DIR_OUT ) { +/* Return cached value in case of P2 bank */ +if (c->output_states) +return c->output_states[gpio]; + + val =readl (c->output +LPC32XX_GPIO_STATE ); + val >>= gpio + c->offset;
Do it like this:
return !!(readl(c->output + LPC32XX_GPIO_STATE) & BIT(c->offset + offset));
Quoted 1 line
+ } else {
Just skip the else clause and un-indent. If we're here, it is input.
drivers/gpio/gpio-lpc32xx.c line 175
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+if (c->input_mask) { +/* Special case of GPI and GPIO decomposition */ + ret =bit_abs (gpio, c->input_mask); +if (ret <0 ) +return ret; + }else { + ret = gpio; + } + + val =readl (c->input) >> ret;
Just
return !!(readl(c->input) & BIT(offset));
Quoted 4 lines
drivers/gpio/gpio-lpc32xx.c new file +4
-0+ } + +return val &0x1 ; +}
(...)
Quoted 1 line
+static int
lpc32xx_gpio_dir_input(struct gpio_chip *chip, unsigned gpio)
Rename "gpio" to "offset"
drivers/gpio/gpio-lpc32xx.c line 198
Quoted 7 lines
drivers/gpio/gpio-lpc32xx.c new file +7
-0+{ +struct lpc32xx_gpio_chip *c =to_lpc32xx_gpio_chip (chip); + +if (c->output_only) +return -EINVAL ; + +writel (BIT (gpio + c->offset), c->dir +LPC32XX_GPIO_CLEAR );
This looks right.
drivers/gpio/gpio-lpc32xx.c line 204
Quoted 2 lines
drivers/gpio/gpio-lpc32xx.c new file +2
-0+static int lpc32xx_gpio_dir_output (struct gpio_chip *chip, +unsigned gpio,int value)
Rename "gpio" to "offset".
Quoted 8 lines
drivers/gpio/gpio-lpc32xx.c new file +8
-0+static int lpc32xx_gpio_to_irq (struct gpio_chip *chip,unsigned gpio) +{ +struct lpc32xx_gpio_chip *bank =to_lpc32xx_gpio_chip (chip); +int ret; + +if (!bank->chip.irqchip) { +dev_dbg (bank->dev,"%s: no interrupt controller\n" , + bank->gpio_names[gpio]);
drivers/gpio/gpio-lpc32xx.c line 234
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+return -EINVAL ; + } + +if (bank->input_mask) { + ret =bit_map (gpio, bank->input_mask, bank->interrupt_mask); +if (ret >=0 ) + ret =irq_find_mapping (bank->chip.irqdomain, ret); + } + +return ret;
What happens if bank.irqchip is exis and bank->input_mask is blank?
You return an unitialized ret variable.
Again: why is it not possible to use the GPIOLIB_IRQCHIP and get rid of all the custom IRQ handling in the driver? If you do one device per bank I think this would be possible, and maybe even if you keep this composite driver, because gpiochip_irqchip_add() can be called several times if referring to the same gpio_chip.
Quoted 25 lines
drivers/gpio/gpio-lpc32xx.c new file +25
-0+static irqreturn_t lpc32xx_gpio_irq_handler (int irq,void *irqdata) +{ +struct lpc32xx_irq_data *data = irqdata; +struct lpc32xx_gpio_chip *c = data->bank; +int virq =irq_find_mapping (c->chip.irqdomain, data->idx); +u32 val; + +if (irq_get_trigger_type (virq) ==IRQ_TYPE_EDGE_BOTH ) { + val =lpc32xx_gpio_get (&c->chip, c->irqs[data->idx].gpio); +if (val) +irq_set_irq_type (irq,IRQ_TYPE_EDGE_FALLING ); +else +irq_set_irq_type (irq,IRQ_TYPE_EDGE_RISING ); + } + +generic_handle_irq (virq); + +return IRQ_HANDLED ; +} + +static int lpc32xx_gpio_irq_set_type (struct irq_data *d,unsigned type) +{ +struct irq_domain *id = d->domain; +struct lpc32xx_gpio_chip *c = (struct lpc32xx_gpio_chip *)id->host_data; +u32 val;
drivers/gpio/gpio-lpc32xx.c line 271
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+ +irqd_set_trigger_type (d, type); + +if (type ==IRQ_TYPE_EDGE_BOTH ) { + val =lpc32xx_gpio_get (&c->chip, c->irqs[d->hwirq].gpio); +if (val) + type =IRQ_TYPE_EDGE_FALLING ; +else + type =IRQ_TYPE_EDGE_RISING ; + }
Use a switch() statement here. What if somebody tries to set a LEVEL IRQ? You need a default: clause.
Quoted 1 line
+
irq_set_irq_type(c->irqs[d->hwirq].parent, type);
This looks weird. Why is the IRQ output line from the gpiochip changing type just because it is starting to detect IRQs on its GPIO lines in a different way?
Just because the gpiochip starts to do edges, it doesn't mean it starts outputting edges all of a sudden, if i was level IRQs before (the most common).
Please explain this or put in a big comment about how these IRQs work on the platform.
drivers/gpio/gpio-lpc32xx.c line 287
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+static unsigned int lpc32xx_gpio_irq_startup (struct irq_data *d) +{ +struct irq_domain *id = d->domain; +struct lpc32xx_gpio_chip *c = (struct lpc32xx_gpio_chip *)id->host_data; +int ret; + + ret =request_irq (c->irqs[d->hwirq].parent, + lpc32xx_gpio_irq_handler, +IRQF_TRIGGER_NONE ,dev_name (c->dev), + &c->irqs[d->hwirq]);
And here the parent IRQ is requested with NONE trigger. Why? Isn't the gpio chip coupled to that line with some buffer logic that clearly states which IRQ type should be used to interface the parent?
drivers/gpio/gpio-lpc32xx.c line 330
Quoted 7 lines
drivers/gpio/gpio-lpc32xx.c new file +7
-0+static int lpc32xx_of_xlate (struct gpio_chip *gc, +const struct of_phandle_args *gpiospec,u32 *flags) +{ +u32 idx = gpiospec->args[0 ]; + +if (idx >LPC32XX_GPIO_BANKS || &lpc32xx_gpio_chips[idx]->chip != gc) +return -EINVAL ;
return of_gpio_simple_xlate(gc, gpiospec, flags);
drivers/gpio/gpio-lpc32xx.c line 336
Quoted 6 lines
drivers/gpio/gpio-lpc32xx.c new file +6
-0+ +if (flags) + *flags = gpiospec->args[2 ]; + +return gpiospec->args[1 ]; +}
Then you don't need to reimplement this, right?
Review
Quoted 2 lines
(quoted diff) +2
-0+static int lpc32xx_add_irqchip(struct lpc32xx_gpio_chip *bank, + struct device_node *node)
Please try to use GPIOLIB_IRQCHIP instead. Please.
drivers/gpio/gpio-lpc32xx.c line 352
Quoted 13 lines
drivers/gpio/gpio-lpc32xx.c new file +13
-0+{ +int idx, virq; +struct gpio_chip *gpiochip = &bank->chip; +struct irq_chip *irqchip; + + irqchip =devm_kzalloc (bank->dev,sizeof (*irqchip),GFP_KERNEL ); +if (!irqchip) +return -ENOMEM ; + + irqchip->name = bank->chip.label; + irqchip->irq_startup = lpc32xx_gpio_irq_startup; + irqchip->irq_shutdown = lpc32xx_gpio_irq_shutdown; + irqchip->irq_set_type = lpc32xx_gpio_irq_set_type;
If you persist on this, you need to have .irq_request_resources()/.irq_release_resources() calling gpiochip_lock_as_irq() and gpiochip_unlock_as_irq() to protect the IRQ lines. GPIOLIB_IRQCHIP would again handle this for you.
Quoted 22 lines
drivers/gpio/gpio-lpc32xx.c new file +22
-0+static int lpc32xx_set_irqs (struct lpc32xx_gpio_chip *bank, +struct device_node *node) +{ +int idx, ret; +u32 nirq; + + nirq =bit_count (bank->interrupt_mask); +if (nirq !=of_irq_count (node)) { +dev_err (bank->dev,"mismatch in interrupt configuration\n" ); +return -EINVAL ; + } + + bank->irqs =devm_kzalloc (bank->dev,sizeof (*bank->irqs) * nirq, +GFP_KERNEL ); +if (!bank->irqs) +return -ENOMEM ; + +for (idx =0 ; idx < nirq; idx++) { + bank->irqs[idx].parent =irq_of_parse_and_map (node, idx); +if (!bank->irqs[idx].parent) { +dev_err (bank->dev,"can not parse irq %d\n" , idx); +return -EINVAL ;
drivers/gpio/gpio-lpc32xx.c line 414
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+ } + + bank->irqs[idx].bank = bank; + bank->irqs[idx].idx = idx; + + ret =bit_map (idx, bank->interrupt_mask, bank->input_mask); +if (ret <0 ) +return ret; + bank->irqs[idx].gpio = ret; + }
This all looks weird to me. A bit like a reimplemented irqdomain, and I have no clue what this code is doing because this weird IRQ scheme is not explained anywhere.
I strongly suspect that creating one device per GPIO bank would make things simpler. Or at least one GPIO chip per bank.
drivers/gpio/gpio-lpc32xx.c line 430
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+static int lpc32xx_set_gpio_names (struct lpc32xx_gpio_chip *bank,u32 ngpio) +{ +char *gpio_names; +u32 mask, idx, val =0 , step =strlen (bank->chip.label) +4 ; + + bank->gpio_names =devm_kzalloc (bank->dev, + ngpio *sizeof (*bank->gpio_names), +GFP_KERNEL ); +if (!bank->gpio_names) +return -ENOMEM ;
The gpio_chip has a names field. If you want to do this stuff, use that.
Quoted 6 lines
drivers/gpio/gpio-lpc32xx.c new file +6
-0+static int of_node_to_gpio (struct device *dev,struct device_node *node) +{ +int ret, id; +struct lpc32xx_gpio_chip *bank; +u32 ngpio =0 ; +
drivers/gpio/gpio-lpc32xx.c line 483
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+ bank =devm_kzalloc (dev,sizeof (*bank),GFP_KERNEL ); +if (!bank) +return -ENOMEM ; + bank->dev = dev; + + bank->base =of_iomap (node,0 ); +if (!bank->base) { +dev_err (dev,"cannot map GPIO registers\n" ); +return -ENOMEM ; + }
If every bank has its own register range it *should* be its own device too.
drivers/gpio/gpio-lpc32xx.c line 491
Quoted 7 lines
drivers/gpio/gpio-lpc32xx.c new file +7
-0+ bank->input_only =of_property_read_bool (node,"gpio-input-only" ); + bank->output_only =of_property_read_bool (node,"gpio-output-only" ); +if (bank->input_only && bank->output_only) { +dev_err (dev,"%s: input-only and output-only is invalid\n" , + node->full_name); +return -EINVAL ; + }
Since these properties are named "gpio-*" not "nxp,*" they should be documented in Documentation/devicetree/bindings/gpio/gpio.txt before you use them. (They seem totally reasonable and useful to me!)
Quoted 1 line
+
of_property_read_u32(node, "gpio-offset", &bank->offset);
What is this exactly?
Quoted 69 lines
drivers/gpio/gpio-lpc32xx.c new file +69
-0+ +if (of_find_property (node,"gpio-input-mask" , &id)) { +if (id <2 *sizeof (u32)) { +dev_err (dev,"invalid format of gpio-input-mask\n" ); +return -EINVAL ; + } + +of_property_read_u32_index (node,"gpio-input-mask" , +0 , &bank->input_mask); +of_property_read_u32_index (node,"gpio-input-mask" , +1 , &bank->interrupt_mask); + +if ((bank->input_mask & bank->interrupt_mask) != + bank->interrupt_mask) { +dev_err (dev,"interrupt mask must be a subset of input mask\n" ); +return -EINVAL ; + } + + ret =lpc32xx_set_irqs (bank, node); +if (ret) +return ret; + } + +if (bank->input_mask) + ngpio =bit_count (bank->input_mask); +else +of_property_read_u32 (node,"gpios" , &ngpio); +if (!ngpio) { +dev_err (dev,"cannot get number of gpios\n" ); +return -EINVAL ; + } + +/* +* Special handling of P2, the bank does not have output state +* register and its mapping starts from direction control registers +*/ +if (of_property_read_bool (node,"gpio-no-output-state" )) { + bank->output_states =devm_kzalloc (dev, ngpio * (sizeof (u32)), +GFP_KERNEL ); +if (!bank->output_states) +return -ENOMEM ; + + bank->input = bank->base +LPC32XX_GPIO_P2_INPUT ; + bank->output = bank->base +LPC32XX_GPIO_P2_OUTPUT ; + bank->dir = bank->base +LPC32XX_GPIO_P2_DIR ; + }else { + bank->input = bank->base +LPC32XX_GPIO_INPUT ; + bank->output = bank->base +LPC32XX_GPIO_OUTPUT ; + bank->dir = bank->base +LPC32XX_GPIO_DIR ; + } + +of_property_read_string (node,"gpio-bank-name" , &bank->chip.label); +if (bank->chip.label) { + ret =lpc32xx_set_gpio_names (bank, ngpio); +if (ret) +return ret; + } + +/* Default GPIO bank enumeration, first address cell from reg */ + ret =of_property_read_u32 (node,"reg" , &id); +if (ret <0 || id >=LPC32XX_GPIO_BANKS ) { +dev_err (dev,"can not get valid GPIO bank id\n" ); +return -EINVAL ; + } + + lpc32xx_gpio_chips[id] = bank; + + bank->chip.dev = dev; +
drivers/gpio/gpio-lpc32xx.c line 572
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+ bank->chip.direction_input = lpc32xx_gpio_dir_input; + bank->chip.direction_output = lpc32xx_gpio_dir_output; + bank->chip.get_direction = lpc32xx_gpio_dir_get; + + bank->chip.get = lpc32xx_gpio_get; + bank->chip.set = lpc32xx_gpio_set; + + bank->chip.to_irq = lpc32xx_gpio_to_irq; + + bank->chip.base = id *32 ;
Use -1 instead so you get a dynamically assigned base.
drivers/gpio/gpio-lpc32xx.c line 578
Quoted 6 lines
drivers/gpio/gpio-lpc32xx.c new file +6
-0+ bank->chip.ngpio = ngpio; + bank->chip.names = bank->gpio_names; + bank->chip.can_sleep = false; + + bank->chip.of_xlate = lpc32xx_of_xlate; + bank->chip.of_gpio_n_cells =3 ;
I wonder if you even need your own translation functions. With one device per bank, certainly not.
Quoted 15 lines
drivers/gpio/gpio-lpc32xx.c new file +15
-0+ bank->chip.of_node = dev->of_node; + + ret =gpiochip_add (&bank->chip); +if (ret) +return ret; + +if (of_find_property (node,"interrupt-controller" ,NULL )) { +if (bank->input_mask) + ret =lpc32xx_add_irqchip (bank, node); + } + +return ret; +} + +static int lpc32xx_gpio_remove (struct platform_device *pdev);
drivers/gpio/gpio-lpc32xx.c line 603
Quoted 10 lines
drivers/gpio/gpio-lpc32xx.c new file +10
-0+static int lpc32xx_gpio_probe (struct platform_device *pdev) +{ +struct device_node *child; +int ret; + +for_each_available_child_of_node (pdev->dev.of_node, child) { + ret =of_node_to_gpio (&pdev->dev, child); +if (ret) + goto error; + }
If the node in the device tree just declares "simple-bus" it will spawn devices for all subnodes, if they have compatible ="" strings.
drivers/gpio/gpio-lpc32xx.c line 623
Quoted 8 lines
drivers/gpio/gpio-lpc32xx.c new file +8
-0+static int lpc32xx_gpio_remove (struct platform_device *pdev) +{ +u32 idx, i; +struct irq_domain *irqd; + +for (idx =0 ; idx <LPC32XX_GPIO_BANKS ; idx++) { +if (!lpc32xx_gpio_chips[idx]) +continue ;
Use the platform device data to find a reference to the chip and remove it. Get rid of this complicated array. Look at how other drivers do it.
Just use platform_set_drvdata(pdev, foo); in probe() then in remove():
struct my_gpio *foo = platform_get_drvdata(pdev);
gpiochip_remove(&foo->gc);
Like that.
Yours,
Linus Walleij
cover gpio: lpc32xx: add new LPC32xx GPIO controller driver
In reply to Vladimir ZapolskiyQuoted 4 lines · Vladimir Zapolskiy
Unfortunately the old driver has too many hacks inherited from the ancient legacy NXP BSP and I find it can not be maintained, the new driver has compatible interface to device tree clients, so moving to the new driver should not impact any GPIO consumers.
Has it been tested on the old hardware too?
Yours,
Linus Walleij
Vladimir Zapolskiy
maintainercover gpio: lpc32xx: add new LPC32xx GPIO controller driver
Hi Linus,
Quoted 8 lines · Linus Walleij
Quoted 4 lines · Vladimir Zapolskiy
Unfortunately the old driver has too many hacks inherited from the ancient legacy NXP BSP and I find it can not be maintained, the new driver has compatible interface to device tree clients, so moving to the new driver should not impact any GPIO consumers.
Has it been tested on the old hardware too?
the hardware here is the same, the driver is different.
With the existing LPC32xx DTS files I tested that the DTS GPIO client's interface is unchanged and that GPIO client drivers correctly work with the new GPIO controller driver.
As a summary about the change,
* DT binding of GPIO controller in lpc32xx.dtsi is extended (no removed properties),
* client's DT binding to the GPIO controller are untouched,
* GPIO controller code is completely replaced,
* GPIO client's side is untouched.
As a reminder about the necessity of the change, the legacy driver operates with hardware IRQs, this prevents switching of the platfrom to SPARSE_IRQ model.
It is important that the change depends on IRQ chip driver, which is also under review.
I would appreciate, if review of this change can be started, so that both IRQ chip driver and GPIO controller driver are accepted in one merge window.
--
With best wishes,
Vladimir