[PATCH 0/4] gpio: lpc32xx: add new LPC32xx GPIO controller driver

[PATCH 0/4] gpio: lpc32xx: add new LPC32xx GPIO controller driver

[Vladimir Zapolskiy]

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(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] arm: dts: lpc32xx: extend description of gpio controller node

[Vladimir Zapolskiy]

The 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.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 arch/arm/boot/dts/lpc32xx.dtsi | 85 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 84 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/lpc32xx.dtsi b/arch/arm/boot/dts/lpc32xx.dtsi
index 603f005..9153654 100644
--- a/arch/arm/boot/dts/lpc32xx.dtsi
+++ b/arch/arm/boot/dts/lpc32xx.dtsi
@@ -398,11 +398,94 @@
 				clocks = <&clk LPC32XX_CLK_RTC>;
 			};
 
-			gpio: gpio@40028000 {
+			gpio: gpio-controller@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@0 {
+					reg = <0 0x40 0x1C>;
+					gpio-bank-name = "p0";
+					gpios = <8>;
+
+					interrupt-parent = <&sic2>;
+					interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
+				};
+
+				gpio_p1: gpio-controller@1 {
+					reg = <1 0x60 0x1C>;
+					gpio-bank-name = "p1";
+					gpios = <24>;
+
+					interrupt-parent = <&sic2>;
+					interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
+				};
+
+				gpio_p2: gpio-controller@2 {
+					reg = <2 0x10 0x18>;
+					gpio-bank-name = "p2";
+					gpios = <13>;
+					gpio-no-output-state;
+				};
+
+				gpio_gpio: gpio-controller@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@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@5 {
+					reg = <5 0x00 0x10>;
+					gpio-bank-name = "gpo";
+					gpios = <24>;
+					gpio-output-only;
+				};
 			};
 
 			timer4: timer@4002C000 {
-- 
2.1.4

[PATCH 1/4] dt-bindings: gpio: update desription of LPC32xx GPIO controller

[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 <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/gpio/gpio_lpc32xx.txt      | 121 ++++++++++++++++++++-
 1 file changed, 120 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
index 4981936..d2da63c 100644
--- a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
@@ -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@0 {
+			reg = <0 0x40 0x1C>;
+			gpio-bank-name = "p0";
+			gpios = <8>;
+
+			interrupt-parent = <&sic2>;
+			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpio_p1: gpio-controller@1 {
+			reg = <1 0x60 0x1C>;
+			gpio-bank-name = "p1";
+			gpios = <24>;
+
+			interrupt-parent = <&sic2>;
+			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
+		};
+
+		gpio_p2: gpio-controller@2 {
+			reg = <2 0x10 0x18>;
+			gpio-bank-name = "p2";
+			gpios = <13>;
+			gpio-no-output-state;
+		};
+
+		gpio_gpio: gpio-controller@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@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@5 {
+			reg = <5 0x00 0x10>;
+			gpio-bank-name = "gpo";
+			gpios = <24>;
+			gpio-output-only;
+		};
 	};
 
 	leds {
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] dt-bindings: gpio: update desription of LPC32xx GPIO controller

[Rob Herring]

On Fri, Nov 20, 2015 at 03:29:52AM +0200, Vladimir Zapolskiy wrote:
> 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 <vz@mleia.com>
> ---
>  .../devicetree/bindings/gpio/gpio_lpc32xx.txt      | 121 ++++++++++++++++++++-
>  1 file changed, 120 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
> index 4981936..d2da63c 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
> @@ -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?

> +- #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,

Seems like nr-gpios should have been a mask instead...

> +- 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.

> +- 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 */

Can't bank and pin be encoded into one cell as the gpio core binding 
suggests.

> +
> +		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@0 {
> +			reg = <0 0x40 0x1C>;
> +			gpio-bank-name = "p0";
> +			gpios = <8>;
> +
> +			interrupt-parent = <&sic2>;
> +			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpio_p1: gpio-controller@1 {
> +			reg = <1 0x60 0x1C>;
> +			gpio-bank-name = "p1";
> +			gpios = <24>;
> +
> +			interrupt-parent = <&sic2>;
> +			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
> +		};
> +
> +		gpio_p2: gpio-controller@2 {
> +			reg = <2 0x10 0x18>;
> +			gpio-bank-name = "p2";
> +			gpios = <13>;
> +			gpio-no-output-state;
> +		};
> +
> +		gpio_gpio: gpio-controller@3 {
> +			reg = <3 0x00 0x1C>;

This overlaps with bank 2.

> +			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@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@5 {
> +			reg = <5 0x00 0x10>;
> +			gpio-bank-name = "gpo";
> +			gpios = <24>;
> +			gpio-output-only;
> +		};
>  	};
>  
>  	leds {
> -- 
> 2.1.4
> 

Re: [PATCH 1/4] dt-bindings: gpio: update desription of LPC32xx GPIO controller

[Vladimir Zapolskiy]

On 20.11.2015 16:13, Rob Herring wrote:
> On Fri, Nov 20, 2015 at 03:29:52AM +0200, Vladimir Zapolskiy wrote:
>> 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 <vz@mleia.com>
>> ---
>>  .../devicetree/bindings/gpio/gpio_lpc32xx.txt      | 121 ++++++++++++++++++++-
>>  1 file changed, 120 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
>> index 4981936..d2da63c 100644
>> --- a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
>> +++ b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
>> @@ -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...

> 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?

>> +- #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,
> 
> Seems like nr-gpios should have been a mask instead...
> 
>> +- 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.
> 
>> +- 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 */
> 
> 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.

>> +
>> +		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@0 {
>> +			reg = <0 0x40 0x1C>;
>> +			gpio-bank-name = "p0";
>> +			gpios = <8>;
>> +
>> +			interrupt-parent = <&sic2>;
>> +			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		gpio_p1: gpio-controller@1 {
>> +			reg = <1 0x60 0x1C>;
>> +			gpio-bank-name = "p1";
>> +			gpios = <24>;
>> +
>> +			interrupt-parent = <&sic2>;
>> +			interrupts = <8 IRQ_TYPE_LEVEL_HIGH>;
>> +		};
>> +
>> +		gpio_p2: gpio-controller@2 {
>> +			reg = <2 0x10 0x18>;
>> +			gpio-bank-name = "p2";
>> +			gpios = <13>;
>> +			gpio-no-output-state;
>> +		};
>> +
>> +		gpio_gpio: gpio-controller@3 {
>> +			reg = <3 0x00 0x1C>;
> 
> This overlaps with bank 2.

Yes, it is. Thousand thanks to hardware designers.

--
With best wishes,
Vladimir

Re: [PATCH 1/4] dt-bindings: gpio: update desription of LPC32xx GPIO controller

[Rob Herring]

On Fri, Nov 20, 2015 at 08:27:35PM +0200, Vladimir Zapolskiy wrote:
> On 20.11.2015 16:13, Rob Herring wrote:
> > On Fri, Nov 20, 2015 at 03:29:52AM +0200, Vladimir Zapolskiy wrote:
> >> 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 <vz@mleia.com>
> >> ---
> >>  .../devicetree/bindings/gpio/gpio_lpc32xx.txt      | 121 ++++++++++++++++++++-
> >>  1 file changed, 120 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
> >> index 4981936..d2da63c 100644
> >> --- a/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
> >> +++ b/Documentation/devicetree/bindings/gpio/gpio_lpc32xx.txt
> >> @@ -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...
> 
> > 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 ...?

> 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.


> >> +		gpio_p2: gpio-controller@2 {
> >> +			reg = <2 0x10 0x18>;
> >> +			gpio-bank-name = "p2";
> >> +			gpios = <13>;
> >> +			gpio-no-output-state;
> >> +		};
> >> +
> >> +		gpio_gpio: gpio-controller@3 {
> >> +			reg = <3 0x00 0x1C>;
> > 
> > This overlaps with bank 2.
> 
> 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

Re: [PATCH 1/4] dt-bindings: gpio: update desription of LPC32xx GPIO controller

[Linus Walleij]

On Fri, Nov 20, 2015 at 2:29 AM, Vladimir Zapolskiy <vz@mleia.com> wrote:

> 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 <vz@mleia.com>

(...)

> +
> +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.

> +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.

> +- 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

> +- 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.

> +- 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.

> +- 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,

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.

> +- 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.

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.

> +                       gpio-output-only;

You forgot to documen this property.

Yours,
Linus Walleij

Re: [PATCH 1/4] dt-bindings: gpio: update desription of LPC32xx GPIO controller

[Vladimir Zapolskiy]

Hi Linus,

On 30.11.2015 12:40, Linus Walleij wrote:
> On Fri, Nov 20, 2015 at 2:29 AM, Vladimir Zapolskiy <vz@mleia.com> wrote:
> 
>> 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 <vz@mleia.com>
> 
> (...)
> 
>> +
>> +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

> 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.

>> +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@xxx device node names with p0@xxx etc.?

>> +- 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.

>> +- 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.

>> +- 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...

>> +- 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,
> 
> 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.

>> +- 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.
> 
> 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.

>> +                       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

Re: [PATCH 1/4] dt-bindings: gpio: update desription of LPC32xx GPIO controller

[Linus Walleij]

On Mon, Nov 30, 2015 at 1:13 PM, Vladimir Zapolskiy
<vladimir_zapolskiy-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org> wrote:
> [Me]
>> 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@xxx device node names with p0@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.

>>> +- 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?

>>> +- 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.

>>> +- 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,
>>
>> 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.

OK, and should it be an nxp,* property?

>>> +- 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.
>>
>> 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.

OMG that sounds so weird.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] gpio: lpc32xx: remove legacy LPC32xx GPIO driver

[Vladimir Zapolskiy]

Legacy 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.

Signed-off-by: Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
---
 drivers/gpio/Makefile       |   1 -
 drivers/gpio/gpio-lpc32xx.c | 577 --------------------------------------------
 2 files changed, 578 deletions(-)
 delete mode 100644 drivers/gpio/gpio-lpc32xx.c

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 986dbd8..e3ccd99 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -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
diff --git a/drivers/gpio/gpio-lpc32xx.c b/drivers/gpio/gpio-lpc32xx.c
deleted file mode 100644
index 47e2dde..0000000
--- a/drivers/gpio/gpio-lpc32xx.c
+++ /dev/null
@@ -1,577 +0,0 @@
-/*
- * GPIO driver for LPC32xx SoC
- *
- * Author: Kevin Wells <kevin.wells-3arQi8VN3Tc@public.gmane.org>
- *
- * Copyright (C) 2010 NXP Semiconductors
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/io.h>
-#include <linux/errno.h>
-#include <linux/gpio.h>
-#include <linux/of.h>
-#include <linux/of_gpio.h>
-#include <linux/platform_device.h>
-#include <linux/module.h>
-#include <linux/platform_data/gpio-lpc32xx.h>
-
-#include <mach/hardware.h>
-#include <mach/platform.h>
-#include <mach/irqs.h>
-
-#define LPC32XX_GPIO_P3_INP_STATE		_GPREG(0x000)
-#define LPC32XX_GPIO_P3_OUTP_SET		_GPREG(0x004)
-#define LPC32XX_GPIO_P3_OUTP_CLR		_GPREG(0x008)
-#define LPC32XX_GPIO_P3_OUTP_STATE		_GPREG(0x00C)
-#define LPC32XX_GPIO_P2_DIR_SET			_GPREG(0x010)
-#define LPC32XX_GPIO_P2_DIR_CLR			_GPREG(0x014)
-#define LPC32XX_GPIO_P2_DIR_STATE		_GPREG(0x018)
-#define LPC32XX_GPIO_P2_INP_STATE		_GPREG(0x01C)
-#define LPC32XX_GPIO_P2_OUTP_SET		_GPREG(0x020)
-#define LPC32XX_GPIO_P2_OUTP_CLR		_GPREG(0x024)
-#define LPC32XX_GPIO_P2_MUX_SET			_GPREG(0x028)
-#define LPC32XX_GPIO_P2_MUX_CLR			_GPREG(0x02C)
-#define LPC32XX_GPIO_P2_MUX_STATE		_GPREG(0x030)
-#define LPC32XX_GPIO_P0_INP_STATE		_GPREG(0x040)
-#define LPC32XX_GPIO_P0_OUTP_SET		_GPREG(0x044)
-#define LPC32XX_GPIO_P0_OUTP_CLR		_GPREG(0x048)
-#define LPC32XX_GPIO_P0_OUTP_STATE		_GPREG(0x04C)
-#define LPC32XX_GPIO_P0_DIR_SET			_GPREG(0x050)
-#define LPC32XX_GPIO_P0_DIR_CLR			_GPREG(0x054)
-#define LPC32XX_GPIO_P0_DIR_STATE		_GPREG(0x058)
-#define LPC32XX_GPIO_P1_INP_STATE		_GPREG(0x060)
-#define LPC32XX_GPIO_P1_OUTP_SET		_GPREG(0x064)
-#define LPC32XX_GPIO_P1_OUTP_CLR		_GPREG(0x068)
-#define LPC32XX_GPIO_P1_OUTP_STATE		_GPREG(0x06C)
-#define LPC32XX_GPIO_P1_DIR_SET			_GPREG(0x070)
-#define LPC32XX_GPIO_P1_DIR_CLR			_GPREG(0x074)
-#define LPC32XX_GPIO_P1_DIR_STATE		_GPREG(0x078)
-
-#define GPIO012_PIN_TO_BIT(x)			(1 << (x))
-#define GPIO3_PIN_TO_BIT(x)			(1 << ((x) + 25))
-#define GPO3_PIN_TO_BIT(x)			(1 << (x))
-#define GPIO012_PIN_IN_SEL(x, y)		(((x) >> (y)) & 1)
-#define GPIO3_PIN_IN_SHIFT(x)			((x) == 5 ? 24 : 10 + (x))
-#define GPIO3_PIN_IN_SEL(x, y)			(((x) >> GPIO3_PIN_IN_SHIFT(y)) & 1)
-#define GPIO3_PIN5_IN_SEL(x)			(((x) >> 24) & 1)
-#define GPI3_PIN_IN_SEL(x, y)			(((x) >> (y)) & 1)
-#define GPO3_PIN_IN_SEL(x, y)			(((x) >> (y)) & 1)
-
-struct gpio_regs {
-	void __iomem *inp_state;
-	void __iomem *outp_state;
-	void __iomem *outp_set;
-	void __iomem *outp_clr;
-	void __iomem *dir_set;
-	void __iomem *dir_clr;
-};
-
-/*
- * GPIO names
- */
-static const char *gpio_p0_names[LPC32XX_GPIO_P0_MAX] = {
-	"p0.0", "p0.1", "p0.2", "p0.3",
-	"p0.4", "p0.5", "p0.6", "p0.7"
-};
-
-static const char *gpio_p1_names[LPC32XX_GPIO_P1_MAX] = {
-	"p1.0", "p1.1", "p1.2", "p1.3",
-	"p1.4", "p1.5", "p1.6", "p1.7",
-	"p1.8", "p1.9", "p1.10", "p1.11",
-	"p1.12", "p1.13", "p1.14", "p1.15",
-	"p1.16", "p1.17", "p1.18", "p1.19",
-	"p1.20", "p1.21", "p1.22", "p1.23",
-};
-
-static const char *gpio_p2_names[LPC32XX_GPIO_P2_MAX] = {
-	"p2.0", "p2.1", "p2.2", "p2.3",
-	"p2.4", "p2.5", "p2.6", "p2.7",
-	"p2.8", "p2.9", "p2.10", "p2.11",
-	"p2.12"
-};
-
-static const char *gpio_p3_names[LPC32XX_GPIO_P3_MAX] = {
-	"gpio00", "gpio01", "gpio02", "gpio03",
-	"gpio04", "gpio05"
-};
-
-static const char *gpi_p3_names[LPC32XX_GPI_P3_MAX] = {
-	"gpi00", "gpi01", "gpi02", "gpi03",
-	"gpi04", "gpi05", "gpi06", "gpi07",
-	"gpi08", "gpi09",  NULL,    NULL,
-	 NULL,    NULL,    NULL,   "gpi15",
-	"gpi16", "gpi17", "gpi18", "gpi19",
-	"gpi20", "gpi21", "gpi22", "gpi23",
-	"gpi24", "gpi25", "gpi26", "gpi27",
-	"gpi28"
-};
-
-static const char *gpo_p3_names[LPC32XX_GPO_P3_MAX] = {
-	"gpo00", "gpo01", "gpo02", "gpo03",
-	"gpo04", "gpo05", "gpo06", "gpo07",
-	"gpo08", "gpo09", "gpo10", "gpo11",
-	"gpo12", "gpo13", "gpo14", "gpo15",
-	"gpo16", "gpo17", "gpo18", "gpo19",
-	"gpo20", "gpo21", "gpo22", "gpo23"
-};
-
-static struct gpio_regs gpio_grp_regs_p0 = {
-	.inp_state	= LPC32XX_GPIO_P0_INP_STATE,
-	.outp_set	= LPC32XX_GPIO_P0_OUTP_SET,
-	.outp_clr	= LPC32XX_GPIO_P0_OUTP_CLR,
-	.dir_set	= LPC32XX_GPIO_P0_DIR_SET,
-	.dir_clr	= LPC32XX_GPIO_P0_DIR_CLR,
-};
-
-static struct gpio_regs gpio_grp_regs_p1 = {
-	.inp_state	= LPC32XX_GPIO_P1_INP_STATE,
-	.outp_set	= LPC32XX_GPIO_P1_OUTP_SET,
-	.outp_clr	= LPC32XX_GPIO_P1_OUTP_CLR,
-	.dir_set	= LPC32XX_GPIO_P1_DIR_SET,
-	.dir_clr	= LPC32XX_GPIO_P1_DIR_CLR,
-};
-
-static struct gpio_regs gpio_grp_regs_p2 = {
-	.inp_state	= LPC32XX_GPIO_P2_INP_STATE,
-	.outp_set	= LPC32XX_GPIO_P2_OUTP_SET,
-	.outp_clr	= LPC32XX_GPIO_P2_OUTP_CLR,
-	.dir_set	= LPC32XX_GPIO_P2_DIR_SET,
-	.dir_clr	= LPC32XX_GPIO_P2_DIR_CLR,
-};
-
-static struct gpio_regs gpio_grp_regs_p3 = {
-	.inp_state	= LPC32XX_GPIO_P3_INP_STATE,
-	.outp_state	= LPC32XX_GPIO_P3_OUTP_STATE,
-	.outp_set	= LPC32XX_GPIO_P3_OUTP_SET,
-	.outp_clr	= LPC32XX_GPIO_P3_OUTP_CLR,
-	.dir_set	= LPC32XX_GPIO_P2_DIR_SET,
-	.dir_clr	= LPC32XX_GPIO_P2_DIR_CLR,
-};
-
-struct lpc32xx_gpio_chip {
-	struct gpio_chip	chip;
-	struct gpio_regs	*gpio_grp;
-};
-
-static inline struct lpc32xx_gpio_chip *to_lpc32xx_gpio(
-	struct gpio_chip *gpc)
-{
-	return container_of(gpc, struct lpc32xx_gpio_chip, chip);
-}
-
-static void __set_gpio_dir_p012(struct lpc32xx_gpio_chip *group,
-	unsigned pin, int input)
-{
-	if (input)
-		__raw_writel(GPIO012_PIN_TO_BIT(pin),
-			group->gpio_grp->dir_clr);
-	else
-		__raw_writel(GPIO012_PIN_TO_BIT(pin),
-			group->gpio_grp->dir_set);
-}
-
-static void __set_gpio_dir_p3(struct lpc32xx_gpio_chip *group,
-	unsigned pin, int input)
-{
-	u32 u = GPIO3_PIN_TO_BIT(pin);
-
-	if (input)
-		__raw_writel(u, group->gpio_grp->dir_clr);
-	else
-		__raw_writel(u, group->gpio_grp->dir_set);
-}
-
-static void __set_gpio_level_p012(struct lpc32xx_gpio_chip *group,
-	unsigned pin, int high)
-{
-	if (high)
-		__raw_writel(GPIO012_PIN_TO_BIT(pin),
-			group->gpio_grp->outp_set);
-	else
-		__raw_writel(GPIO012_PIN_TO_BIT(pin),
-			group->gpio_grp->outp_clr);
-}
-
-static void __set_gpio_level_p3(struct lpc32xx_gpio_chip *group,
-	unsigned pin, int high)
-{
-	u32 u = GPIO3_PIN_TO_BIT(pin);
-
-	if (high)
-		__raw_writel(u, group->gpio_grp->outp_set);
-	else
-		__raw_writel(u, group->gpio_grp->outp_clr);
-}
-
-static void __set_gpo_level_p3(struct lpc32xx_gpio_chip *group,
-	unsigned pin, int high)
-{
-	if (high)
-		__raw_writel(GPO3_PIN_TO_BIT(pin), group->gpio_grp->outp_set);
-	else
-		__raw_writel(GPO3_PIN_TO_BIT(pin), group->gpio_grp->outp_clr);
-}
-
-static int __get_gpio_state_p012(struct lpc32xx_gpio_chip *group,
-	unsigned pin)
-{
-	return GPIO012_PIN_IN_SEL(__raw_readl(group->gpio_grp->inp_state),
-		pin);
-}
-
-static int __get_gpio_state_p3(struct lpc32xx_gpio_chip *group,
-	unsigned pin)
-{
-	int state = __raw_readl(group->gpio_grp->inp_state);
-
-	/*
-	 * P3 GPIO pin input mapping is not contiguous, GPIOP3-0..4 is mapped
-	 * to bits 10..14, while GPIOP3-5 is mapped to bit 24.
-	 */
-	return GPIO3_PIN_IN_SEL(state, pin);
-}
-
-static int __get_gpi_state_p3(struct lpc32xx_gpio_chip *group,
-	unsigned pin)
-{
-	return GPI3_PIN_IN_SEL(__raw_readl(group->gpio_grp->inp_state), pin);
-}
-
-static int __get_gpo_state_p3(struct lpc32xx_gpio_chip *group,
-	unsigned pin)
-{
-	return GPO3_PIN_IN_SEL(__raw_readl(group->gpio_grp->outp_state), pin);
-}
-
-/*
- * GPIO primitives.
- */
-static int lpc32xx_gpio_dir_input_p012(struct gpio_chip *chip,
-	unsigned pin)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	__set_gpio_dir_p012(group, pin, 1);
-
-	return 0;
-}
-
-static int lpc32xx_gpio_dir_input_p3(struct gpio_chip *chip,
-	unsigned pin)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	__set_gpio_dir_p3(group, pin, 1);
-
-	return 0;
-}
-
-static int lpc32xx_gpio_dir_in_always(struct gpio_chip *chip,
-	unsigned pin)
-{
-	return 0;
-}
-
-static int lpc32xx_gpio_get_value_p012(struct gpio_chip *chip, unsigned pin)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	return __get_gpio_state_p012(group, pin);
-}
-
-static int lpc32xx_gpio_get_value_p3(struct gpio_chip *chip, unsigned pin)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	return __get_gpio_state_p3(group, pin);
-}
-
-static int lpc32xx_gpi_get_value(struct gpio_chip *chip, unsigned pin)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	return __get_gpi_state_p3(group, pin);
-}
-
-static int lpc32xx_gpio_dir_output_p012(struct gpio_chip *chip, unsigned pin,
-	int value)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	__set_gpio_level_p012(group, pin, value);
-	__set_gpio_dir_p012(group, pin, 0);
-
-	return 0;
-}
-
-static int lpc32xx_gpio_dir_output_p3(struct gpio_chip *chip, unsigned pin,
-	int value)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	__set_gpio_level_p3(group, pin, value);
-	__set_gpio_dir_p3(group, pin, 0);
-
-	return 0;
-}
-
-static int lpc32xx_gpio_dir_out_always(struct gpio_chip *chip, unsigned pin,
-	int value)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	__set_gpo_level_p3(group, pin, value);
-	return 0;
-}
-
-static void lpc32xx_gpio_set_value_p012(struct gpio_chip *chip, unsigned pin,
-	int value)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	__set_gpio_level_p012(group, pin, value);
-}
-
-static void lpc32xx_gpio_set_value_p3(struct gpio_chip *chip, unsigned pin,
-	int value)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	__set_gpio_level_p3(group, pin, value);
-}
-
-static void lpc32xx_gpo_set_value(struct gpio_chip *chip, unsigned pin,
-	int value)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	__set_gpo_level_p3(group, pin, value);
-}
-
-static int lpc32xx_gpo_get_value(struct gpio_chip *chip, unsigned pin)
-{
-	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
-
-	return __get_gpo_state_p3(group, pin);
-}
-
-static int lpc32xx_gpio_request(struct gpio_chip *chip, unsigned pin)
-{
-	if (pin < chip->ngpio)
-		return 0;
-
-	return -EINVAL;
-}
-
-static int lpc32xx_gpio_to_irq_p01(struct gpio_chip *chip, unsigned offset)
-{
-	return IRQ_LPC32XX_P0_P1_IRQ;
-}
-
-static const char lpc32xx_gpio_to_irq_gpio_p3_table[] = {
-	IRQ_LPC32XX_GPIO_00,
-	IRQ_LPC32XX_GPIO_01,
-	IRQ_LPC32XX_GPIO_02,
-	IRQ_LPC32XX_GPIO_03,
-	IRQ_LPC32XX_GPIO_04,
-	IRQ_LPC32XX_GPIO_05,
-};
-
-static int lpc32xx_gpio_to_irq_gpio_p3(struct gpio_chip *chip, unsigned offset)
-{
-	if (offset < ARRAY_SIZE(lpc32xx_gpio_to_irq_gpio_p3_table))
-		return lpc32xx_gpio_to_irq_gpio_p3_table[offset];
-	return -ENXIO;
-}
-
-static const char lpc32xx_gpio_to_irq_gpi_p3_table[] = {
-	IRQ_LPC32XX_GPI_00,
-	IRQ_LPC32XX_GPI_01,
-	IRQ_LPC32XX_GPI_02,
-	IRQ_LPC32XX_GPI_03,
-	IRQ_LPC32XX_GPI_04,
-	IRQ_LPC32XX_GPI_05,
-	IRQ_LPC32XX_GPI_06,
-	IRQ_LPC32XX_GPI_07,
-	IRQ_LPC32XX_GPI_08,
-	IRQ_LPC32XX_GPI_09,
-	-ENXIO, /* 10 */
-	-ENXIO, /* 11 */
-	-ENXIO, /* 12 */
-	-ENXIO, /* 13 */
-	-ENXIO, /* 14 */
-	-ENXIO, /* 15 */
-	-ENXIO, /* 16 */
-	-ENXIO, /* 17 */
-	-ENXIO, /* 18 */
-	IRQ_LPC32XX_GPI_19,
-	-ENXIO, /* 20 */
-	-ENXIO, /* 21 */
-	-ENXIO, /* 22 */
-	-ENXIO, /* 23 */
-	-ENXIO, /* 24 */
-	-ENXIO, /* 25 */
-	-ENXIO, /* 26 */
-	-ENXIO, /* 27 */
-	IRQ_LPC32XX_GPI_28,
-};
-
-static int lpc32xx_gpio_to_irq_gpi_p3(struct gpio_chip *chip, unsigned offset)
-{
-	if (offset < ARRAY_SIZE(lpc32xx_gpio_to_irq_gpi_p3_table))
-		return lpc32xx_gpio_to_irq_gpi_p3_table[offset];
-	return -ENXIO;
-}
-
-static struct lpc32xx_gpio_chip lpc32xx_gpiochip[] = {
-	{
-		.chip = {
-			.label			= "gpio_p0",
-			.direction_input	= lpc32xx_gpio_dir_input_p012,
-			.get			= lpc32xx_gpio_get_value_p012,
-			.direction_output	= lpc32xx_gpio_dir_output_p012,
-			.set			= lpc32xx_gpio_set_value_p012,
-			.request		= lpc32xx_gpio_request,
-			.to_irq			= lpc32xx_gpio_to_irq_p01,
-			.base			= LPC32XX_GPIO_P0_GRP,
-			.ngpio			= LPC32XX_GPIO_P0_MAX,
-			.names			= gpio_p0_names,
-			.can_sleep		= false,
-		},
-		.gpio_grp = &gpio_grp_regs_p0,
-	},
-	{
-		.chip = {
-			.label			= "gpio_p1",
-			.direction_input	= lpc32xx_gpio_dir_input_p012,
-			.get			= lpc32xx_gpio_get_value_p012,
-			.direction_output	= lpc32xx_gpio_dir_output_p012,
-			.set			= lpc32xx_gpio_set_value_p012,
-			.request		= lpc32xx_gpio_request,
-			.to_irq			= lpc32xx_gpio_to_irq_p01,
-			.base			= LPC32XX_GPIO_P1_GRP,
-			.ngpio			= LPC32XX_GPIO_P1_MAX,
-			.names			= gpio_p1_names,
-			.can_sleep		= false,
-		},
-		.gpio_grp = &gpio_grp_regs_p1,
-	},
-	{
-		.chip = {
-			.label			= "gpio_p2",
-			.direction_input	= lpc32xx_gpio_dir_input_p012,
-			.get			= lpc32xx_gpio_get_value_p012,
-			.direction_output	= lpc32xx_gpio_dir_output_p012,
-			.set			= lpc32xx_gpio_set_value_p012,
-			.request		= lpc32xx_gpio_request,
-			.base			= LPC32XX_GPIO_P2_GRP,
-			.ngpio			= LPC32XX_GPIO_P2_MAX,
-			.names			= gpio_p2_names,
-			.can_sleep		= false,
-		},
-		.gpio_grp = &gpio_grp_regs_p2,
-	},
-	{
-		.chip = {
-			.label			= "gpio_p3",
-			.direction_input	= lpc32xx_gpio_dir_input_p3,
-			.get			= lpc32xx_gpio_get_value_p3,
-			.direction_output	= lpc32xx_gpio_dir_output_p3,
-			.set			= lpc32xx_gpio_set_value_p3,
-			.request		= lpc32xx_gpio_request,
-			.to_irq			= lpc32xx_gpio_to_irq_gpio_p3,
-			.base			= LPC32XX_GPIO_P3_GRP,
-			.ngpio			= LPC32XX_GPIO_P3_MAX,
-			.names			= gpio_p3_names,
-			.can_sleep		= false,
-		},
-		.gpio_grp = &gpio_grp_regs_p3,
-	},
-	{
-		.chip = {
-			.label			= "gpi_p3",
-			.direction_input	= lpc32xx_gpio_dir_in_always,
-			.get			= lpc32xx_gpi_get_value,
-			.request		= lpc32xx_gpio_request,
-			.to_irq			= lpc32xx_gpio_to_irq_gpi_p3,
-			.base			= LPC32XX_GPI_P3_GRP,
-			.ngpio			= LPC32XX_GPI_P3_MAX,
-			.names			= gpi_p3_names,
-			.can_sleep		= false,
-		},
-		.gpio_grp = &gpio_grp_regs_p3,
-	},
-	{
-		.chip = {
-			.label			= "gpo_p3",
-			.direction_output	= lpc32xx_gpio_dir_out_always,
-			.set			= lpc32xx_gpo_set_value,
-			.get			= lpc32xx_gpo_get_value,
-			.request		= lpc32xx_gpio_request,
-			.base			= LPC32XX_GPO_P3_GRP,
-			.ngpio			= LPC32XX_GPO_P3_MAX,
-			.names			= gpo_p3_names,
-			.can_sleep		= false,
-		},
-		.gpio_grp = &gpio_grp_regs_p3,
-	},
-};
-
-static int lpc32xx_of_xlate(struct gpio_chip *gc,
-			    const struct of_phandle_args *gpiospec, u32 *flags)
-{
-	/* Is this the correct bank? */
-	u32 bank = gpiospec->args[0];
-	if ((bank >= ARRAY_SIZE(lpc32xx_gpiochip) ||
-	    (gc != &lpc32xx_gpiochip[bank].chip)))
-		return -EINVAL;
-
-	if (flags)
-		*flags = gpiospec->args[2];
-	return gpiospec->args[1];
-}
-
-static int lpc32xx_gpio_probe(struct platform_device *pdev)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(lpc32xx_gpiochip); i++) {
-		if (pdev->dev.of_node) {
-			lpc32xx_gpiochip[i].chip.of_xlate = lpc32xx_of_xlate;
-			lpc32xx_gpiochip[i].chip.of_gpio_n_cells = 3;
-			lpc32xx_gpiochip[i].chip.of_node = pdev->dev.of_node;
-		}
-		gpiochip_add(&lpc32xx_gpiochip[i].chip);
-	}
-
-	return 0;
-}
-
-#ifdef CONFIG_OF
-static const struct of_device_id lpc32xx_gpio_of_match[] = {
-	{ .compatible = "nxp,lpc3220-gpio", },
-	{ },
-};
-#endif
-
-static struct platform_driver lpc32xx_gpio_driver = {
-	.driver		= {
-		.name	= "lpc32xx-gpio",
-		.of_match_table = of_match_ptr(lpc32xx_gpio_of_match),
-	},
-	.probe		= lpc32xx_gpio_probe,
-};
-
-module_platform_driver(lpc32xx_gpio_driver);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] gpio: lpc32xx: add new LPC32xx GPIO controller driver

[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.

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 <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
---
 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

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b18bea0..fce33d3 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -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
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index e3ccd99..f71a770 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -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
diff --git a/drivers/gpio/gpio-lpc32xx.c b/drivers/gpio/gpio-lpc32xx.c
new file mode 100644
index 0000000..99db0f8
--- /dev/null
+++ b/drivers/gpio/gpio-lpc32xx.c
@@ -0,0 +1,660 @@
+/*
+ * NXP LPC32xx GPIO controller driver
+ *
+ * Copyright (C) 2015 Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#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>
+
+#define LPC32XX_GPIO_BANKS		6
+
+#define LPC32XX_GPIO_INPUT		0x00
+#define LPC32XX_GPIO_OUTPUT		0x04
+#define LPC32XX_GPIO_DIR		0x10
+
+#define LPC32XX_GPIO_P2_DIR		0x00
+#define LPC32XX_GPIO_P2_INPUT		0x0C
+#define LPC32XX_GPIO_P2_OUTPUT		0x10
+
+#define LPC32XX_GPIO_CLEAR		0x04
+#define LPC32XX_GPIO_STATE		0x08
+
+#define to_lpc32xx_gpio_chip(c)	container_of(chip, struct lpc32xx_gpio_chip, c)
+
+struct lpc32xx_gpio_chip;
+
+struct lpc32xx_irq_data {
+	struct lpc32xx_gpio_chip *bank;
+	u32 idx;
+	u32 parent;
+	u32 gpio;
+};
+
+struct lpc32xx_gpio_chip {
+	struct gpio_chip chip;
+
+	struct device *dev;
+	void __iomem *base;
+	void __iomem *input;
+	void __iomem *output;
+	void __iomem *dir;
+
+	bool input_only;
+	bool output_only;
+	u32 *output_states;
+	u32 offset;
+	u32 input_mask;
+	u32 interrupt_mask;
+
+	const char **gpio_names;
+
+	u32 nirq;
+	struct resource *irq_res;
+	struct lpc32xx_irq_data *irqs;
+};
+
+static struct lpc32xx_gpio_chip *lpc32xx_gpio_chips[LPC32XX_GPIO_BANKS];
+
+/* 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;
+}
+
+/*
+ * If mask has less or equal than "rel" set bits, i.e.
+ * relative bit is set, find its abosulute bit position
+ */
+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
+ */
+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);
+}
+
+static int lpc32xx_gpio_dir_get(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct lpc32xx_gpio_chip *c = to_lpc32xx_gpio_chip(chip);
+	u32 val;
+
+	if (c->output_only)
+		return GPIOF_DIR_OUT;
+	if (c->input_only)
+		return GPIOF_DIR_IN;
+
+	val = readl(c->dir + LPC32XX_GPIO_STATE);
+	if (val & BIT(gpio + c->offset))
+		return GPIOF_DIR_OUT;
+	else
+		return GPIOF_DIR_IN;
+}
+
+static int lpc32xx_gpio_get(struct gpio_chip *chip, unsigned gpio)
+{
+	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;
+	} else {
+		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;
+	}
+
+	return val & 0x1;
+}
+
+static void lpc32xx_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
+{
+	struct lpc32xx_gpio_chip *c = to_lpc32xx_gpio_chip(chip);
+
+	if (value)
+		writel(BIT(gpio + c->offset), c->output);
+	else
+		writel(BIT(gpio + c->offset), c->output + LPC32XX_GPIO_CLEAR);
+}
+
+static int lpc32xx_gpio_dir_input(struct gpio_chip *chip, unsigned gpio)
+{
+	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);
+
+	return 0;
+}
+
+static int lpc32xx_gpio_dir_output(struct gpio_chip *chip,
+				   unsigned gpio, int value)
+{
+	struct lpc32xx_gpio_chip *c = to_lpc32xx_gpio_chip(chip);
+
+	if (c->input_only)
+		return -EINVAL;
+
+	writel(BIT(gpio + c->offset), c->dir);
+	lpc32xx_gpio_set(chip, gpio, value);
+
+	return 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]);
+		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;
+}
+
+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;
+
+	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;
+	}
+
+	irq_set_irq_type(c->irqs[d->hwirq].parent, type);
+
+	return 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]);
+	if (ret)
+		dev_err(c->dev, "can not request irq %lu: %d\n", d->hwirq, ret);
+
+	return ret;
+}
+
+static void lpc32xx_gpio_irq_shutdown(struct irq_data *d)
+{
+	struct irq_domain *id = d->domain;
+	struct lpc32xx_gpio_chip *c = (struct lpc32xx_gpio_chip *)id->host_data;
+
+	free_irq(c->irqs[d->hwirq].parent, &c->irqs[d->hwirq]);
+}
+
+static int lpc32xx_gpiochip_irq_map(struct irq_domain *d, unsigned int virq,
+				    irq_hw_number_t hwirq)
+{
+	struct lpc32xx_gpio_chip *bank = d->host_data;
+
+	irq_set_chip_and_handler(virq, bank->chip.irqchip, handle_simple_irq);
+	irq_set_noprobe(virq);
+
+	return 0;
+}
+
+static void lpc32xx_gpiochip_irq_unmap(struct irq_domain *d, unsigned int virq)
+{
+	irq_set_chip_and_handler(virq, NULL, NULL);
+}
+
+static const struct irq_domain_ops lpc32xx_gpiochip_domain_ops = {
+	.map	= lpc32xx_gpiochip_irq_map,
+	.unmap	= lpc32xx_gpiochip_irq_unmap,
+	.xlate	= irq_domain_xlate_twocell,
+};
+
+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;
+
+	if (flags)
+		*flags = gpiospec->args[2];
+
+	return gpiospec->args[1];
+}
+
+static int lpc32xx_add_irqchip(struct lpc32xx_gpio_chip *bank,
+			       struct device_node *node)
+{
+	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;
+
+	gpiochip->irqdomain = irq_domain_add_linear(node, bank->nirq,
+					  &lpc32xx_gpiochip_domain_ops, bank);
+	if (!gpiochip->irqdomain) {
+		dev_err(bank->dev, "unable to add irq domain\n");
+		return -EINVAL;
+	}
+
+	gpiochip->irqchip = irqchip;
+	for (idx = 0; idx < bank->nirq; idx++) {
+		virq = irq_create_mapping(gpiochip->irqdomain, idx);
+		if (!virq) {
+			dev_err(bank->dev, "can not create irq mapping\n");
+			goto error;
+		}
+	}
+
+	return 0;
+
+ error:
+	for (idx = 0; idx < bank->nirq; idx++)
+		irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, idx));
+
+	irq_domain_remove(gpiochip->irqdomain);
+	gpiochip->irqdomain = NULL;
+	gpiochip->irqchip = NULL;
+
+	return -EINVAL;
+}
+
+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;
+		}
+
+		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;
+	}
+
+	bank->nirq = nirq;
+
+	return 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;
+
+	gpio_names = devm_kzalloc(bank->dev, ngpio * step, GFP_KERNEL);
+	if (!gpio_names)
+		return -ENOMEM;
+
+	if (bank->input_mask)
+		mask = bank->input_mask;
+	else
+		mask = BIT(ngpio) - 1;
+
+	for (idx = 0; idx < ngpio; val++, mask >>= 1) {
+		if (mask & 0x1) {
+			/*
+			 * In DT and reference manual GPI pins are referenced
+			 * by bit position, give userspace the same pin names,
+			 * however inside GPIO framework these GPI pins follow
+			 * a different (contiguous) enumeration scheme.
+			 */
+			if (bank->input_only)
+				sprintf(gpio_names + idx * step, "%s.%02u",
+					bank->chip.label, val);
+			else
+				sprintf(gpio_names + idx * step, "%s.%02u",
+					bank->chip.label, idx);
+
+			bank->gpio_names[idx] = gpio_names + idx * step;
+			idx++;
+		}
+		if (!mask) {
+			dev_err(bank->dev, "invalid number of GPIOs\n");
+			return -EINVAL;
+		}
+	}
+
+	return 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;
+
+	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;
+	}
+
+	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;
+	}
+
+	of_property_read_u32(node, "gpio-offset", &bank->offset);
+
+	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;
+
+	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;
+	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;
+	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);
+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;
+	}
+
+	return 0;
+
+ error:
+	of_node_put(child);
+	dev_err(&pdev->dev, "can not register gpio chip: %d\n", ret);
+
+	lpc32xx_gpio_remove(pdev);
+
+	return ret;
+}
+
+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;
+
+		if (lpc32xx_gpio_chips[idx]->chip.irqchip) {
+			irqd = lpc32xx_gpio_chips[idx]->chip.irqdomain;
+
+			for (i = 0; i < lpc32xx_gpio_chips[idx]->nirq; i++)
+				irq_dispose_mapping(irq_find_mapping(irqd, i));
+
+			irq_domain_remove(irqd);
+			lpc32xx_gpio_chips[idx]->chip.irqdomain = NULL;
+			lpc32xx_gpio_chips[idx]->chip.irqchip = NULL;
+		}
+
+		gpiochip_remove(&lpc32xx_gpio_chips[idx]->chip);
+		lpc32xx_gpio_chips[idx] = NULL;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id lpc32xx_gpio_of_match[] = {
+	{ .compatible = "nxp,lpc3220-gpio", },
+	{ },
+};
+
+static struct platform_driver lpc32xx_gpio_driver = {
+	.probe		= lpc32xx_gpio_probe,
+	.remove		= lpc32xx_gpio_remove,
+	.driver		= {
+		.name	= "lpc32xx-gpio",
+		.of_match_table = of_match_ptr(lpc32xx_gpio_of_match),
+	},
+};
+module_platform_driver(lpc32xx_gpio_driver);
+
+MODULE_AUTHOR("Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("GPIO driver for LPC32xx");
+MODULE_LICENSE("GPL");
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] gpio: lpc32xx: add new LPC32xx GPIO controller driver

[Linus Walleij]

On Fri, Nov 20, 2015 at 2:29 AM, Vladimir Zapolskiy <vz@mleia.com> wrote:

> 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.
>
> 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 <vz@mleia.com>
(...)

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 use
GPIOLIB_IRQCHIP which would be a big win.

> +#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?

> +#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.

> +#define to_lpc32xx_gpio_chip(c)        container_of(chip, struct lpc32xx_gpio_chip, c)

Rewrite this into a static inline function. Defines are ugly.

> +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?

> +struct lpc32xx_gpio_chip {
> +       struct gpio_chip chip;
> +
> +       struct device *dev;
> +       void __iomem *base;
> +       void __iomem *input;
> +       void __iomem *output;
> +       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?

> +       u32 nirq;
> +       struct resource *irq_res;
> +       struct lpc32xx_irq_data *irqs;
> +};

Kerneldoc this struct so I understand it, too.

> +
> +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...

> +/* 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);

> +/*
> + * If mask has less or equal than "rel" set bits, i.e.
> + * relative bit is set, find its abosulute bit position

Spelling.

> + */
> +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
> + */
> +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.

> +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.

> +{
> +       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));

> +       } else {

Just skip the else clause and un-indent. If we're here, it is input.

> +               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));

> +       }
> +
> +       return val & 0x1;
> +}

(...)
> +static int lpc32xx_gpio_dir_input(struct gpio_chip *chip, unsigned gpio)

Rename "gpio" to "offset"

> +{
> +       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.

> +static int lpc32xx_gpio_dir_output(struct gpio_chip *chip,
> +                                  unsigned gpio, int value)

Rename "gpio" to "offset".

> +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]);
> +               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.

> +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;
> +
> +       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.

> +       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.

> +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?

> +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);

> +
> +       if (flags)
> +               *flags = gpiospec->args[2];
> +
> +       return gpiospec->args[1];
> +}

Then you don't need to reimplement this, right?

> +static int lpc32xx_add_irqchip(struct lpc32xx_gpio_chip *bank,
> +                              struct device_node *node)

Please try to use GPIOLIB_IRQCHIP instead. Please.

> +{
> +       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.

> +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;
> +               }
> +
> +               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.

> +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.

> +static int of_node_to_gpio(struct device *dev, struct device_node *node)
> +{
> +       int ret, id;
> +       struct lpc32xx_gpio_chip *bank;
> +       u32 ngpio = 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.

> +       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!)

> +       of_property_read_u32(node, "gpio-offset", &bank->offset);

What is this exactly?

> +
> +       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;
> +
> +       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.

> +       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.

> +       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);
> +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.

> +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

Re: [PATCH 0/4] gpio: lpc32xx: add new LPC32xx GPIO controller driver

[Linus Walleij]

On Fri, Nov 20, 2015 at 2:29 AM, Vladimir Zapolskiy <vz@mleia.com> wrote:

> 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

Re: [PATCH 0/4] gpio: lpc32xx: add new LPC32xx GPIO controller driver

[Vladimir Zapolskiy]

Hi Linus,

On 30.11.2015 10:54, Linus Walleij wrote:
> On Fri, Nov 20, 2015 at 2:29 AM, Vladimir Zapolskiy <vz@mleia.com> wrote:
> 
>> 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