[PATCH 0/5] Introduce STMFX I2C GPIO expander

[PATCH 0/5] Introduce STMFX I2C GPIO expander

[Amelie Delaunay]

This series adds support for STMicroelectronics Multi-Function eXpander
(STMFX) GPIO expander, used on some STM32 discovery and evaluation boards.

STMFX is an STM32L152 based I2C slave controller, whose firmware embeds an
I/O expansion feature, offering 24 GPIOs.

STMFX pinctrl/GPIO driver provides a GPIO interface supporting inputs and
outputs, and a pinctrl interface supporting push-pull and open-drain
configuration.
STMFX GPIO expander can also be used as interrupt controller.

Previous series [1], based on MFD and GPIO frameworks, is abandoned and
completely reworked.

[1] https://lkml.org/lkml/2018/2/8/300

Amelie Delaunay (5):
  dt-bindings: pinctrl: document the STMFX pinctrl bindings
  pinctrl: Add STMFX GPIO expander Pinctrl/GPIO driver
  ARM: dts: stm32: add STMFX pinctrl/gpio expander support on
    stm32746g-eval
  ARM: dts: stm32: add orange and blue leds on stm32746g-eval
  ARM: dts: stm32: add joystick support on stm32746g-eval

 .../devicetree/bindings/pinctrl/pinctrl-stmfx.txt  | 118 +++
 arch/arm/boot/dts/stm32746g-eval.dts               |  66 ++
 drivers/pinctrl/Kconfig                            |  13 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-stmfx.c                    | 985 +++++++++++++++++++++
 5 files changed, 1183 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt
 create mode 100644 drivers/pinctrl/pinctrl-stmfx.c

-- 
2.7.4

[PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Amelie Delaunay]

This patch adds documentation of device tree bindings for the
STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 .../devicetree/bindings/pinctrl/pinctrl-stmfx.txt  | 118 +++++++++++++++++++++
 1 file changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt
new file mode 100644
index 0000000..4d8941de
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt
@@ -0,0 +1,118 @@
+STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander bindings
+
+ST Multi-Function eXpander (STMFX) is a slave controller using I2C for
+communication with the main MCU. It offers up to 24 GPIOs expansion.
+
+Required properties:
+- compatible: should be "st,stmfx-0300".
+- reg: I2C slave address of the device.
+- interrupt-parent: phandle of the STMFX parent interrupt controller.
+- interrutps: interrupt specifier triggered by MFX_IRQ_OUT signal.
+
+Optional property:
+- drive-open-drain: configure MFX_IRQ_OUT as open drain.
+- vdd-supply: phandle of the regulator supplying STMFX.
+
+Required properties for gpio controller sub-node:
+- #gpio-cells: should be <2>, the first cell is the GPIO number and the second
+  cell is the gpio flags in accordance with <dt-bindings/gpio/gpio.h>.
+- gpio-controller: marks the device as a GPIO controller.
+Please refer to ../gpio/gpio.txt.
+
+Optional properties for gpio controller sub-node:
+- #interrupt-cells: should be <2>, the first cell is the GPIO number and the
+  second cell is the interrupt flags in accordance with
+  <dt-bindings/interrupt-controller/irq.h>.
+- interrupt-controller: marks the device as an interrupt controller.
+
+Please refer to pinctrl-bindings.txt for pin configuration.
+
+Required properties for pin configuration sub-nodes:
+- pins: list of pins to which the configuration applies.
+
+Optional properties for pin configuration sub-nodes (pinconf-generic ones):
+- bias-disable: disable any bias on the pin.
+- bias-pull-up: the pin will be pulled up.
+- bias-pull-pin-default: use the pin-default pull state.
+- bias-pull-down: the pin will be pulled down.
+- drive-open-drain: the pin will be driven with open drain.
+- drive-push-pull: the pin will be driven actively high and low.
+- output-high: the pin will be configured as an output driving high level.
+- output-low: the pin will be configured as an output driving low level.
+
+Note that STMFX pins[15:0] are called "gpio[15:0]", and STMFX pins[23:16] are
+called "agpio[7:0]". Example, to refer to pin 18 of STMFX, use "agpio2".
+
+Example:
+
+	stmfxpinctrl: stmfx@42 {
+		compatible = "st,stmfx-0300";
+		reg = <0x42>;
+		interrupt-parent = <&gpioi>;
+		interrupts = <8 IRQ_TYPE_EDGE_RISING>;
+		vdd-supply = <&v3v3>;
+		status = "okay";
+
+		stmfxgpio: gpio {
+			#gpio-cells = <2>;
+			#interrupt-cells = <2>;
+			gpio-controller;
+			interrupt-controller;
+			status = "okay";
+		};
+
+		joystick_pins: joystick@0 {
+			pins = "gpio0", "gpio1", "gpio2", "gpio3", "gpio4";
+			drive-push-pull;
+			bias-pull-down;
+		};
+	};
+
+	joystick {
+		compatible = "gpio-keys";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		pinctrl-0 = <&joystick_pins>;
+		pinctrl-names = "default";
+		button@0 {
+			label = "JoySel";
+			linux,code = <KEY_ENTER>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <0 IRQ_TYPE_EDGE_RISING>;
+		};
+		button@1 {
+			label = "JoyDown";
+			linux,code = <KEY_DOWN>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <1 IRQ_TYPE_EDGE_RISING>;
+		};
+		button@2 {
+			label = "JoyLeft";
+			linux,code = <KEY_LEFT>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <2 IRQ_TYPE_EDGE_RISING>;
+		};
+		button@3 {
+			label = "JoyRight";
+			linux,code = <KEY_RIGHT>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <3 IRQ_TYPE_EDGE_RISING>;
+		};
+		button@4 {
+			label = "JoyUp";
+			linux,code = <KEY_UP>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <4 IRQ_TYPE_EDGE_RISING>;
+		};
+	};
+
+	leds {
+		compatible = "gpio-leds";
+		orange {
+			gpios = <&stmfxgpio 17 1>;
+		};
+
+		blue {
+			gpios = <&stmfxgpio 19 1>;
+		};
+	}
-- 
2.7.4

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Rob Herring]

On Wed, Apr 11, 2018 at 11:47:01AM +0200, Amelie Delaunay wrote:
> This patch adds documentation of device tree bindings for the
> STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander.
> 
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> ---
>  .../devicetree/bindings/pinctrl/pinctrl-stmfx.txt  | 118 +++++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt
> new file mode 100644
> index 0000000..4d8941de
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt
> @@ -0,0 +1,118 @@
> +STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander bindings
> +
> +ST Multi-Function eXpander (STMFX) is a slave controller using I2C for
> +communication with the main MCU. It offers up to 24 GPIOs expansion.
> +
> +Required properties:
> +- compatible: should be "st,stmfx-0300".
> +- reg: I2C slave address of the device.
> +- interrupt-parent: phandle of the STMFX parent interrupt controller.
> +- interrutps: interrupt specifier triggered by MFX_IRQ_OUT signal.
> +
> +Optional property:

s/property/properties/

> +- drive-open-drain: configure MFX_IRQ_OUT as open drain.
> +- vdd-supply: phandle of the regulator supplying STMFX.
> +
> +Required properties for gpio controller sub-node:
> +- #gpio-cells: should be <2>, the first cell is the GPIO number and the second
> +  cell is the gpio flags in accordance with <dt-bindings/gpio/gpio.h>.
> +- gpio-controller: marks the device as a GPIO controller.
> +Please refer to ../gpio/gpio.txt.
> +
> +Optional properties for gpio controller sub-node:
> +- #interrupt-cells: should be <2>, the first cell is the GPIO number and the
> +  second cell is the interrupt flags in accordance with
> +  <dt-bindings/interrupt-controller/irq.h>.
> +- interrupt-controller: marks the device as an interrupt controller.
> +
> +Please refer to pinctrl-bindings.txt for pin configuration.
> +
> +Required properties for pin configuration sub-nodes:
> +- pins: list of pins to which the configuration applies.
> +
> +Optional properties for pin configuration sub-nodes (pinconf-generic ones):
> +- bias-disable: disable any bias on the pin.
> +- bias-pull-up: the pin will be pulled up.
> +- bias-pull-pin-default: use the pin-default pull state.
> +- bias-pull-down: the pin will be pulled down.
> +- drive-open-drain: the pin will be driven with open drain.
> +- drive-push-pull: the pin will be driven actively high and low.
> +- output-high: the pin will be configured as an output driving high level.
> +- output-low: the pin will be configured as an output driving low level.
> +
> +Note that STMFX pins[15:0] are called "gpio[15:0]", and STMFX pins[23:16] are
> +called "agpio[7:0]". Example, to refer to pin 18 of STMFX, use "agpio2".
> +
> +Example:
> +
> +	stmfxpinctrl: stmfx@42 {
> +		compatible = "st,stmfx-0300";
> +		reg = <0x42>;
> +		interrupt-parent = <&gpioi>;
> +		interrupts = <8 IRQ_TYPE_EDGE_RISING>;
> +		vdd-supply = <&v3v3>;
> +		status = "okay";

Don't show status in examples.

> +
> +		stmfxgpio: gpio {

Why does this need to be a sub node? Are there functions beyond GPIO?

> +			#gpio-cells = <2>;
> +			#interrupt-cells = <2>;
> +			gpio-controller;
> +			interrupt-controller;
> +			status = "okay";
> +		};
> +
> +		joystick_pins: joystick@0 {

A unit-address without a reg prop is not valid.

> +			pins = "gpio0", "gpio1", "gpio2", "gpio3", "gpio4";
> +			drive-push-pull;
> +			bias-pull-down;
> +		};
> +	};
> +
> +	joystick {
> +		compatible = "gpio-keys";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-0 = <&joystick_pins>;
> +		pinctrl-names = "default";
> +		button@0 {
> +			label = "JoySel";
> +			linux,code = <KEY_ENTER>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <0 IRQ_TYPE_EDGE_RISING>;
> +		};
> +		button@1 {
> +			label = "JoyDown";
> +			linux,code = <KEY_DOWN>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <1 IRQ_TYPE_EDGE_RISING>;
> +		};
> +		button@2 {
> +			label = "JoyLeft";
> +			linux,code = <KEY_LEFT>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <2 IRQ_TYPE_EDGE_RISING>;
> +		};
> +		button@3 {
> +			label = "JoyRight";
> +			linux,code = <KEY_RIGHT>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <3 IRQ_TYPE_EDGE_RISING>;
> +		};
> +		button@4 {
> +			label = "JoyUp";
> +			linux,code = <KEY_UP>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +		};
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		orange {
> +			gpios = <&stmfxgpio 17 1>;
> +		};
> +
> +		blue {
> +			gpios = <&stmfxgpio 19 1>;
> +		};
> +	}
> -- 
> 2.7.4
> 

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Linus Walleij]

On Mon, Apr 16, 2018 at 8:19 PM, Rob Herring <robh@kernel.org> wrote:
> On Wed, Apr 11, 2018 at 11:47:01AM +0200, Amelie Delaunay wrote:
>> This patch adds documentation of device tree bindings for the
>> STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>

>> +     stmfxpinctrl: stmfx@42 {
>> +             compatible = "st,stmfx-0300";
>> +             reg = <0x42>;
>> +             interrupt-parent = <&gpioi>;
>> +             interrupts = <8 IRQ_TYPE_EDGE_RISING>;
>> +             vdd-supply = <&v3v3>;
>> +             status = "okay";
>
> Don't show status in examples.
>
>> +
>> +             stmfxgpio: gpio {
>
> Why does this need to be a sub node? Are there functions beyond GPIO?

Amelie can answer to whether there are, I suspect there
are and in the review of patch 2 I suggest a MFD parent
and parent/child spawning of a MFD child for the GPIO
and pin control device.

Yours,
Linus Walleij

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Amelie DELAUNAY]

On 04/26/2018 02:49 PM, Linus Walleij wrote:
> On Mon, Apr 16, 2018 at 8:19 PM, Rob Herring <robh@kernel.org> wrote:
>> On Wed, Apr 11, 2018 at 11:47:01AM +0200, Amelie Delaunay wrote:
>>> This patch adds documentation of device tree bindings for the
>>> STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander.
>>>
>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> 
>>> +     stmfxpinctrl: stmfx@42 {
>>> +             compatible = "st,stmfx-0300";
>>> +             reg = <0x42>;
>>> +             interrupt-parent = <&gpioi>;
>>> +             interrupts = <8 IRQ_TYPE_EDGE_RISING>;
>>> +             vdd-supply = <&v3v3>;
>>> +             status = "okay";
>>
>> Don't show status in examples.
>>
>>> +
>>> +             stmfxgpio: gpio {
>>
>> Why does this need to be a sub node? Are there functions beyond GPIO?
> 
> Amelie can answer to whether there are, I suspect there
> are and in the review of patch 2 I suggest a MFD parent
> and parent/child spawning of a MFD child for the GPIO
> and pin control device.
> 
> Yours,
> Linus Walleij
> 

Indeed, stmfx has other functions than GPIO. But, after comments done 
here: [1] and there: [2], it has been decided to move MFD parent/GPIO 
child drivers into a single PINCTRL/GPIO driver because of the following 
reasons:
- Other stmfx functions (IDD measurement and TouchScreen controller) are 
not used on any of the boards using an stmfx and supported by Linux, so 
no way to test these functions, and no need to maintain them while they 
are not being used.
- But, in the case a new board will use more than GPIO function on 
stmfx, the actual implementation allow to easily extract common init 
part of stmfx and put it in an MFD driver.

So I could remove gpio sub-node and put its contents in stmfx node and 
keep single PINCTRL/GPIO driver for the time being.
Please advise,

Thanks,
Amelie

[1] https://lkml.org/lkml/2018/2/12/265
[2] https://lkml.org/lkml/2018/2/21/1503

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Linus Walleij]

On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:

> Indeed, stmfx has other functions than GPIO. But, after comments done
> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
> child drivers into a single PINCTRL/GPIO driver because of the following
> reasons:
> - Other stmfx functions (IDD measurement and TouchScreen controller) are
> not used on any of the boards using an stmfx and supported by Linux, so
> no way to test these functions, and no need to maintain them while they
> are not being used.
> - But, in the case a new board will use more than GPIO function on
> stmfx, the actual implementation allow to easily extract common init
> part of stmfx and put it in an MFD driver.
>
> So I could remove gpio sub-node and put its contents in stmfx node and
> keep single PINCTRL/GPIO driver for the time being.
> Please advise,

I would normally advice to use the right modeling from the start, create
the MFD driver and spawn the devices from there. It is confusing
if the layout of the driver(s) doesn't really match the layout of the
hardware.

I understand that it is a pain to write new MFD drivers to get your
things going and it would be "nice to get this working really quick
now" but in my experience it is better to do it right from the start.

Yours,
Linus Walleij

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Amelie DELAUNAY]

On 05/16/2018 04:20 PM, Linus Walleij wrote:
> On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
> 
>> Indeed, stmfx has other functions than GPIO. But, after comments done
>> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
>> child drivers into a single PINCTRL/GPIO driver because of the following
>> reasons:
>> - Other stmfx functions (IDD measurement and TouchScreen controller) are
>> not used on any of the boards using an stmfx and supported by Linux, so
>> no way to test these functions, and no need to maintain them while they
>> are not being used.
>> - But, in the case a new board will use more than GPIO function on
>> stmfx, the actual implementation allow to easily extract common init
>> part of stmfx and put it in an MFD driver.
>>
>> So I could remove gpio sub-node and put its contents in stmfx node and
>> keep single PINCTRL/GPIO driver for the time being.
>> Please advise,
> 
> I would normally advice to use the right modeling from the start, create
> the MFD driver and spawn the devices from there. It is confusing
> if the layout of the driver(s) doesn't really match the layout of the
> hardware.
> 
> I understand that it is a pain to write new MFD drivers to get your
> things going and it would be "nice to get this working really quick
> now" but in my experience it is better to do it right from the start.
> 

Hi Linus,

Thanks for your advice. I understand the point.
So, the right modeling would be to:
- create an MFD driver with the common init part of stmfx
- remove all common init part of stmfx-pinctrl driver and keep only all 
gpio/pinctrl functions.

I will not develop the other stmfx functions (IDD measurement driver and 
TouchScreen controller driver) because, as explained ealier, they are 
not used on any of the boards using an stmfx and supported by Linux, so 
no way to test these functions, and no need to maintain them while they 
are not being used.

Lee, are you OK with that ?

Regards,
Amelie

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Lee Jones]

On Wed, 16 May 2018, Amelie DELAUNAY wrote:

> 
> 
> On 05/16/2018 04:20 PM, Linus Walleij wrote:
> > On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
> > 
> >> Indeed, stmfx has other functions than GPIO. But, after comments done
> >> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
> >> child drivers into a single PINCTRL/GPIO driver because of the following
> >> reasons:
> >> - Other stmfx functions (IDD measurement and TouchScreen controller) are
> >> not used on any of the boards using an stmfx and supported by Linux, so
> >> no way to test these functions, and no need to maintain them while they
> >> are not being used.
> >> - But, in the case a new board will use more than GPIO function on
> >> stmfx, the actual implementation allow to easily extract common init
> >> part of stmfx and put it in an MFD driver.
> >>
> >> So I could remove gpio sub-node and put its contents in stmfx node and
> >> keep single PINCTRL/GPIO driver for the time being.
> >> Please advise,
> > 
> > I would normally advice to use the right modeling from the start, create
> > the MFD driver and spawn the devices from there. It is confusing
> > if the layout of the driver(s) doesn't really match the layout of the
> > hardware.
> > 
> > I understand that it is a pain to write new MFD drivers to get your
> > things going and it would be "nice to get this working really quick
> > now" but in my experience it is better to do it right from the start.
> > 
> 
> Hi Linus,
> 
> Thanks for your advice. I understand the point.
> So, the right modeling would be to:
> - create an MFD driver with the common init part of stmfx
> - remove all common init part of stmfx-pinctrl driver and keep only all 
> gpio/pinctrl functions.
> 
> I will not develop the other stmfx functions (IDD measurement driver and 
> TouchScreen controller driver) because, as explained ealier, they are 
> not used on any of the boards using an stmfx and supported by Linux, so 
> no way to test these functions, and no need to maintain them while they 
> are not being used.
> 
> Lee, are you OK with that ?

I missed a lot of this conversation I think, but from what I've read,
it sounds fine.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Amelie DELAUNAY]

On 05/17/2018 08:36 AM, Lee Jones wrote:
> On Wed, 16 May 2018, Amelie DELAUNAY wrote:
> 
>>
>>
>> On 05/16/2018 04:20 PM, Linus Walleij wrote:
>>> On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>>>
>>>> Indeed, stmfx has other functions than GPIO. But, after comments done
>>>> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
>>>> child drivers into a single PINCTRL/GPIO driver because of the following
>>>> reasons:
>>>> - Other stmfx functions (IDD measurement and TouchScreen controller) are
>>>> not used on any of the boards using an stmfx and supported by Linux, so
>>>> no way to test these functions, and no need to maintain them while they
>>>> are not being used.
>>>> - But, in the case a new board will use more than GPIO function on
>>>> stmfx, the actual implementation allow to easily extract common init
>>>> part of stmfx and put it in an MFD driver.
>>>>
>>>> So I could remove gpio sub-node and put its contents in stmfx node and
>>>> keep single PINCTRL/GPIO driver for the time being.
>>>> Please advise,
>>>
>>> I would normally advice to use the right modeling from the start, create
>>> the MFD driver and spawn the devices from there. It is confusing
>>> if the layout of the driver(s) doesn't really match the layout of the
>>> hardware.
>>>
>>> I understand that it is a pain to write new MFD drivers to get your
>>> things going and it would be "nice to get this working really quick
>>> now" but in my experience it is better to do it right from the start.
>>>
>>
>> Hi Linus,
>>
>> Thanks for your advice. I understand the point.
>> So, the right modeling would be to:
>> - create an MFD driver with the common init part of stmfx
>> - remove all common init part of stmfx-pinctrl driver and keep only all
>> gpio/pinctrl functions.
>>
>> I will not develop the other stmfx functions (IDD measurement driver and
>> TouchScreen controller driver) because, as explained ealier, they are
>> not used on any of the boards using an stmfx and supported by Linux, so
>> no way to test these functions, and no need to maintain them while they
>> are not being used.
>>
>> Lee, are you OK with that ?
> 
> I missed a lot of this conversation I think, but from what I've read,
> it sounds fine.
> 

I summarize the situation:
- I still don't have an official datasheet for STMFX device which could 
justify the use of an MFD driver;
- the MFD driver will contain the STMFX chip initialization stuff such 
as regmap initialization (regmap structure will be shared with the 
child), chip initialization, global interrupt management;
- there will be only one child (GPIO/PINCTRL node) for the time being.

So, is "MFD driver + GPIO/PINCTRL driver" the right modeling, and does 
it still sound fine after this summary ? :)

Thanks,
Amelie

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Lee Jones]

On Fri, 18 May 2018, Amelie DELAUNAY wrote:

> On 05/17/2018 08:36 AM, Lee Jones wrote:
> > On Wed, 16 May 2018, Amelie DELAUNAY wrote:
> > 
> >>
> >>
> >> On 05/16/2018 04:20 PM, Linus Walleij wrote:
> >>> On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
> >>>
> >>>> Indeed, stmfx has other functions than GPIO. But, after comments done
> >>>> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
> >>>> child drivers into a single PINCTRL/GPIO driver because of the following
> >>>> reasons:
> >>>> - Other stmfx functions (IDD measurement and TouchScreen controller) are
> >>>> not used on any of the boards using an stmfx and supported by Linux, so
> >>>> no way to test these functions, and no need to maintain them while they
> >>>> are not being used.
> >>>> - But, in the case a new board will use more than GPIO function on
> >>>> stmfx, the actual implementation allow to easily extract common init
> >>>> part of stmfx and put it in an MFD driver.
> >>>>
> >>>> So I could remove gpio sub-node and put its contents in stmfx node and
> >>>> keep single PINCTRL/GPIO driver for the time being.
> >>>> Please advise,
> >>>
> >>> I would normally advice to use the right modeling from the start, create
> >>> the MFD driver and spawn the devices from there. It is confusing
> >>> if the layout of the driver(s) doesn't really match the layout of the
> >>> hardware.
> >>>
> >>> I understand that it is a pain to write new MFD drivers to get your
> >>> things going and it would be "nice to get this working really quick
> >>> now" but in my experience it is better to do it right from the start.
> >>>
> >>
> >> Hi Linus,
> >>
> >> Thanks for your advice. I understand the point.
> >> So, the right modeling would be to:
> >> - create an MFD driver with the common init part of stmfx
> >> - remove all common init part of stmfx-pinctrl driver and keep only all
> >> gpio/pinctrl functions.
> >>
> >> I will not develop the other stmfx functions (IDD measurement driver and
> >> TouchScreen controller driver) because, as explained ealier, they are
> >> not used on any of the boards using an stmfx and supported by Linux, so
> >> no way to test these functions, and no need to maintain them while they
> >> are not being used.
> >>
> >> Lee, are you OK with that ?
> > 
> > I missed a lot of this conversation I think, but from what I've read,
> > it sounds fine.
> > 
> 
> I summarize the situation:
> - I still don't have an official datasheet for STMFX device which could 
> justify the use of an MFD driver;
> - the MFD driver will contain the STMFX chip initialization stuff such 
> as regmap initialization (regmap structure will be shared with the 
> child), chip initialization, global interrupt management;
> - there will be only one child (GPIO/PINCTRL node) for the time being.
> 
> So, is "MFD driver + GPIO/PINCTRL driver" the right modeling, and does 
> it still sound fine after this summary ? :)

It is starting to sound like there will only ever be one child device,
which starts to cross the line into "this is not an MFD" (M = Multi)
territory.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Amelie DELAUNAY]

On 05/18/2018 03:52 PM, Lee Jones wrote:
> On Fri, 18 May 2018, Amelie DELAUNAY wrote:
> 
>> On 05/17/2018 08:36 AM, Lee Jones wrote:
>>> On Wed, 16 May 2018, Amelie DELAUNAY wrote:
>>>
>>>>
>>>>
>>>> On 05/16/2018 04:20 PM, Linus Walleij wrote:
>>>>> On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>>>>>
>>>>>> Indeed, stmfx has other functions than GPIO. But, after comments done
>>>>>> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
>>>>>> child drivers into a single PINCTRL/GPIO driver because of the following
>>>>>> reasons:
>>>>>> - Other stmfx functions (IDD measurement and TouchScreen controller) are
>>>>>> not used on any of the boards using an stmfx and supported by Linux, so
>>>>>> no way to test these functions, and no need to maintain them while they
>>>>>> are not being used.
>>>>>> - But, in the case a new board will use more than GPIO function on
>>>>>> stmfx, the actual implementation allow to easily extract common init
>>>>>> part of stmfx and put it in an MFD driver.
>>>>>>
>>>>>> So I could remove gpio sub-node and put its contents in stmfx node and
>>>>>> keep single PINCTRL/GPIO driver for the time being.
>>>>>> Please advise,
>>>>>
>>>>> I would normally advice to use the right modeling from the start, create
>>>>> the MFD driver and spawn the devices from there. It is confusing
>>>>> if the layout of the driver(s) doesn't really match the layout of the
>>>>> hardware.
>>>>>
>>>>> I understand that it is a pain to write new MFD drivers to get your
>>>>> things going and it would be "nice to get this working really quick
>>>>> now" but in my experience it is better to do it right from the start.
>>>>>
>>>>
>>>> Hi Linus,
>>>>
>>>> Thanks for your advice. I understand the point.
>>>> So, the right modeling would be to:
>>>> - create an MFD driver with the common init part of stmfx
>>>> - remove all common init part of stmfx-pinctrl driver and keep only all
>>>> gpio/pinctrl functions.
>>>>
>>>> I will not develop the other stmfx functions (IDD measurement driver and
>>>> TouchScreen controller driver) because, as explained ealier, they are
>>>> not used on any of the boards using an stmfx and supported by Linux, so
>>>> no way to test these functions, and no need to maintain them while they
>>>> are not being used.
>>>>
>>>> Lee, are you OK with that ?
>>>
>>> I missed a lot of this conversation I think, but from what I've read,
>>> it sounds fine.
>>>
>>
>> I summarize the situation:
>> - I still don't have an official datasheet for STMFX device which could
>> justify the use of an MFD driver;
>> - the MFD driver will contain the STMFX chip initialization stuff such
>> as regmap initialization (regmap structure will be shared with the
>> child), chip initialization, global interrupt management;
>> - there will be only one child (GPIO/PINCTRL node) for the time being.
>>
>> So, is "MFD driver + GPIO/PINCTRL driver" the right modeling, and does
>> it still sound fine after this summary ? :)
> 
> It is starting to sound like there will only ever be one child device,
> which starts to cross the line into "this is not an MFD" (M = Multi)
> territory.
> 

... for the time being. So, Linus, Lee, is it possible to find common 
ground ?

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Linus Walleij]

On Fri, May 18, 2018 at 9:29 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
> On 05/17/2018 08:36 AM, Lee Jones wrote:
>> On Wed, 16 May 2018, Amelie DELAUNAY wrote:
>>> On 05/16/2018 04:20 PM, Linus Walleij wrote:
>>>> On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>>>>
>>>>> Indeed, stmfx has other functions than GPIO. But, after comments done
>>>>> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
>>>>> child drivers into a single PINCTRL/GPIO driver because of the following
>>>>> reasons:
>>>>> - Other stmfx functions (IDD measurement and TouchScreen controller) are
>>>>> not used on any of the boards using an stmfx and supported by Linux, so
>>>>> no way to test these functions, and no need to maintain them while they
>>>>> are not being used.
>>>>> - But, in the case a new board will use more than GPIO function on
>>>>> stmfx, the actual implementation allow to easily extract common init
>>>>> part of stmfx and put it in an MFD driver.
>>>>>
>>>>> So I could remove gpio sub-node and put its contents in stmfx node and
>>>>> keep single PINCTRL/GPIO driver for the time being.
>>>>> Please advise,
>>>>
>>>> I would normally advice to use the right modeling from the start, create
>>>> the MFD driver and spawn the devices from there. It is confusing
>>>> if the layout of the driver(s) doesn't really match the layout of the
>>>> hardware.
>>>>
>>>> I understand that it is a pain to write new MFD drivers to get your
>>>> things going and it would be "nice to get this working really quick
>>>> now" but in my experience it is better to do it right from the start.
>>>>
>>>
>>> Hi Linus,
>>>
>>> Thanks for your advice. I understand the point.
>>> So, the right modeling would be to:
>>> - create an MFD driver with the common init part of stmfx
>>> - remove all common init part of stmfx-pinctrl driver and keep only all
>>> gpio/pinctrl functions.
>>>
>>> I will not develop the other stmfx functions (IDD measurement driver and
>>> TouchScreen controller driver) because, as explained ealier, they are
>>> not used on any of the boards using an stmfx and supported by Linux, so
>>> no way to test these functions, and no need to maintain them while they
>>> are not being used.
>>>
>>> Lee, are you OK with that ?
>>
>> I missed a lot of this conversation I think, but from what I've read,
>> it sounds fine.
>>
>
> I summarize the situation:
> - I still don't have an official datasheet for STMFX device which could
> justify the use of an MFD driver;
> - the MFD driver will contain the STMFX chip initialization stuff such
> as regmap initialization (regmap structure will be shared with the
> child), chip initialization, global interrupt management;
> - there will be only one child (GPIO/PINCTRL node) for the time being.

But there will be more devices in it. And they will invariably be put
to use later, and there will be new versions of the chip as well, and
then you will be happy about doing the MFD core, which makes it
easy to add new variants with different subdevices.

> So, is "MFD driver + GPIO/PINCTRL driver" the right modeling, and does
> it still sound fine after this summary ? :)

No I think it should use an MFD core.

Mainly because of device tree concerns.

The main reason is that the device tree bindings will be different if
you add an MFD core later, the GPIO and pinctrl driver will
move to a child node, making old device trees incompatible.

We could have a single driver in GPIO+pin control if it is a child
of an MFD node in the device tree, but it doesn't make much
sense as the I2C device need to be probing to the MFD core.

Yours,
Linus Walleij

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Amelie DELAUNAY]

On 05/24/2018 09:13 AM, Linus Walleij wrote:
> On Fri, May 18, 2018 at 9:29 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>> On 05/17/2018 08:36 AM, Lee Jones wrote:
>>> On Wed, 16 May 2018, Amelie DELAUNAY wrote:
>>>> On 05/16/2018 04:20 PM, Linus Walleij wrote:
>>>>> On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>>>>>
>>>>>> Indeed, stmfx has other functions than GPIO. But, after comments done
>>>>>> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
>>>>>> child drivers into a single PINCTRL/GPIO driver because of the following
>>>>>> reasons:
>>>>>> - Other stmfx functions (IDD measurement and TouchScreen controller) are
>>>>>> not used on any of the boards using an stmfx and supported by Linux, so
>>>>>> no way to test these functions, and no need to maintain them while they
>>>>>> are not being used.
>>>>>> - But, in the case a new board will use more than GPIO function on
>>>>>> stmfx, the actual implementation allow to easily extract common init
>>>>>> part of stmfx and put it in an MFD driver.
>>>>>>
>>>>>> So I could remove gpio sub-node and put its contents in stmfx node and
>>>>>> keep single PINCTRL/GPIO driver for the time being.
>>>>>> Please advise,
>>>>>
>>>>> I would normally advice to use the right modeling from the start, create
>>>>> the MFD driver and spawn the devices from there. It is confusing
>>>>> if the layout of the driver(s) doesn't really match the layout of the
>>>>> hardware.
>>>>>
>>>>> I understand that it is a pain to write new MFD drivers to get your
>>>>> things going and it would be "nice to get this working really quick
>>>>> now" but in my experience it is better to do it right from the start.
>>>>>
>>>>
>>>> Hi Linus,
>>>>
>>>> Thanks for your advice. I understand the point.
>>>> So, the right modeling would be to:
>>>> - create an MFD driver with the common init part of stmfx
>>>> - remove all common init part of stmfx-pinctrl driver and keep only all
>>>> gpio/pinctrl functions.
>>>>
>>>> I will not develop the other stmfx functions (IDD measurement driver and
>>>> TouchScreen controller driver) because, as explained ealier, they are
>>>> not used on any of the boards using an stmfx and supported by Linux, so
>>>> no way to test these functions, and no need to maintain them while they
>>>> are not being used.
>>>>
>>>> Lee, are you OK with that ?
>>>
>>> I missed a lot of this conversation I think, but from what I've read,
>>> it sounds fine.
>>>
>>
>> I summarize the situation:
>> - I still don't have an official datasheet for STMFX device which could
>> justify the use of an MFD driver;
>> - the MFD driver will contain the STMFX chip initialization stuff such
>> as regmap initialization (regmap structure will be shared with the
>> child), chip initialization, global interrupt management;
>> - there will be only one child (GPIO/PINCTRL node) for the time being.
> 
> But there will be more devices in it. And they will invariably be put
> to use later, and there will be new versions of the chip as well, and
> then you will be happy about doing the MFD core, which makes it
> easy to add new variants with different subdevices.
> 
>> So, is "MFD driver + GPIO/PINCTRL driver" the right modeling, and does
>> it still sound fine after this summary ? :)
> 
> No I think it should use an MFD core.
> 
> Mainly because of device tree concerns.
> 
> The main reason is that the device tree bindings will be different if
> you add an MFD core later, the GPIO and pinctrl driver will
> move to a child node, making old device trees incompatible.
> 
> We could have a single driver in GPIO+pin control if it is a child
> of an MFD node in the device tree, but it doesn't make much
> sense as the I2C device need to be probing to the MFD core.
> 

I agree with you Linus, and that's why all STMFX chip initialization 
stuff was decorrelated in pinctrl-stmfx. This shows that this stuff 
needs to be in an MFD core.

But as there is only one child for now (due to the reasons mentioned 
earlier), it can suggest that it is not a Multi-Function Device.

I'm not able to target when IDD or TS functions will be required on a 
Linux product, but it still makes sense to consider that these functions 
will be used on a Linux product.

So, I think MFD core + GPIO/pinctrl driver is the right modeling, but I 
wanted to be sure that this is okay for everyone. I don't want to spend 
time on something that will not be accepted due to its modeling.

Regards,
Amelie

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Lee Jones]

On Mon, 28 May 2018, Amelie DELAUNAY wrote:

> On 05/24/2018 09:13 AM, Linus Walleij wrote:
> > On Fri, May 18, 2018 at 9:29 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
> >> On 05/17/2018 08:36 AM, Lee Jones wrote:
> >>> On Wed, 16 May 2018, Amelie DELAUNAY wrote:
> >>>> On 05/16/2018 04:20 PM, Linus Walleij wrote:
> >>>>> On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
> >>>>>
> >>>>>> Indeed, stmfx has other functions than GPIO. But, after comments done
> >>>>>> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
> >>>>>> child drivers into a single PINCTRL/GPIO driver because of the following
> >>>>>> reasons:
> >>>>>> - Other stmfx functions (IDD measurement and TouchScreen controller) are
> >>>>>> not used on any of the boards using an stmfx and supported by Linux, so
> >>>>>> no way to test these functions, and no need to maintain them while they
> >>>>>> are not being used.
> >>>>>> - But, in the case a new board will use more than GPIO function on
> >>>>>> stmfx, the actual implementation allow to easily extract common init
> >>>>>> part of stmfx and put it in an MFD driver.
> >>>>>>
> >>>>>> So I could remove gpio sub-node and put its contents in stmfx node and
> >>>>>> keep single PINCTRL/GPIO driver for the time being.
> >>>>>> Please advise,
> >>>>>
> >>>>> I would normally advice to use the right modeling from the start, create
> >>>>> the MFD driver and spawn the devices from there. It is confusing
> >>>>> if the layout of the driver(s) doesn't really match the layout of the
> >>>>> hardware.
> >>>>>
> >>>>> I understand that it is a pain to write new MFD drivers to get your
> >>>>> things going and it would be "nice to get this working really quick
> >>>>> now" but in my experience it is better to do it right from the start.
> >>>>>
> >>>>
> >>>> Hi Linus,
> >>>>
> >>>> Thanks for your advice. I understand the point.
> >>>> So, the right modeling would be to:
> >>>> - create an MFD driver with the common init part of stmfx
> >>>> - remove all common init part of stmfx-pinctrl driver and keep only all
> >>>> gpio/pinctrl functions.
> >>>>
> >>>> I will not develop the other stmfx functions (IDD measurement driver and
> >>>> TouchScreen controller driver) because, as explained ealier, they are
> >>>> not used on any of the boards using an stmfx and supported by Linux, so
> >>>> no way to test these functions, and no need to maintain them while they
> >>>> are not being used.
> >>>>
> >>>> Lee, are you OK with that ?
> >>>
> >>> I missed a lot of this conversation I think, but from what I've read,
> >>> it sounds fine.
> >>>
> >>
> >> I summarize the situation:
> >> - I still don't have an official datasheet for STMFX device which could
> >> justify the use of an MFD driver;
> >> - the MFD driver will contain the STMFX chip initialization stuff such
> >> as regmap initialization (regmap structure will be shared with the
> >> child), chip initialization, global interrupt management;
> >> - there will be only one child (GPIO/PINCTRL node) for the time being.
> > 
> > But there will be more devices in it. And they will invariably be put
> > to use later, and there will be new versions of the chip as well, and
> > then you will be happy about doing the MFD core, which makes it
> > easy to add new variants with different subdevices.
> > 
> >> So, is "MFD driver + GPIO/PINCTRL driver" the right modeling, and does
> >> it still sound fine after this summary ? :)
> > 
> > No I think it should use an MFD core.
> > 
> > Mainly because of device tree concerns.
> > 
> > The main reason is that the device tree bindings will be different if
> > you add an MFD core later, the GPIO and pinctrl driver will
> > move to a child node, making old device trees incompatible.
> > 
> > We could have a single driver in GPIO+pin control if it is a child
> > of an MFD node in the device tree, but it doesn't make much
> > sense as the I2C device need to be probing to the MFD core.
> > 
> 
> I agree with you Linus, and that's why all STMFX chip initialization 
> stuff was decorrelated in pinctrl-stmfx. This shows that this stuff 
> needs to be in an MFD core.
> 
> But as there is only one child for now (due to the reasons mentioned 
> earlier), it can suggest that it is not a Multi-Function Device.
> 
> I'm not able to target when IDD or TS functions will be required on a 
> Linux product, but it still makes sense to consider that these functions 
> will be used on a Linux product.
> 
> So, I think MFD core + GPIO/pinctrl driver is the right modeling, but I 
> wanted to be sure that this is okay for everyone. I don't want to spend 
> time on something that will not be accepted due to its modeling.

It's fine.  Go ahead.

Thanks for seeing this through to a reasonable conclusion.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/5] dt-bindings: pinctrl: document the STMFX pinctrl bindings

[Amelie DELAUNAY]

On 04/16/2018 08:19 PM, Rob Herring wrote:
> On Wed, Apr 11, 2018 at 11:47:01AM +0200, Amelie Delaunay wrote:
>> This patch adds documentation of device tree bindings for the
>> STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>> ---
>>   .../devicetree/bindings/pinctrl/pinctrl-stmfx.txt  | 118 +++++++++++++++++++++
>>   1 file changed, 118 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt
>> new file mode 100644
>> index 0000000..4d8941de
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-stmfx.txt
>> @@ -0,0 +1,118 @@
>> +STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander bindings
>> +
>> +ST Multi-Function eXpander (STMFX) is a slave controller using I2C for
>> +communication with the main MCU. It offers up to 24 GPIOs expansion.
>> +
>> +Required properties:
>> +- compatible: should be "st,stmfx-0300".
>> +- reg: I2C slave address of the device.
>> +- interrupt-parent: phandle of the STMFX parent interrupt controller.
>> +- interrutps: interrupt specifier triggered by MFX_IRQ_OUT signal.
>> +
>> +Optional property:
> 
> s/property/properties/
> 
>> +- drive-open-drain: configure MFX_IRQ_OUT as open drain.
>> +- vdd-supply: phandle of the regulator supplying STMFX.
>> +
>> +Required properties for gpio controller sub-node:
>> +- #gpio-cells: should be <2>, the first cell is the GPIO number and the second
>> +  cell is the gpio flags in accordance with <dt-bindings/gpio/gpio.h>.
>> +- gpio-controller: marks the device as a GPIO controller.
>> +Please refer to ../gpio/gpio.txt.
>> +
>> +Optional properties for gpio controller sub-node:
>> +- #interrupt-cells: should be <2>, the first cell is the GPIO number and the
>> +  second cell is the interrupt flags in accordance with
>> +  <dt-bindings/interrupt-controller/irq.h>.
>> +- interrupt-controller: marks the device as an interrupt controller.
>> +
>> +Please refer to pinctrl-bindings.txt for pin configuration.
>> +
>> +Required properties for pin configuration sub-nodes:
>> +- pins: list of pins to which the configuration applies.
>> +
>> +Optional properties for pin configuration sub-nodes (pinconf-generic ones):
>> +- bias-disable: disable any bias on the pin.
>> +- bias-pull-up: the pin will be pulled up.
>> +- bias-pull-pin-default: use the pin-default pull state.
>> +- bias-pull-down: the pin will be pulled down.
>> +- drive-open-drain: the pin will be driven with open drain.
>> +- drive-push-pull: the pin will be driven actively high and low.
>> +- output-high: the pin will be configured as an output driving high level.
>> +- output-low: the pin will be configured as an output driving low level.
>> +
>> +Note that STMFX pins[15:0] are called "gpio[15:0]", and STMFX pins[23:16] are
>> +called "agpio[7:0]". Example, to refer to pin 18 of STMFX, use "agpio2".
>> +
>> +Example:
>> +
>> +	stmfxpinctrl: stmfx@42 {
>> +		compatible = "st,stmfx-0300";
>> +		reg = <0x42>;
>> +		interrupt-parent = <&gpioi>;
>> +		interrupts = <8 IRQ_TYPE_EDGE_RISING>;
>> +		vdd-supply = <&v3v3>;
>> +		status = "okay";
> 
> Don't show status in examples.
> 

I'll fix it.

>> +
>> +		stmfxgpio: gpio {
> 
> Why does this need to be a sub node? Are there functions beyond GPIO?
> 

I'll address this point in my response to Linus.

>> +			#gpio-cells = <2>;
>> +			#interrupt-cells = <2>;
>> +			gpio-controller;
>> +			interrupt-controller;
>> +			status = "okay";
>> +		};
>> +
>> +		joystick_pins: joystick@0 {
> 
> A unit-address without a reg prop is not valid.
> 

OK, I'll replace it by 'joystick_pins: joystick-0'.

>> +			pins = "gpio0", "gpio1", "gpio2", "gpio3", "gpio4";
>> +			drive-push-pull;
>> +			bias-pull-down;
>> +		};
>> +	};
>> +
>> +	joystick {
>> +		compatible = "gpio-keys";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		pinctrl-0 = <&joystick_pins>;
>> +		pinctrl-names = "default";
>> +		button@0 {
>> +			label = "JoySel";
>> +			linux,code = <KEY_ENTER>;
>> +			interrupt-parent = <&stmfxgpio>;
>> +			interrupts = <0 IRQ_TYPE_EDGE_RISING>;
>> +		};
>> +		button@1 {
>> +			label = "JoyDown";
>> +			linux,code = <KEY_DOWN>;
>> +			interrupt-parent = <&stmfxgpio>;
>> +			interrupts = <1 IRQ_TYPE_EDGE_RISING>;
>> +		};
>> +		button@2 {
>> +			label = "JoyLeft";
>> +			linux,code = <KEY_LEFT>;
>> +			interrupt-parent = <&stmfxgpio>;
>> +			interrupts = <2 IRQ_TYPE_EDGE_RISING>;
>> +		};
>> +		button@3 {
>> +			label = "JoyRight";
>> +			linux,code = <KEY_RIGHT>;
>> +			interrupt-parent = <&stmfxgpio>;
>> +			interrupts = <3 IRQ_TYPE_EDGE_RISING>;
>> +		};
>> +		button@4 {
>> +			label = "JoyUp";
>> +			linux,code = <KEY_UP>;
>> +			interrupt-parent = <&stmfxgpio>;
>> +			interrupts = <4 IRQ_TYPE_EDGE_RISING>;
>> +		};
>> +	};
>> +
>> +	leds {
>> +		compatible = "gpio-leds";
>> +		orange {
>> +			gpios = <&stmfxgpio 17 1>;
>> +		};
>> +
>> +		blue {
>> +			gpios = <&stmfxgpio 19 1>;
>> +		};
>> +	}
>> -- 
>> 2.7.4
>>

Thanks,
Amelie

[PATCH 2/5] pinctrl: Add STMFX GPIO expander Pinctrl/GPIO driver

[Amelie Delaunay]

This patch adds pinctrl/GPIO driver for STMicroelectronics
Multi-Function eXpander (STMFX) GPIO expander.
STMFX is an I2C slave controller, offering up to 24 GPIOs.
The driver relies on generic pin config interface to configure the GPIOs.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 drivers/pinctrl/Kconfig         |  13 +
 drivers/pinctrl/Makefile        |   1 +
 drivers/pinctrl/pinctrl-stmfx.c | 985 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 999 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-stmfx.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 0f254b3..4ab07aa 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -263,6 +263,19 @@ config PINCTRL_ST
 	select PINCONF
 	select GPIOLIB_IRQCHIP
 
+config PINCTRL_STMFX
+	tristate "STMicroelectronics STMFX I2C GPIO expander pinctrl driver"
+	depends on GPIOLIB && I2C=y
+	select GENERIC_PINCONF
+	select GPIOLIB_IRQCHIP
+	select REGMAP_I2C
+	help
+	  I2C driver for STMicroelectronics Multi-Function eXpander (STMFX)
+	  GPIO expander.
+	  This provides a GPIO interface supporting inputs and outputs,
+	  and configuring push-pull, open-drain, and can also be used as
+	  interrupt-controller.
+
 config PINCTRL_TZ1090
 	bool "Toumaz Xenif TZ1090 pin control driver"
 	depends on SOC_TZ1090
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index d369263..271c464 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_PINCTRL_LANTIQ)	+= pinctrl-lantiq.o
 obj-$(CONFIG_PINCTRL_LPC18XX)	+= pinctrl-lpc18xx.o
 obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
 obj-$(CONFIG_PINCTRL_ST) 	+= pinctrl-st.o
+obj-$(CONFIG_PINCTRL_STMFX) 	+= pinctrl-stmfx.o
 obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 obj-$(CONFIG_PINCTRL_INGENIC)	+= pinctrl-ingenic.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
new file mode 100644
index 0000000..d59a7bc
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-stmfx.c
@@ -0,0 +1,985 @@
+// SPDX-Licence-Identifier: GPL-2.0
+/*
+ * Driver for STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander
+ *
+ * Copyright (C) 2018 STMicroelectronics
+ * Author(s): Amelie Delaunay <amelie.delaunay@st.com>.
+ */
+#include <linux/bitfield.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include "core.h"
+#include "pinctrl-utils.h"
+
+/* General */
+#define STMFX_REG_CHIP_ID		0x00 /* R */
+#define STMFX_REG_FW_VERSION_MSB	0x01 /* R */
+#define STMFX_REG_FW_VERSION_LSB	0x02 /* R */
+#define STMFX_REG_SYS_CTRL		0x40 /* RW */
+/* IRQ output management */
+#define STMFX_REG_IRQ_OUT_PIN		0x41 /* RW */
+#define STMFX_REG_IRQ_SRC_EN		0x42 /* RW */
+#define STMFX_REG_IRQ_PENDING		0x08 /* R */
+#define STMFX_REG_IRQ_ACK		0x44 /* RW */
+
+/* MFX boot time is around 10ms, so after reset, we have to wait this delay */
+#define STMFX_BOOT_TIME_MS 10
+
+/* GPIOs expander */
+/* GPIO_STATE1 0x10, GPIO_STATE2 0x11, GPIO_STATE3 0x12 */
+#define STMFX_REG_GPIO_STATE		0x10 /* R */
+/* GPIO_DIR1 0x60, GPIO_DIR2 0x61, GPIO_DIR3 0x63 */
+#define STMFX_REG_GPIO_DIR		0x60 /* RW */
+/* GPIO_TYPE1 0x64, GPIO_TYPE2 0x65, GPIO_TYPE3 0x66 */
+#define STMFX_REG_GPIO_TYPE		0x64 /* RW */
+/* GPIO_PUPD1 0x68, GPIO_PUPD2 0x69, GPIO_PUPD3 0x6A */
+#define STMFX_REG_GPIO_PUPD		0x68 /* RW */
+/* GPO_SET1 0x6C, GPO_SET2 0x6D, GPO_SET3 0x6E */
+#define STMFX_REG_GPO_SET		0x6C /* RW */
+/* GPO_CLR1 0x70, GPO_CLR2 0x71, GPO_CLR3 0x72 */
+#define STMFX_REG_GPO_CLR		0x70 /* RW */
+/* IRQ_GPI_SRC1 0x48, IRQ_GPI_SRC2 0x49, IRQ_GPI_SRC3 0x4A */
+#define STMFX_REG_IRQ_GPI_SRC		0x48 /* RW */
+/* IRQ_GPI_EVT1 0x4C, IRQ_GPI_EVT2 0x4D, IRQ_GPI_EVT3 0x4E */
+#define STMFX_REG_IRQ_GPI_EVT		0x4C /* RW */
+/* IRQ_GPI_TYPE1 0x50, IRQ_GPI_TYPE2 0x51, IRQ_GPI_TYPE3 0x52 */
+#define STMFX_REG_IRQ_GPI_TYPE		0x50 /* RW */
+/* IRQ_GPI_PENDING1 0x0C, IRQ_GPI_PENDING2 0x0D, IRQ_GPI_PENDING3 0x0E*/
+#define STMFX_REG_IRQ_GPI_PENDING	0x0C /* R */
+/* IRQ_GPI_ACK1 0x54, IRQ_GPI_ACK2 0x55, IRQ_GPI_ACK3 0x56 */
+#define STMFX_REG_IRQ_GPI_ACK		0x54 /* RW */
+
+/* STMFX_REG_CHIP_ID bitfields */
+#define STMFX_REG_CHIP_ID_MASK		GENMASK(7, 0)
+
+/* STMFX_REG_SYS_CTRL bitfields */
+#define STMFX_REG_SYS_CTRL_GPIO_EN	BIT(0)
+#define STMFX_REG_SYS_CTRL_ALTGPIO_EN	BIT(3)
+#define STMFX_REG_SYS_CTRL_SWRST	BIT(7)
+
+/* STMFX_REG_IRQ_OUT_PIN bitfields */
+#define STMFX_REG_IRQ_OUT_PIN_TYPE	BIT(0) /* 0-OD 1-PP */
+#define STMFX_REG_IRQ_OUT_PIN_POL	BIT(1) /* 0-active LOW 1-active HIGH */
+
+/* STMFX_REG_IRQ_SRC_EN bitfields */
+#define STMFX_REG_IRQ_SRC_EN_GPIO	BIT(0)
+
+/* STMFX_REG_IRQ_PENDING bitfields */
+#define STMFX_REG_IRQ_PENDING_GPIO	BIT(0)
+
+#define NR_GPIO_REGS			3
+#define NR_GPIOS_PER_REG		8
+#define get_reg(offset)			((offset) / NR_GPIOS_PER_REG)
+#define get_shift(offset)		((offset) % NR_GPIOS_PER_REG)
+#define get_mask(offset)		(BIT(get_shift(offset)))
+
+static const struct pinctrl_pin_desc stmfx_pins[] = {
+	PINCTRL_PIN(0, "gpio0"),
+	PINCTRL_PIN(1, "gpio1"),
+	PINCTRL_PIN(2, "gpio2"),
+	PINCTRL_PIN(3, "gpio3"),
+	PINCTRL_PIN(4, "gpio4"),
+	PINCTRL_PIN(5, "gpio5"),
+	PINCTRL_PIN(6, "gpio6"),
+	PINCTRL_PIN(7, "gpio7"),
+	PINCTRL_PIN(8, "gpio8"),
+	PINCTRL_PIN(9, "gpio9"),
+	PINCTRL_PIN(10, "gpio10"),
+	PINCTRL_PIN(11, "gpio11"),
+	PINCTRL_PIN(12, "gpio12"),
+	PINCTRL_PIN(13, "gpio13"),
+	PINCTRL_PIN(14, "gpio14"),
+	PINCTRL_PIN(15, "gpio15"),
+	PINCTRL_PIN(16, "agpio0"),
+	PINCTRL_PIN(17, "agpio1"),
+	PINCTRL_PIN(18, "agpio2"),
+	PINCTRL_PIN(19, "agpio3"),
+	PINCTRL_PIN(20, "agpio4"),
+	PINCTRL_PIN(21, "agpio5"),
+	PINCTRL_PIN(22, "agpio6"),
+	PINCTRL_PIN(23, "agpio7"),
+};
+
+struct stmfx {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regulator *vdd;
+	struct stmfx_pinctrl *pctl;
+	int irq;
+};
+
+struct stmfx_pinctrl {
+	struct device *dev;
+	struct regmap *regmap;
+	struct pinctrl_dev *pctl_dev;
+	struct pinctrl_desc pctl_desc;
+	struct gpio_chip gpio_chip;
+	struct irq_chip irq_chip;
+	struct mutex lock; /* IRQ bus lock */
+	u8 irq_gpi_src[NR_GPIO_REGS];
+	u8 irq_gpi_type[NR_GPIO_REGS];
+	u8 irq_gpi_evt[NR_GPIO_REGS];
+	u8 irq_toggle_edge[NR_GPIO_REGS];
+#ifdef CONFIG_PM
+	u8 bkp_gpio_state[NR_GPIO_REGS];
+	u8 bkp_gpio_dir[NR_GPIO_REGS];
+	u8 bkp_gpio_type[NR_GPIO_REGS];
+	u8 bkp_gpio_pupd[NR_GPIO_REGS];
+#endif
+};
+
+static int stmfx_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gc);
+	u32 reg = STMFX_REG_GPIO_STATE + get_reg(offset);
+	u32 mask = get_mask(offset);
+	u32 value;
+	int ret;
+
+	ret = regmap_read(pctl->regmap, reg, &value);
+
+	return ret ? ret : !!(value & mask);
+}
+
+static void stmfx_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gc);
+	u32 reg = value ? STMFX_REG_GPO_SET : STMFX_REG_GPO_CLR;
+	u32 mask = get_mask(offset);
+
+	regmap_write_bits(pctl->regmap, reg + get_reg(offset), mask, mask);
+}
+
+static int stmfx_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gc);
+	u32 reg = STMFX_REG_GPIO_DIR + get_reg(offset);
+	u32 mask = get_mask(offset);
+	u32 val;
+	int ret;
+
+	ret = regmap_read(pctl->regmap, reg, &val);
+	/*
+	 * On stmfx, gpio pins direction is (0)input, (1)output.
+	 * .get_direction returns 0=out, 1=in
+	 */
+
+	return ret ? ret : !(val & mask);
+}
+
+static int stmfx_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gc);
+	u32 reg = STMFX_REG_GPIO_DIR + get_reg(offset);
+	u32 mask = get_mask(offset);
+
+	return regmap_write_bits(pctl->regmap, reg, mask, 0);
+}
+
+static int stmfx_gpio_direction_output(struct gpio_chip *gc,
+				       unsigned int offset, int value)
+{
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gc);
+	u32 reg = STMFX_REG_GPIO_DIR + get_reg(offset);
+	u32 mask = get_mask(offset);
+
+	stmfx_gpio_set(gc, offset, value);
+
+	return regmap_write_bits(pctl->regmap, reg, mask, mask);
+}
+
+static int stmfx_pinconf_get_pupd(struct stmfx_pinctrl *pctl,
+				  unsigned int offset)
+{
+	u32 reg = STMFX_REG_GPIO_PUPD + get_reg(offset);
+	u32 pupd;
+	int ret;
+
+	ret = regmap_read(pctl->regmap, reg, &pupd);
+	if (ret)
+		return ret;
+
+	return (pupd & get_mask(offset));
+}
+
+static int stmfx_pinconf_set_pupd(struct stmfx_pinctrl *pctl,
+				  unsigned int offset, u32 pupd)
+{
+	u32 reg = STMFX_REG_GPIO_PUPD + get_reg(offset);
+	u32 mask = get_mask(offset);
+
+	return regmap_write_bits(pctl->regmap, reg, mask, pupd ? mask : 0);
+}
+
+static int stmfx_pinconf_get_type(struct stmfx_pinctrl *pctl,
+				  unsigned int offset)
+{
+	u32 reg = STMFX_REG_GPIO_TYPE + get_reg(offset);
+	u32 type;
+	int ret;
+
+	ret = regmap_read(pctl->regmap, reg, &type);
+	if (ret)
+		return 0;
+
+	return (type & get_mask(offset));
+}
+
+static int stmfx_pinconf_set_type(struct stmfx_pinctrl *pctl,
+				  unsigned int offset, u32 type)
+{
+	u32 reg = STMFX_REG_GPIO_TYPE + get_reg(offset);
+	u32 mask = get_mask(offset);
+
+	return regmap_write_bits(pctl->regmap, reg, mask, type ? mask : 0);
+}
+
+static int stmfx_pinconf_get(struct pinctrl_dev *pctldev,
+			     unsigned int pin, unsigned long *config)
+{
+	struct stmfx_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	u32 param = pinconf_to_config_param(*config);
+	u32 dir, type, pupd;
+	u32 arg = 0;
+	int ret;
+
+	dir = stmfx_gpio_get_direction(&pctl->gpio_chip, pin);
+	if (dir < 0)
+		return dir;
+	type = stmfx_pinconf_get_type(pctl, pin);
+	if (type < 0)
+		return type;
+	pupd = stmfx_pinconf_get_pupd(pctl, pin);
+	if (pupd < 0)
+		return pupd;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (dir == GPIOF_DIR_OUT) {
+			if (type & pupd)
+				arg = 0;
+			else
+				arg = 1;
+		} else {
+			if (!type)
+				arg = 1;
+		}
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (dir == GPIOF_DIR_IN && type && !pupd)
+			arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (type && pupd)
+			arg = 1;
+		break;
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		if ((dir == GPIOF_DIR_OUT && type) ||
+		    (dir == GPIOF_DIR_IN && !type))
+			arg = 1;
+		break;
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		if ((dir == GPIOF_DIR_OUT && !type) ||
+		    (dir == GPIOF_DIR_IN && type))
+			arg = 1;
+		break;
+	case PIN_CONFIG_OUTPUT:
+		if (dir == GPIOF_DIR_IN)
+			return -EINVAL;
+
+		ret = stmfx_gpio_get(&pctl->gpio_chip, pin);
+		if (ret < 0)
+			return ret;
+
+		arg = ret;
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int stmfx_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			     unsigned long *configs, unsigned int num_configs)
+{
+	struct stmfx_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param;
+	u32 arg;
+	int dir, i, ret;
+
+	dir = stmfx_gpio_get_direction(&pctl->gpio_chip, pin);
+	if (dir < 0)
+		return dir;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+		case PIN_CONFIG_BIAS_DISABLE:
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			ret = stmfx_pinconf_set_pupd(pctl, pin, 0);
+			if (ret)
+				return ret;
+			break;
+		case PIN_CONFIG_BIAS_PULL_UP:
+			ret = stmfx_pinconf_set_pupd(pctl, pin, 1);
+			if (ret)
+				return ret;
+			break;
+		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+			if (dir == GPIOF_DIR_OUT)
+				ret = stmfx_pinconf_set_type(pctl, pin, 1);
+			else
+				ret = stmfx_pinconf_set_type(pctl, pin, 0);
+			if (ret)
+				return ret;
+			break;
+		case PIN_CONFIG_DRIVE_PUSH_PULL:
+			if (dir == GPIOF_DIR_OUT)
+				ret = stmfx_pinconf_set_type(pctl, pin, 0);
+			else
+				ret = stmfx_pinconf_set_type(pctl, pin, 1);
+			if (ret)
+				return ret;
+			break;
+		case PIN_CONFIG_OUTPUT:
+			ret = stmfx_gpio_direction_output(&pctl->gpio_chip,
+							  pin, arg);
+			if (ret)
+				return ret;
+			break;
+		default:
+			return -ENOTSUPP;
+		}
+	}
+
+	return 0;
+}
+
+static void stmfx_pinconf_dbg_show(struct pinctrl_dev *pctldev,
+				   struct seq_file *s, unsigned int offset)
+{
+	struct stmfx_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
+	int dir, type, pupd, val;
+
+	dir = stmfx_gpio_get_direction(&pctl->gpio_chip, offset);
+	if (dir < 0)
+		return;
+	type = stmfx_pinconf_get_type(pctl, offset);
+	if (type < 0)
+		return;
+	pupd = stmfx_pinconf_get_pupd(pctl, offset);
+	if (pupd < 0)
+		return;
+	val = stmfx_gpio_get(&pctl->gpio_chip, offset);
+	if (val < 0)
+		return;
+
+	if (dir == GPIOF_DIR_IN) {
+		seq_printf(s, "input %s ", val ? "high" : "low");
+		if (type)
+			seq_printf(s, "with internal pull-%s ",
+				   pupd ? "up" : "down");
+		else
+			seq_printf(s, "%s ", pupd ? "floating" : "analog");
+	} else {
+		seq_printf(s, "output %s ", val ? "high" : "low");
+		if (type)
+			seq_printf(s, "open drain %s internal pull-up ",
+				   pupd ? "with" : "without");
+		else
+			seq_puts(s, "push pull no pull ");
+	}
+}
+
+static const struct pinconf_ops stmfx_pinconf_ops = {
+	.pin_config_get		= stmfx_pinconf_get,
+	.pin_config_set		= stmfx_pinconf_set,
+	.pin_config_dbg_show	= stmfx_pinconf_dbg_show,
+};
+
+static int stmfx_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	return 0;
+}
+
+static const char *stmfx_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+						unsigned int selector)
+{
+	return NULL;
+}
+
+static int stmfx_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+					unsigned int selector,
+					const unsigned int **pins,
+					unsigned int *num_pins)
+{
+	return -ENOTSUPP;
+}
+
+static const struct pinctrl_ops stmfx_pinctrl_ops = {
+	.get_groups_count = stmfx_pinctrl_get_groups_count,
+	.get_group_name = stmfx_pinctrl_get_group_name,
+	.get_group_pins = stmfx_pinctrl_get_group_pins,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static void stmfx_gpio_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data);
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip);
+	u32 reg = get_reg(data->hwirq);
+	u32 mask = get_mask(data->hwirq);
+
+	pctl->irq_gpi_src[reg] &= ~mask;
+}
+
+static void stmfx_gpio_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data);
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip);
+	u32 reg = get_reg(data->hwirq);
+	u32 mask = get_mask(data->hwirq);
+
+	pctl->irq_gpi_src[reg] |= mask;
+}
+
+static int stmfx_gpio_irq_set_type(struct irq_data *data,
+				   unsigned int type)
+{
+	struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data);
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip);
+	u32 reg = get_reg(data->hwirq);
+	u32 mask = get_mask(data->hwirq);
+
+	if (type & IRQ_TYPE_NONE)
+		return -EINVAL;
+
+	if (type & IRQ_TYPE_EDGE_BOTH) {
+		pctl->irq_gpi_evt[reg] |= mask;
+		irq_set_handler_locked(data, handle_edge_irq);
+	} else {
+		pctl->irq_gpi_evt[reg] &= ~mask;
+		irq_set_handler_locked(data, handle_level_irq);
+	}
+
+	if ((type & IRQ_TYPE_EDGE_RISING) || (type & IRQ_TYPE_LEVEL_HIGH))
+		pctl->irq_gpi_type[reg] |= mask;
+	else
+		pctl->irq_gpi_type[reg] &= ~mask;
+
+	/*
+	 * In case of (type & IRQ_TYPE_EDGE_BOTH), we need to know current
+	 * GPIO value to set the right edge trigger. But in atomic context
+	 * here we can't access registers over I2C. That's why (type &
+	 * IRQ_TYPE_EDGE_BOTH) will be managed in .irq_sync_unlock.
+	 */
+
+	if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH)
+		pctl->irq_toggle_edge[reg] |= mask;
+	else
+		pctl->irq_toggle_edge[reg] &= mask;
+
+	return 0;
+}
+
+static void stmfx_gpio_irq_bus_lock(struct irq_data *data)
+{
+	struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data);
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip);
+
+	mutex_lock(&pctl->lock);
+}
+
+static void stmfx_gpio_irq_bus_sync_unlock(struct irq_data *data)
+{
+	struct gpio_chip *gpio_chip = irq_data_get_irq_chip_data(data);
+	struct stmfx_pinctrl *pctl = gpiochip_get_data(gpio_chip);
+	u32 reg = get_reg(data->hwirq);
+	u32 mask = get_mask(data->hwirq);
+
+	/*
+	 * In case of IRQ_TYPE_EDGE_BOTH), read the current GPIO value
+	 * (this couldn't be done in .irq_set_type because of atomic context)
+	 * to set the right irq trigger type.
+	 */
+	if (pctl->irq_toggle_edge[reg] & mask) {
+		if (stmfx_gpio_get(gpio_chip, data->hwirq))
+			pctl->irq_gpi_type[reg] &= ~mask;
+		else
+			pctl->irq_gpi_type[reg] |= mask;
+	}
+
+	regmap_bulk_write(pctl->regmap, STMFX_REG_IRQ_GPI_EVT,
+			  pctl->irq_gpi_evt, NR_GPIO_REGS);
+	regmap_bulk_write(pctl->regmap, STMFX_REG_IRQ_GPI_TYPE,
+			  pctl->irq_gpi_type, NR_GPIO_REGS);
+	regmap_bulk_write(pctl->regmap, STMFX_REG_IRQ_GPI_SRC,
+			  pctl->irq_gpi_src, NR_GPIO_REGS);
+
+	/*
+	 * If at least one GPIO irq is enabled among the 24 available,
+	 * enable global GPIO irq
+	 */
+	if (*(u32 *)pctl->irq_gpi_src & GENMASK(23, 0))
+		regmap_write_bits(pctl->regmap, STMFX_REG_IRQ_SRC_EN,
+				  STMFX_REG_IRQ_SRC_EN_GPIO,
+				  STMFX_REG_IRQ_SRC_EN_GPIO);
+	else
+		regmap_write_bits(pctl->regmap, STMFX_REG_IRQ_SRC_EN,
+				  STMFX_REG_IRQ_SRC_EN_GPIO, 0);
+
+	mutex_unlock(&pctl->lock);
+}
+
+static void stmfx_gpio_irq_toggle_trigger(struct stmfx_pinctrl *pctl,
+					  unsigned int offset)
+{
+	u32 reg = get_reg(offset);
+	u32 mask = get_mask(offset);
+	int val;
+
+	if (!(pctl->irq_toggle_edge[reg] & mask))
+		return;
+
+	val = stmfx_gpio_get(&pctl->gpio_chip, offset);
+	if (val < 0)
+		return;
+
+	if (val) {
+		pctl->irq_gpi_type[reg] &= mask;
+		regmap_write_bits(pctl->regmap, STMFX_REG_IRQ_GPI_TYPE + reg,
+				  get_mask(offset), 0);
+
+	} else {
+		pctl->irq_gpi_type[reg] |= mask;
+		regmap_write_bits(pctl->regmap, STMFX_REG_IRQ_GPI_TYPE + reg,
+				  get_mask(offset), get_mask(offset));
+	}
+}
+
+static irqreturn_t stmfx_gpio_irq_thread_fn(int irq, void *dev_id)
+{
+	struct stmfx_pinctrl *pctl = (struct stmfx_pinctrl *)dev_id;
+	struct gpio_chip *gc = &pctl->gpio_chip;
+	u8 pending[NR_GPIO_REGS];
+	unsigned long n, status;
+	int ret;
+
+	ret = regmap_bulk_read(pctl->regmap, STMFX_REG_IRQ_GPI_PENDING,
+			       &pending, NR_GPIO_REGS);
+	if (ret)
+		return IRQ_NONE;
+
+	ret = regmap_bulk_write(pctl->regmap, STMFX_REG_IRQ_GPI_ACK,
+				pending, NR_GPIO_REGS);
+	if (ret)
+		return IRQ_NONE;
+
+	status = *(unsigned long *)pending;
+	for_each_set_bit(n, &status, gc->ngpio) {
+		handle_nested_irq(irq_find_mapping(gc->irq.domain, n));
+		stmfx_gpio_irq_toggle_trigger(pctl, n);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int stmfx_gpio_irq_init(struct stmfx_pinctrl *pctl, int irq)
+{
+	u32 irqtrigger;
+	int ret;
+
+	irqtrigger = irq_get_trigger_type(irq);
+	ret = devm_request_threaded_irq(pctl->dev, irq, NULL,
+					stmfx_gpio_irq_thread_fn,
+					irqtrigger | IRQF_ONESHOT | IRQF_SHARED,
+					pctl->irq_chip.name, pctl);
+	if (ret) {
+		dev_err(pctl->dev, "cannot request irq%d\n", irq);
+		return ret;
+	}
+
+	pctl->irq_chip.name = "stmfxgpio";
+	pctl->irq_chip.irq_mask = stmfx_gpio_irq_mask;
+	pctl->irq_chip.irq_unmask = stmfx_gpio_irq_unmask;
+	pctl->irq_chip.irq_set_type = stmfx_gpio_irq_set_type;
+	pctl->irq_chip.irq_bus_lock = stmfx_gpio_irq_bus_lock;
+	pctl->irq_chip.irq_bus_sync_unlock = stmfx_gpio_irq_bus_sync_unlock;
+
+	ret = gpiochip_irqchip_add_nested(&pctl->gpio_chip, &pctl->irq_chip,
+					  0, handle_simple_irq, IRQ_TYPE_NONE);
+	if (ret) {
+		dev_err(pctl->dev, "cannot add irqchip to gpiochip\n");
+		return ret;
+	}
+
+	gpiochip_set_nested_irqchip(&pctl->gpio_chip, &pctl->irq_chip, irq);
+
+	return 0;
+}
+
+static int stmfx_gpio_function_enable(struct stmfx *stmfx, u32 npins)
+{
+	u32 mask = STMFX_REG_SYS_CTRL_GPIO_EN;
+
+	if (npins > 16)
+		mask += STMFX_REG_SYS_CTRL_ALTGPIO_EN;
+
+	return regmap_write_bits(stmfx->regmap, STMFX_REG_SYS_CTRL, mask, mask);
+}
+
+static int stmfx_gpio_init(struct stmfx *stmfx, struct device_node *np)
+{
+	struct stmfx_pinctrl *pctl;
+	int ret;
+
+	pctl = devm_kzalloc(stmfx->dev, sizeof(*pctl), GFP_KERNEL);
+	if (!pctl)
+		return -ENOMEM;
+
+	pctl->dev = stmfx->dev;
+	pctl->regmap = stmfx->regmap;
+	stmfx->pctl = pctl;
+
+	mutex_init(&pctl->lock);
+
+	/* Register pin controller */
+	pctl->pctl_desc.name = "stmfx-pinctrl";
+	pctl->pctl_desc.pctlops = &stmfx_pinctrl_ops;
+	pctl->pctl_desc.confops = &stmfx_pinconf_ops;
+	pctl->pctl_desc.pins = stmfx_pins;
+	pctl->pctl_desc.npins = ARRAY_SIZE(stmfx_pins);
+	pctl->pctl_desc.owner = THIS_MODULE;
+
+	ret = devm_pinctrl_register_and_init(pctl->dev, &pctl->pctl_desc,
+					     pctl, &pctl->pctl_dev);
+	if (ret) {
+		dev_err(pctl->dev, "pinctrl registration failed\n");
+		return ret;
+	}
+
+	ret = pinctrl_enable(pctl->pctl_dev);
+	if (ret) {
+		dev_err(pctl->dev, "pinctrl enable failed\n");
+		return ret;
+	}
+
+	/* Register gpio controller */
+	pctl->gpio_chip.label = "stmfx-gpio";
+	pctl->gpio_chip.parent = pctl->dev;
+	pctl->gpio_chip.get_direction = stmfx_gpio_get_direction;
+	pctl->gpio_chip.direction_input = stmfx_gpio_direction_input;
+	pctl->gpio_chip.direction_output = stmfx_gpio_direction_output;
+	pctl->gpio_chip.get = stmfx_gpio_get;
+	pctl->gpio_chip.set = stmfx_gpio_set;
+	pctl->gpio_chip.set_config = gpiochip_generic_config;
+	pctl->gpio_chip.base = -1;
+	pctl->gpio_chip.ngpio = pctl->pctl_desc.npins;
+	pctl->gpio_chip.can_sleep = true;
+	pctl->gpio_chip.of_node = np;
+
+	ret = stmfx_gpio_irq_init(pctl, stmfx->irq);
+	if (ret)
+		return ret;
+
+	ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
+	if (ret) {
+		dev_err(pctl->dev, "gpio_chip registration failed\n");
+		return ret;
+	}
+
+	if (!of_find_property(np, "gpio-ranges", NULL)) {
+		ret = gpiochip_add_pin_range(&pctl->gpio_chip,
+					     dev_name(pctl->dev),
+					     0, 0, pctl->pctl_desc.npins);
+		if (ret)
+			return ret;
+	}
+
+	ret = stmfx_gpio_function_enable(stmfx, pctl->gpio_chip.ngpio);
+	if (ret)
+		return ret;
+
+	dev_info(pctl->dev, "%d GPIOs available\n", pctl->gpio_chip.ngpio);
+
+	return 0;
+}
+
+static int stmfx_irq_init(struct stmfx *stmfx)
+{
+	u32 irqoutpin = 0, irqtrigger;
+
+	if (stmfx->irq < 0) {
+		dev_err(stmfx->dev, "failed to get irq: %d\n", stmfx->irq);
+		return stmfx->irq;
+	}
+
+	if (!of_property_read_bool(stmfx->dev->of_node, "drive-open-drain"))
+		irqoutpin |= STMFX_REG_IRQ_OUT_PIN_TYPE;
+
+	irqtrigger = irq_get_trigger_type(stmfx->irq);
+	if ((irqtrigger & IRQ_TYPE_EDGE_RISING) ||
+	    (irqtrigger & IRQ_TYPE_LEVEL_HIGH))
+		irqoutpin |= STMFX_REG_IRQ_OUT_PIN_POL;
+
+	return regmap_write(stmfx->regmap, STMFX_REG_IRQ_OUT_PIN, irqoutpin);
+}
+
+static int stmfx_chip_reset(struct stmfx *stmfx)
+{
+	int ret;
+
+	ret = regmap_update_bits(stmfx->regmap, STMFX_REG_SYS_CTRL,
+				 STMFX_REG_SYS_CTRL_SWRST,
+				 STMFX_REG_SYS_CTRL_SWRST);
+	if (ret)
+		return ret;
+
+	msleep(STMFX_BOOT_TIME_MS);
+
+	return ret;
+}
+
+static int stmfx_chip_init(struct stmfx *stmfx, struct i2c_client *client)
+{
+	u32 id;
+	u8 version[2];
+	int ret;
+
+	stmfx->vdd = devm_regulator_get_optional(stmfx->dev, "vdd");
+	if (IS_ERR(stmfx->vdd)) {
+		ret = PTR_ERR(stmfx->vdd);
+		if (ret != -ENODEV) {
+			if (ret != -EPROBE_DEFER)
+				dev_err(stmfx->dev,
+					"no vdd regulator found:%d\n", ret);
+			return ret;
+		}
+	}
+
+	if (!IS_ERR(stmfx->vdd)) {
+		ret = regulator_enable(stmfx->vdd);
+		if (ret) {
+			dev_err(stmfx->dev, "vdd enable failed: %d\n", ret);
+			return ret;
+		}
+	}
+
+	ret = regmap_read(stmfx->regmap, STMFX_REG_CHIP_ID, &id);
+	if (ret) {
+		dev_err(stmfx->dev, "error reading chip id: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Check that ID is the complement of the I2C address:
+	 * STMFX I2C address follows the 7-bit format (MSB), that's why
+	 * client->addr is shifted.
+	 *
+	 * STMFX_I2C_ADDR|       STMFX         |        Linux
+	 *   input pin   | I2C device address  | I2C device address
+	 *---------------------------------------------------------
+	 *       0       | b: 1000 010x h:0x84 |       0x42
+	 *       1       | b: 1000 011x h:0x86 |       0x43
+	 */
+	if (FIELD_GET(STMFX_REG_CHIP_ID_MASK, ~id) != (client->addr << 1)) {
+		dev_err(stmfx->dev, "unknown chip id: %#x\n", id);
+		return -EINVAL;
+	}
+
+	ret = regmap_bulk_read(stmfx->regmap, STMFX_REG_FW_VERSION_MSB,
+			       version, ARRAY_SIZE(version));
+	if (ret) {
+		dev_err(stmfx->dev, "error reading fw version: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(stmfx->dev, "STMFX id: %#x, fw version: %x.%02x\n",
+		 id, version[0], version[1]);
+
+	return stmfx_chip_reset(stmfx);
+}
+
+static void stmfx_chip_exit(struct stmfx *stmfx)
+{
+	regmap_write(stmfx->regmap, STMFX_REG_IRQ_SRC_EN, 0);
+	regmap_write(stmfx->regmap, STMFX_REG_SYS_CTRL, 0);
+
+	if (!IS_ERR(stmfx->vdd))
+		regulator_disable(stmfx->vdd);
+}
+
+static const struct regmap_config stmfx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int stmfx_probe(struct i2c_client *client,
+		       const struct i2c_device_id *id)
+{
+	struct device_node *np = client->dev.of_node, *child;
+	struct stmfx *stmfx;
+	int ret;
+
+	stmfx = devm_kzalloc(&client->dev, sizeof(*stmfx), GFP_KERNEL);
+	if (!stmfx)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, stmfx);
+
+	stmfx->dev = &client->dev;
+
+	stmfx->regmap = devm_regmap_init_i2c(client, &stmfx_regmap_config);
+	if (IS_ERR(stmfx->regmap)) {
+		ret = PTR_ERR(stmfx->regmap);
+		dev_err(stmfx->dev,
+			"Failed to allocate register map: %d\n", ret);
+		return ret;
+	}
+
+	ret = stmfx_chip_init(stmfx, client);
+	if (ret) {
+		if (ret == -ETIMEDOUT)
+			return -EPROBE_DEFER;
+		return ret;
+	}
+
+	stmfx->irq = client->irq;
+	ret = stmfx_irq_init(stmfx);
+	if (ret)
+		return ret;
+
+	for_each_available_child_of_node(np, child) {
+		if (of_property_read_bool(child, "gpio-controller")) {
+			ret = stmfx_gpio_init(stmfx, child);
+			if (ret)
+				goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	stmfx_chip_exit(stmfx);
+
+	return ret;
+}
+
+static int stmfx_remove(struct i2c_client *client)
+{
+	struct stmfx *stmfx = i2c_get_clientdata(client);
+
+	stmfx_chip_exit(stmfx);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static void stmfx_gpio_backup_regs(struct stmfx_pinctrl *pctl)
+{
+	regmap_bulk_read(pctl->regmap, STMFX_REG_GPIO_STATE,
+			 &pctl->bkp_gpio_state, NR_GPIO_REGS);
+	regmap_bulk_read(pctl->regmap, STMFX_REG_GPIO_DIR,
+			 &pctl->bkp_gpio_dir, NR_GPIO_REGS);
+	regmap_bulk_read(pctl->regmap, STMFX_REG_GPIO_TYPE,
+			 &pctl->bkp_gpio_type, NR_GPIO_REGS);
+	regmap_bulk_read(pctl->regmap, STMFX_REG_GPIO_PUPD,
+			 &pctl->bkp_gpio_pupd, NR_GPIO_REGS);
+}
+
+static void stmfx_gpio_restore_regs(struct stmfx_pinctrl *pctl)
+{
+	regmap_bulk_write(pctl->regmap, STMFX_REG_GPIO_STATE,
+			  pctl->bkp_gpio_state, NR_GPIO_REGS);
+	regmap_bulk_write(pctl->regmap, STMFX_REG_GPIO_DIR,
+			  pctl->bkp_gpio_dir, NR_GPIO_REGS);
+	regmap_bulk_write(pctl->regmap, STMFX_REG_GPIO_TYPE,
+			  pctl->bkp_gpio_type, NR_GPIO_REGS);
+	regmap_bulk_write(pctl->regmap, STMFX_REG_GPIO_PUPD,
+			  pctl->bkp_gpio_pupd, NR_GPIO_REGS);
+
+	regmap_bulk_write(pctl->regmap, STMFX_REG_IRQ_GPI_EVT,
+			  pctl->irq_gpi_evt, NR_GPIO_REGS);
+	regmap_bulk_write(pctl->regmap, STMFX_REG_IRQ_GPI_TYPE,
+			  pctl->irq_gpi_type, NR_GPIO_REGS);
+	regmap_bulk_write(pctl->regmap, STMFX_REG_IRQ_GPI_SRC,
+			  pctl->irq_gpi_src, NR_GPIO_REGS);
+
+	/*
+	 * If at least one GPIO irq is enabled among the 24 available,
+	 * enable global GPIO irq
+	 */
+	if (*(u32 *)pctl->irq_gpi_src & GENMASK(23, 0))
+		regmap_write_bits(pctl->regmap, STMFX_REG_IRQ_SRC_EN,
+				  STMFX_REG_IRQ_SRC_EN_GPIO,
+				  STMFX_REG_IRQ_SRC_EN_GPIO);
+}
+
+static int stmfx_suspend(struct device *dev)
+{
+	struct stmfx *stmfx = dev_get_drvdata(dev);
+
+	stmfx_gpio_backup_regs(stmfx->pctl);
+
+	if (!IS_ERR(stmfx->vdd))
+		regulator_disable(stmfx->vdd);
+
+	return 0;
+}
+
+static int stmfx_resume(struct device *dev)
+{
+	struct stmfx *stmfx = dev_get_drvdata(dev);
+
+	if (!IS_ERR(stmfx->vdd))
+		regulator_enable(stmfx->vdd);
+
+	/* Restore IRQ_OUT_PIN register */
+	stmfx_irq_init(stmfx);
+
+	/* Restore SYS_CTRL register */
+	stmfx_gpio_function_enable(stmfx, stmfx->pctl->gpio_chip.ngpio);
+
+	/* Restore all GPIO expander function relative registers */
+	stmfx_gpio_restore_regs(stmfx->pctl);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(stmfx_dev_pm_ops, stmfx_suspend, stmfx_resume);
+
+static const struct of_device_id stmfx_of_match[] = {
+	{ .compatible = "st,stmfx-0300", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stmfx_of_match);
+
+static struct i2c_driver stmfx_driver = {
+	.driver = {
+		.name = "stmfx-pinctrl",
+		.of_match_table = of_match_ptr(stmfx_of_match),
+		.pm = &stmfx_dev_pm_ops,
+	},
+	.probe = stmfx_probe,
+	.remove = stmfx_remove,
+};
+module_i2c_driver(stmfx_driver);
+
+MODULE_DESCRIPTION("STMFX pinctrl/GPIO driver");
+MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH 2/5] pinctrl: Add STMFX GPIO expander Pinctrl/GPIO driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1959 bytes --]

Hi Amelie,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.16]
[cannot apply to next-20180411]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Amelie-Delaunay/Introduce-STMFX-I2C-GPIO-expander/20180412-082837
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All warnings (new ones prefixed by >>):

   drivers/pinctrl/pinctrl-stmfx.c: In function 'stmfx_resume':
>> drivers/pinctrl/pinctrl-stmfx.c:949:3: warning: ignoring return value of 'regulator_enable', declared with attribute warn_unused_result [-Wunused-result]
      regulator_enable(stmfx->vdd);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/regulator_enable +949 drivers/pinctrl/pinctrl-stmfx.c

   943	
   944	static int stmfx_resume(struct device *dev)
   945	{
   946		struct stmfx *stmfx = dev_get_drvdata(dev);
   947	
   948		if (!IS_ERR(stmfx->vdd))
 > 949			regulator_enable(stmfx->vdd);
   950	
   951		/* Restore IRQ_OUT_PIN register */
   952		stmfx_irq_init(stmfx);
   953	
   954		/* Restore SYS_CTRL register */
   955		stmfx_gpio_function_enable(stmfx, stmfx->pctl->gpio_chip.ngpio);
   956	
   957		/* Restore all GPIO expander function relative registers */
   958		stmfx_gpio_restore_regs(stmfx->pctl);
   959	
   960		return 0;
   961	}
   962	#endif
   963	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53491 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] pinctrl: Add STMFX GPIO expander Pinctrl/GPIO driver

[Linus Walleij]

On Wed, Apr 11, 2018 at 11:47 AM, Amelie Delaunay
<amelie.delaunay@st.com> wrote:

Hi Amelie, thanks for your patch!

> This patch adds pinctrl/GPIO driver for STMicroelectronics
> Multi-Function eXpander (STMFX) GPIO expander.
> STMFX is an I2C slave controller, offering up to 24 GPIOs.
> The driver relies on generic pin config interface to configure the GPIOs.
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>

This is looking very good.

The overall architecture of this patch set is excellent.

I have only one major question about whether this should be
a MFD parent and GPIO-pinctrl-child split driver, see below.

> +config PINCTRL_STMFX
> +       tristate "STMicroelectronics STMFX I2C GPIO expander pinctrl driver"
> +       depends on GPIOLIB && I2C=y
> +       select GENERIC_PINCONF
> +       select GPIOLIB_IRQCHIP
> +       select REGMAP_I2C

Thanks for using all the helpers, it makes the code very small and
consistent and easy to review and maintain.

> +#include <linux/bitfield.h>
> +#include <linux/gpio.h>

Please just use:
 #include <linux/gpio/driver.h>

> +static void stmfx_gpio_irq_toggle_trigger(struct stmfx_pinctrl *pctl,
> +                                         unsigned int offset)
> +{
> +       u32 reg = get_reg(offset);
> +       u32 mask = get_mask(offset);
> +       int val;
> +
> +       if (!(pctl->irq_toggle_edge[reg] & mask))
> +               return;
> +
> +       val = stmfx_gpio_get(&pctl->gpio_chip, offset);
> +       if (val < 0)
> +               return;
> +
> +       if (val) {
> +               pctl->irq_gpi_type[reg] &= mask;
> +               regmap_write_bits(pctl->regmap, STMFX_REG_IRQ_GPI_TYPE + reg,
> +                                 get_mask(offset), 0);
> +
> +       } else {
> +               pctl->irq_gpi_type[reg] |= mask;
> +               regmap_write_bits(pctl->regmap, STMFX_REG_IRQ_GPI_TYPE + reg,
> +                                 get_mask(offset), get_mask(offset));
> +       }
> +}

We had a bit of discussion about edge trigger emulation on the
mailing list. Strange that these HW engineers didn't think of this,
I thought it was widely known that double-edge trigger (not just one
or the other) is needed in contemporary GPIO HW.

> +       if (!of_find_property(np, "gpio-ranges", NULL)) {
> +               ret = gpiochip_add_pin_range(&pctl->gpio_chip,
> +                                            dev_name(pctl->dev),
> +                                            0, 0, pctl->pctl_desc.npins);
> +               if (ret)
> +                       return ret;

Do you really need to support DTBs without gpio-ranges?

I.e. can't you just print an error and exit if there is no range?

I think other drivers has this handling because of older
DT bindings and trees drifting around.

> +static const struct regmap_config stmfx_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +};

This can probably be improved in the future.
Or are there really exactly 255 registers?

> +static int stmfx_probe(struct i2c_client *client,
> +                      const struct i2c_device_id *id)
> +{
> +       struct device_node *np = client->dev.of_node, *child;
> +       struct stmfx *stmfx;
> +       int ret;
> +
> +       stmfx = devm_kzalloc(&client->dev, sizeof(*stmfx), GFP_KERNEL);
> +       if (!stmfx)
> +               return -ENOMEM;
> +
> +       i2c_set_clientdata(client, stmfx);
> +
> +       stmfx->dev = &client->dev;
> +
> +       stmfx->regmap = devm_regmap_init_i2c(client, &stmfx_regmap_config);
> +       if (IS_ERR(stmfx->regmap)) {
> +               ret = PTR_ERR(stmfx->regmap);
> +               dev_err(stmfx->dev,
> +                       "Failed to allocate register map: %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = stmfx_chip_init(stmfx, client);
> +       if (ret) {
> +               if (ret == -ETIMEDOUT)
> +                       return -EPROBE_DEFER;
> +               return ret;
> +       }
> +
> +       stmfx->irq = client->irq;
> +       ret = stmfx_irq_init(stmfx);
> +       if (ret)
> +               return ret;
> +
> +       for_each_available_child_of_node(np, child) {
> +               if (of_property_read_bool(child, "gpio-controller")) {
> +                       ret = stmfx_gpio_init(stmfx, child);
> +                       if (ret)
> +                               goto err;
> +               }
> +       }

Hm so you do not use a MFD driver for the core of the driver?

Instead this driver becomes the core driver?

I guess it is fine as long as the chip is only doing GPIO
and pin control. If the chip has more abilities (such as PWM,
LED...) then the core should be an MFD driver that spawns
GPIO/pinctrl driver as a child, then this child looks up the
regmap from the parent MFD driver.

See for example how the simple STw481x driver does things:
drivers/mfd/stw481x.c
drivers/regulator/stw481x-vmmc.c

This MFD has no GPIO/pin control but it illustrates a simple
parent/child instantiation with an MFD core driver.

It has this DTS entry:

                stw4811@2d {
                        compatible = "st,stw4811";
                        reg = <0x2d>;
                        vmmc_regulator: vmmc {
                                compatible = "st,stw481x-vmmc";
                                regulator-name = "VMMC";
                                regulator-min-microvolt = <1800000>;
                                regulator-max-microvolt = <3300000>;
                        };
                };

The MFD driver matches and spawns the VMMC child
then that driver mathes to the vmmc node.

Yours,
Linus Walleij

Re: [PATCH 2/5] pinctrl: Add STMFX GPIO expander Pinctrl/GPIO driver

[Amelie DELAUNAY]

On 04/26/2018 02:48 PM, Linus Walleij wrote:
> On Wed, Apr 11, 2018 at 11:47 AM, Amelie Delaunay
> <amelie.delaunay@st.com> wrote:
> 
> Hi Amelie, thanks for your patch!
> 

Hi Linus, thanks for reviewing!

>> This patch adds pinctrl/GPIO driver for STMicroelectronics
>> Multi-Function eXpander (STMFX) GPIO expander.
>> STMFX is an I2C slave controller, offering up to 24 GPIOs.
>> The driver relies on generic pin config interface to configure the GPIOs.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> 
> This is looking very good.
> 
> The overall architecture of this patch set is excellent.
> 
> I have only one major question about whether this should be
> a MFD parent and GPIO-pinctrl-child split driver, see below.
> 
>> +config PINCTRL_STMFX
>> +       tristate "STMicroelectronics STMFX I2C GPIO expander pinctrl driver"
>> +       depends on GPIOLIB && I2C=y
>> +       select GENERIC_PINCONF
>> +       select GPIOLIB_IRQCHIP
>> +       select REGMAP_I2C
> 
> Thanks for using all the helpers, it makes the code very small and
> consistent and easy to review and maintain.
> 
>> +#include <linux/bitfield.h>
>> +#include <linux/gpio.h>
> 
> Please just use:
>   #include <linux/gpio/driver.h>
> 

OK, I'll fix it.

>> +static void stmfx_gpio_irq_toggle_trigger(struct stmfx_pinctrl *pctl,
>> +                                         unsigned int offset)
>> +{
>> +       u32 reg = get_reg(offset);
>> +       u32 mask = get_mask(offset);
>> +       int val;
>> +
>> +       if (!(pctl->irq_toggle_edge[reg] & mask))
>> +               return;
>> +
>> +       val = stmfx_gpio_get(&pctl->gpio_chip, offset);
>> +       if (val < 0)
>> +               return;
>> +
>> +       if (val) {
>> +               pctl->irq_gpi_type[reg] &= mask;
>> +               regmap_write_bits(pctl->regmap, STMFX_REG_IRQ_GPI_TYPE + reg,
>> +                                 get_mask(offset), 0);
>> +
>> +       } else {
>> +               pctl->irq_gpi_type[reg] |= mask;
>> +               regmap_write_bits(pctl->regmap, STMFX_REG_IRQ_GPI_TYPE + reg,
>> +                                 get_mask(offset), get_mask(offset));
>> +       }
>> +}
> 
> We had a bit of discussion about edge trigger emulation on the
> mailing list. Strange that these HW engineers didn't think of this,
> I thought it was widely known that double-edge trigger (not just one
> or the other) is needed in contemporary GPIO HW.
> 

Yes! Supported by the STM32L152 on which stmfx is based but not by the 
FW abstraction. So, maybe, in a future stmfx FW version...

>> +       if (!of_find_property(np, "gpio-ranges", NULL)) {
>> +               ret = gpiochip_add_pin_range(&pctl->gpio_chip,
>> +                                            dev_name(pctl->dev),
>> +                                            0, 0, pctl->pctl_desc.npins);
>> +               if (ret)
>> +                       return ret;
> 
> Do you really need to support DTBs without gpio-ranges?
> 
> I.e. can't you just print an error and exit if there is no range?
> 
> I think other drivers has this handling because of older
> DT bindings and trees drifting around.
> 

You're right, no need to support DTBs without gpio-ranges. I'll add 
gpio-ranges property under required section of stmfx bindings.

>> +static const struct regmap_config stmfx_regmap_config = {
>> +       .reg_bits = 8,
>> +       .val_bits = 8,
>> +};
> 
> This can probably be improved in the future.
> Or are there really exactly 255 registers?
> 

OK, I'll have a look to improve it.

>> +static int stmfx_probe(struct i2c_client *client,
>> +                      const struct i2c_device_id *id)
>> +{
>> +       struct device_node *np = client->dev.of_node, *child;
>> +       struct stmfx *stmfx;
>> +       int ret;
>> +
>> +       stmfx = devm_kzalloc(&client->dev, sizeof(*stmfx), GFP_KERNEL);
>> +       if (!stmfx)
>> +               return -ENOMEM;
>> +
>> +       i2c_set_clientdata(client, stmfx);
>> +
>> +       stmfx->dev = &client->dev;
>> +
>> +       stmfx->regmap = devm_regmap_init_i2c(client, &stmfx_regmap_config);
>> +       if (IS_ERR(stmfx->regmap)) {
>> +               ret = PTR_ERR(stmfx->regmap);
>> +               dev_err(stmfx->dev,
>> +                       "Failed to allocate register map: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       ret = stmfx_chip_init(stmfx, client);
>> +       if (ret) {
>> +               if (ret == -ETIMEDOUT)
>> +                       return -EPROBE_DEFER;
>> +               return ret;
>> +       }
>> +
>> +       stmfx->irq = client->irq;
>> +       ret = stmfx_irq_init(stmfx);
>> +       if (ret)
>> +               return ret;
>> +
>> +       for_each_available_child_of_node(np, child) {
>> +               if (of_property_read_bool(child, "gpio-controller")) {
>> +                       ret = stmfx_gpio_init(stmfx, child);
>> +                       if (ret)
>> +                               goto err;
>> +               }
>> +       }
> 
> Hm so you do not use a MFD driver for the core of the driver?
> 
> Instead this driver becomes the core driver?
> 
> I guess it is fine as long as the chip is only doing GPIO
> and pin control. If the chip has more abilities (such as PWM,
> LED...) then the core should be an MFD driver that spawns
> GPIO/pinctrl driver as a child, then this child looks up the
> regmap from the parent MFD driver.

Yes, this resumes the binding patch concerns.
stmfx chip on boards supported by Linux is only doing GPIO and pin 
control. The other abilities (IDD measurement and TouchScreen 
controller) are not used and cannot be tested with Linux for the time 
being. But if these function are used, then the core part of this driver 
(all not stmfx_gpio_/stmfx_pinctrl_/stmfx_pinconf_ prefixed functions) 
will become an MFD driver.

> 
> See for example how the simple STw481x driver does things:
> drivers/mfd/stw481x.c
> drivers/regulator/stw481x-vmmc.c
> 
> This MFD has no GPIO/pin control but it illustrates a simple
> parent/child instantiation with an MFD core driver.
> 
> It has this DTS entry:
> 
>                  stw4811@2d {
>                          compatible = "st,stw4811";
>                          reg = <0x2d>;
>                          vmmc_regulator: vmmc {
>                                  compatible = "st,stw481x-vmmc";
>                                  regulator-name = "VMMC";
>                                  regulator-min-microvolt = <1800000>;
>                                  regulator-max-microvolt = <3300000>;
>                          };
>                  };
> 
> The MFD driver matches and spawns the VMMC child
> then that driver mathes to the vmmc node.
> 
> Yours,
> Linus Walleij
> 

Thanks,
Amelie

[PATCH 3/5] ARM: dts: stm32: add STMFX pinctrl/gpio expander support on stm32746g-eval

[Amelie Delaunay]

This patch adds support for STMicroelectronics Multi-Function eXpander
pinctrl/gpio expander on stm32746g-eval. It is connected on I2C1.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 arch/arm/boot/dts/stm32746g-eval.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/stm32746g-eval.dts b/arch/arm/boot/dts/stm32746g-eval.dts
index 2d4e717..bd9a33d 100644
--- a/arch/arm/boot/dts/stm32746g-eval.dts
+++ b/arch/arm/boot/dts/stm32746g-eval.dts
@@ -106,6 +106,22 @@
 	i2c-scl-rising-time-ns = <185>;
 	i2c-scl-falling-time-ns = <20>;
 	status = "okay";
+
+	stmfxpinctrl: stmfx@42 {
+		compatible = "st,stmfx-0300";
+		reg = <0x42>;
+		interrupt-parent = <&gpioi>;
+		interrupts = <8 1>;
+		status = "okay";
+
+		stmfxgpio: gpio-expander {
+			#gpio-cells = <2>;
+			#interrupt-cells = <2>;
+			gpio-controller;
+			interrupt-controller;
+			status = "okay";
+		};
+	};
 };
 
 &rtc {
-- 
2.7.4

[PATCH 4/5] ARM: dts: stm32: add orange and blue leds on stm32746g-eval

[Amelie Delaunay]

Orange (LD2) and blue (LD4) leds on stm32746g-eval are connected on
STMFX gpio expander, offset 17 and 19.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 arch/arm/boot/dts/stm32746g-eval.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/stm32746g-eval.dts b/arch/arm/boot/dts/stm32746g-eval.dts
index bd9a33d..fd03f54 100644
--- a/arch/arm/boot/dts/stm32746g-eval.dts
+++ b/arch/arm/boot/dts/stm32746g-eval.dts
@@ -67,9 +67,15 @@
 			gpios = <&gpiof 10 1>;
 			linux,default-trigger = "heartbeat";
 		};
+		orange {
+			gpios = <&stmfxgpio 17 1>;
+		};
 		red {
 			gpios = <&gpiob 7 1>;
 		};
+		blue {
+			gpios = <&stmfxgpio 19 1>;
+		};
 	};
 
 	gpio_keys {
-- 
2.7.4

[PATCH 5/5] ARM: dts: stm32: add joystick support on stm32746g-eval

[Amelie Delaunay]

The joystick (B3) on stm32746g-eval uses gpios on STMFX gpio expander.
These gpios need a pin configuration (push-pull and bias-pull-up),
described under stmfxpinctrl node.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 arch/arm/boot/dts/stm32746g-eval.dts | 44 ++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/arch/arm/boot/dts/stm32746g-eval.dts b/arch/arm/boot/dts/stm32746g-eval.dts
index fd03f54..ee9ec55 100644
--- a/arch/arm/boot/dts/stm32746g-eval.dts
+++ b/arch/arm/boot/dts/stm32746g-eval.dts
@@ -90,6 +90,44 @@
 		};
 	};
 
+	joystick {
+		compatible = "gpio-keys";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		pinctrl-0 = <&joystick_pins>;
+		pinctrl-names = "default";
+		button@0 {
+			label = "JoySel";
+			linux,code = <KEY_ENTER>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <0 2>;
+		};
+		button@1 {
+			label = "JoyDown";
+			linux,code = <KEY_DOWN>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <1 2>;
+		};
+		button@2 {
+			label = "JoyLeft";
+			linux,code = <KEY_LEFT>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <2 2>;
+		};
+		button@3 {
+			label = "JoyRight";
+			linux,code = <KEY_RIGHT>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <3 2>;
+		};
+		button@4 {
+			label = "JoyUp";
+			linux,code = <KEY_UP>;
+			interrupt-parent = <&stmfxgpio>;
+			interrupts = <4 2>;
+		};
+	};
+
 	usbotg_hs_phy: usb-phy {
 		#phy-cells = <0>;
 		compatible = "usb-nop-xceiv";
@@ -127,6 +165,12 @@
 			interrupt-controller;
 			status = "okay";
 		};
+
+		joystick_pins: joystick@0 {
+			pins = "gpio0", "gpio1", "gpio2", "gpio3", "gpio4";
+			drive-push-pull;
+			bias-pull-up;
+		};
 	};
 };
 
-- 
2.7.4