[PATCH 0/6] Introduce STMicroelectronics MultiFunction eXpander

[PATCH 0/6] Introduce STMicroelectronics MultiFunction eXpander

[Amelie Delaunay]

This series adds support for STMicroelectronics MultiFunction eXpander
(ST MFX), used on some STM32 discovery and evaluation boards.

ST MFX is an STM32L152 slave controller whose firmware embeds the following
features:
- I/O expander (16 GPIOs + 8 extra if the other features are not enabled),
- resistive touchscreen controller,
- IDD measurement.
Using an I2C bus, the main MCU can control the MFX. MFX internal/external
events are sent to the main MCU by the MFX_IRQ_OUT_PIN.
  
Amelie Delaunay (6):
  dt-bindings: mfd: Add ST Multi-Function eXpander driver
  mfd: Add ST Multi-Function eXpander core driver
  gpio: Add GPIO support for the ST Multi-Function eXpander
  ARM: dts: stm32: add MFX support on I2C1 on stm32746g-eval
  ARM: dts: stm32: add joystick support on stm32746g-eval
  ARM: configs: stm32: enable ST MFX and its GPIO expander feature

 Documentation/devicetree/bindings/mfd/st-mfx.txt |  51 ++
 arch/arm/boot/dts/stm32746g-eval.dts             |  48 ++
 arch/arm/configs/stm32_defconfig                 |   2 +
 drivers/gpio/Kconfig                             |  10 +
 drivers/gpio/Makefile                            |   1 +
 drivers/gpio/gpio-st-mfx.c                       | 589 +++++++++++++++++++++++
 drivers/mfd/Kconfig                              |  15 +
 drivers/mfd/Makefile                             |   1 +
 drivers/mfd/st-mfx.c                             | 526 ++++++++++++++++++++
 include/dt-bindings/gpio/st-mfx-gpio.h           |  24 +
 include/linux/mfd/st-mfx.h                       | 116 +++++
 11 files changed, 1383 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/st-mfx.txt
 create mode 100644 drivers/gpio/gpio-st-mfx.c
 create mode 100644 drivers/mfd/st-mfx.c
 create mode 100644 include/dt-bindings/gpio/st-mfx-gpio.h
 create mode 100644 include/linux/mfd/st-mfx.h

-- 
2.7.4

[PATCH 1/6] dt-bindings: mfd: Add ST Multi-Function eXpander driver

[Amelie Delaunay]

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

Signed-off-by: Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>
---
 Documentation/devicetree/bindings/mfd/st-mfx.txt | 51 ++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/st-mfx.txt

diff --git a/Documentation/devicetree/bindings/mfd/st-mfx.txt b/Documentation/devicetree/bindings/mfd/st-mfx.txt
new file mode 100644
index 0000000..423d800
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/st-mfx.txt
@@ -0,0 +1,51 @@
+STMicroelectronics Multi-Function eXpander
+
+ST Multi-Function eXpander (MFX) is a slave controller using I2C for
+communication with the main MCU. Its main features are gpio expansion, main
+MCU IDD measurement (IDD is the amount of current that flows through VDD)
+and resistive touchscreen controller.
+
+Required properties:
+- compatible: must be "st,mfx"
+- reg: I2C address of the device
+- interrupts: interrupt triggered by MFX_IRQ_OUT signal
+- interrupt-parent: interrupt controller MFX is connected to
+- interrupt-controller: marks the device as an interrupt controller
+- #interrupt-cells: should be <1>, index of the interrupt within the
+  controller, in accordance with the "one cell" variant of
+  <devicetree/bindings/interrupt-controller/interrupt.txt>
+
+Optional nodes:
+
+* GPIO eXpander
+MFX provides 16 programmable GPIOs, and it is also possible to recover 8
+alternate GPIOs if the main functions are not used (touchscreen controller and
+IDD measurement not enabled).
+
+Required properties:
+- compatible : must be "st,mfx-gpio"
+- interrupt-parent : must be <&mfx>
+- interrupts = must be <0>
+- gpio-controller: marks the device node as a GPIO controller
+- #gpio-cells: should be <2>, the first cell is the GPIO offset on this GPIO
+  controller, the second cell is the gpio flags in accordance with
+  <dt-bindings/gpio/st-mfx-gpio.h>.
+
+Example:
+
+	mfx: mfx@42 {
+		compatible = "st,mfx";
+		reg = <0x42>;
+		interrupts = <8 IRQ_TYPE_EDGE_RISING>;
+		interrupt-parent = <&gpioi>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		mfxgpio: mfx_gpio {
+			compatible = "st,mfx-gpio";
+			interrupt-parent = <&mfx>;
+			interrupts = <0>;
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
-- 
2.7.4

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

Re: [PATCH 1/6] dt-bindings: mfd: Add ST Multi-Function eXpander driver

[Rob Herring]

On Thu, Feb 08, 2018 at 03:27:32PM +0100, Amelie Delaunay wrote:
> This patch adds documentation of device tree bindings for the
> STMicroelectronics Multi-Function eXpander (MFX).
> 
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> ---
>  Documentation/devicetree/bindings/mfd/st-mfx.txt | 51 ++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/st-mfx.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/st-mfx.txt b/Documentation/devicetree/bindings/mfd/st-mfx.txt
> new file mode 100644
> index 0000000..423d800
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/st-mfx.txt
> @@ -0,0 +1,51 @@
> +STMicroelectronics Multi-Function eXpander
> +
> +ST Multi-Function eXpander (MFX) is a slave controller using I2C for
> +communication with the main MCU. Its main features are gpio expansion, main
> +MCU IDD measurement (IDD is the amount of current that flows through VDD)
> +and resistive touchscreen controller.

You don't have to implement all the drivers now, but please completely 
describe the device. As is, there is no reason to have a child GPIO 
node.

> +
> +Required properties:
> +- compatible: must be "st,mfx"

Kind of generic. Only 1 single version ever?

> +- reg: I2C address of the device
> +- interrupts: interrupt triggered by MFX_IRQ_OUT signal
> +- interrupt-parent: interrupt controller MFX is connected to
> +- interrupt-controller: marks the device as an interrupt controller
> +- #interrupt-cells: should be <1>, index of the interrupt within the
> +  controller, in accordance with the "one cell" variant of
> +  <devicetree/bindings/interrupt-controller/interrupt.txt>
> +
> +Optional nodes:
> +
> +* GPIO eXpander
> +MFX provides 16 programmable GPIOs, and it is also possible to recover 8
> +alternate GPIOs if the main functions are not used (touchscreen controller and
> +IDD measurement not enabled).
> +
> +Required properties:
> +- compatible : must be "st,mfx-gpio"
> +- interrupt-parent : must be <&mfx>

Not necessary. A parent node with 'interrupt-controller' property is the 
interrupt's parent.

> +- interrupts = must be <0>
> +- gpio-controller: marks the device node as a GPIO controller
> +- #gpio-cells: should be <2>, the first cell is the GPIO offset on this GPIO
> +  controller, the second cell is the gpio flags in accordance with
> +  <dt-bindings/gpio/st-mfx-gpio.h>.

Custom flags? Use standard flags.

DT binding headers should be part of this patch.

> +
> +Example:
> +
> +	mfx: mfx@42 {
> +		compatible = "st,mfx";
> +		reg = <0x42>;
> +		interrupts = <8 IRQ_TYPE_EDGE_RISING>;
> +		interrupt-parent = <&gpioi>;
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		mfxgpio: mfx_gpio {

gpio {

> +			compatible = "st,mfx-gpio";
> +			interrupt-parent = <&mfx>;
> +			interrupts = <0>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +	};
> -- 
> 2.7.4
> 

Re: [PATCH 1/6] dt-bindings: mfd: Add ST Multi-Function eXpander driver

[Amelie DELAUNAY]

On 02/19/2018 12:19 AM, Rob Herring wrote:
> On Thu, Feb 08, 2018 at 03:27:32PM +0100, Amelie Delaunay wrote:
>> This patch adds documentation of device tree bindings for the
>> STMicroelectronics Multi-Function eXpander (MFX).
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>> ---
>>   Documentation/devicetree/bindings/mfd/st-mfx.txt | 51 ++++++++++++++++++++++++
>>   1 file changed, 51 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/mfd/st-mfx.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/st-mfx.txt b/Documentation/devicetree/bindings/mfd/st-mfx.txt
>> new file mode 100644
>> index 0000000..423d800
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/st-mfx.txt
>> @@ -0,0 +1,51 @@
>> +STMicroelectronics Multi-Function eXpander
>> +
>> +ST Multi-Function eXpander (MFX) is a slave controller using I2C for
>> +communication with the main MCU. Its main features are gpio expansion, main
>> +MCU IDD measurement (IDD is the amount of current that flows through VDD)
>> +and resistive touchscreen controller.
> 
> You don't have to implement all the drivers now, but please completely
> describe the device. As is, there is no reason to have a child GPIO
> node.
>
The MFD driver will be abandoned as only GPIO part is used. I'll send a 
V2 soon.
>> +
>> +Required properties:
>> +- compatible: must be "st,mfx"
> 
> Kind of generic. Only 1 single version ever?
> 
"st-mfx" compatible will disappear in V2 (we keep only GPIO driver). MFX 
version can be read in MFX FW_VERSION register. So do I keep 
"st,mfx-gpio" compatible or you want to see mfx version ?

>> +- reg: I2C address of the device
>> +- interrupts: interrupt triggered by MFX_IRQ_OUT signal
>> +- interrupt-parent: interrupt controller MFX is connected to
>> +- interrupt-controller: marks the device as an interrupt controller
>> +- #interrupt-cells: should be <1>, index of the interrupt within the
>> +  controller, in accordance with the "one cell" variant of
>> +  <devicetree/bindings/interrupt-controller/interrupt.txt>
>> +
>> +Optional nodes:
>> +
>> +* GPIO eXpander
>> +MFX provides 16 programmable GPIOs, and it is also possible to recover 8
>> +alternate GPIOs if the main functions are not used (touchscreen controller and
>> +IDD measurement not enabled).
>> +
>> +Required properties:
>> +- compatible : must be "st,mfx-gpio"
>> +- interrupt-parent : must be <&mfx>
> 
> Not necessary. A parent node with 'interrupt-controller' property is the
> interrupt's parent.
> 
I will keep it in mind.
>> +- interrupts = must be <0>
>> +- gpio-controller: marks the device node as a GPIO controller
>> +- #gpio-cells: should be <2>, the first cell is the GPIO offset on this GPIO
>> +  controller, the second cell is the gpio flags in accordance with
>> +  <dt-bindings/gpio/st-mfx-gpio.h>.
> 
> Custom flags? Use standard flags.
> 
> DT binding headers should be part of this patch.
> 
Custom flags because MFX GPIOs have several types:
- Output open drain with internal pull-up resistor
- Output open drain without internal pull-up resistor
- Output push pull without internal pull resistor
- Input with internal pull-up resistor
- Input with internal pull-down resistor
- Input floating
- Input analog.
Standard flags don't have pull up or down information. That's why I am 
using custom flags, they overloads standard flags.
I will add DT bindings header in this patch V2.
>> +
>> +Example:
>> +
>> +	mfx: mfx@42 {
>> +		compatible = "st,mfx";
>> +		reg = <0x42>;
>> +		interrupts = <8 IRQ_TYPE_EDGE_RISING>;
>> +		interrupt-parent = <&gpioi>;
>> +		interrupt-controller;
>> +		#interrupt-cells = <1>;
>> +
>> +		mfxgpio: mfx_gpio {
> 
> gpio {
> 
OK. Will change it in V2.

Thanks,
Amelie
>> +			compatible = "st,mfx-gpio";
>> +			interrupt-parent = <&mfx>;
>> +			interrupts = <0>;
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +		};
>> +	};
>> -- 
>> 2.7.4
>>

Re: [PATCH 1/6] dt-bindings: mfd: Add ST Multi-Function eXpander driver

[Rob Herring]

On Mon, Feb 19, 2018 at 9:59 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>
>
> On 02/19/2018 12:19 AM, Rob Herring wrote:
>> On Thu, Feb 08, 2018 at 03:27:32PM +0100, Amelie Delaunay wrote:
>>> This patch adds documentation of device tree bindings for the
>>> STMicroelectronics Multi-Function eXpander (MFX).
>>>
>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>>> ---
>>>   Documentation/devicetree/bindings/mfd/st-mfx.txt | 51 ++++++++++++++++++++++++
>>>   1 file changed, 51 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/mfd/st-mfx.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/st-mfx.txt b/Documentation/devicetree/bindings/mfd/st-mfx.txt
>>> new file mode 100644
>>> index 0000000..423d800
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/st-mfx.txt
>>> @@ -0,0 +1,51 @@
>>> +STMicroelectronics Multi-Function eXpander
>>> +
>>> +ST Multi-Function eXpander (MFX) is a slave controller using I2C for
>>> +communication with the main MCU. Its main features are gpio expansion, main
>>> +MCU IDD measurement (IDD is the amount of current that flows through VDD)
>>> +and resistive touchscreen controller.
>>
>> You don't have to implement all the drivers now, but please completely
>> describe the device. As is, there is no reason to have a child GPIO
>> node.
>>
> The MFD driver will be abandoned as only GPIO part is used. I'll send a
> V2 soon.
>>> +
>>> +Required properties:
>>> +- compatible: must be "st,mfx"
>>
>> Kind of generic. Only 1 single version ever?
>>
> "st-mfx" compatible will disappear in V2 (we keep only GPIO driver). MFX
> version can be read in MFX FW_VERSION register. So do I keep
> "st,mfx-gpio" compatible or you want to see mfx version ?

That sounds a bit worrying. The other functions will *never* get
accessed? The DT should reflect the h/w including any future needs,
not just what you have a driver for today.

>>> +- reg: I2C address of the device
>>> +- interrupts: interrupt triggered by MFX_IRQ_OUT signal
>>> +- interrupt-parent: interrupt controller MFX is connected to
>>> +- interrupt-controller: marks the device as an interrupt controller
>>> +- #interrupt-cells: should be <1>, index of the interrupt within the
>>> +  controller, in accordance with the "one cell" variant of
>>> +  <devicetree/bindings/interrupt-controller/interrupt.txt>
>>> +
>>> +Optional nodes:
>>> +
>>> +* GPIO eXpander
>>> +MFX provides 16 programmable GPIOs, and it is also possible to recover 8
>>> +alternate GPIOs if the main functions are not used (touchscreen controller and
>>> +IDD measurement not enabled).
>>> +
>>> +Required properties:
>>> +- compatible : must be "st,mfx-gpio"
>>> +- interrupt-parent : must be <&mfx>
>>
>> Not necessary. A parent node with 'interrupt-controller' property is the
>> interrupt's parent.
>>
> I will keep it in mind.
>>> +- interrupts = must be <0>
>>> +- gpio-controller: marks the device node as a GPIO controller
>>> +- #gpio-cells: should be <2>, the first cell is the GPIO offset on this GPIO
>>> +  controller, the second cell is the gpio flags in accordance with
>>> +  <dt-bindings/gpio/st-mfx-gpio.h>.
>>
>> Custom flags? Use standard flags.
>>
>> DT binding headers should be part of this patch.
>>
> Custom flags because MFX GPIOs have several types:
> - Output open drain with internal pull-up resistor
> - Output open drain without internal pull-up resistor
> - Output push pull without internal pull resistor
> - Input with internal pull-up resistor
> - Input with internal pull-down resistor
> - Input floating
> - Input analog.
> Standard flags don't have pull up or down information. That's why I am
> using custom flags, they overloads standard flags.

I'll leave this one to Linus to comment on.

> I will add DT bindings header in this patch V2.

Re: [PATCH 1/6] dt-bindings: mfd: Add ST Multi-Function eXpander driver

[Linus Walleij]

On Mon, Feb 19, 2018 at 4:59 PM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
> On 02/19/2018 12:19 AM, Rob Herring wrote:
>> On Thu, Feb 08, 2018 at 03:27:32PM +0100, Amelie Delaunay wrote:

>>> +- interrupts = must be <0>
>>> +- gpio-controller: marks the device node as a GPIO controller
>>> +- #gpio-cells: should be <2>, the first cell is the GPIO offset on this GPIO
>>> +  controller, the second cell is the gpio flags in accordance with
>>> +  <dt-bindings/gpio/st-mfx-gpio.h>.
>>
>> Custom flags? Use standard flags.
>>
>> DT binding headers should be part of this patch.
>>
> Custom flags because MFX GPIOs have several types:
> - Output open drain with internal pull-up resistor
> - Output open drain without internal pull-up resistor
> - Output push pull without internal pull resistor
> - Input with internal pull-up resistor
> - Input with internal pull-down resistor
> - Input floating
> - Input analog.
> Standard flags don't have pull up or down information. That's why I am
> using custom flags, they overloads standard flags.

This is because pull up/down and tristate/high z (floating)
also known as PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
is controlled by the pin control subsystem, not GPIO.

If your hardware does even more pin control (like multiplexing)
then I suggest that you create a pin control driver back-end for
this and put the resulting driver in drivers/pinctrl.

Some recent additions of expanders in drivers/pinctrl makes
for great inspiration for this work. See for example:
drivers/pinctrl/pinctrl-sx150x.c
drivers/pinctrl/pinctrl-mcp23s08.c

These are both combined pin control and GPIO drivers that
we moved from drivers/gpio because we concluded that they
do more than just GPIO. The GPIO lines are matches to
pins using the GPIO ranges from the call to
gpiochip_add_pin_range() and using gpiochip_generic_config()
as the gpiochip .set_config() callback.

It has been discussed to expose subsets of pin config to
GPIO. Indeed, we already handle open drain/source and
debounce by simply calling into the pin control back-end
or handling it directly in the GPIO driver using the standard
pin config properties.

I am reluctant about this, I think the split of responsibilities
is pretty nice.

I'd recommend looking at the sx150x pin control driver and
check if you can maybe take this direction with the patch?

Yours,
Linus Walleij

Re: [PATCH 1/6] dt-bindings: mfd: Add ST Multi-Function eXpander driver

[Amelie DELAUNAY]

On 02/22/2018 02:22 PM, Linus Walleij wrote:
> On Mon, Feb 19, 2018 at 4:59 PM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>> On 02/19/2018 12:19 AM, Rob Herring wrote:
>>> On Thu, Feb 08, 2018 at 03:27:32PM +0100, Amelie Delaunay wrote:
> 
>>>> +- interrupts = must be <0>
>>>> +- gpio-controller: marks the device node as a GPIO controller
>>>> +- #gpio-cells: should be <2>, the first cell is the GPIO offset on this GPIO
>>>> +  controller, the second cell is the gpio flags in accordance with
>>>> +  <dt-bindings/gpio/st-mfx-gpio.h>.
>>>
>>> Custom flags? Use standard flags.
>>>
>>> DT binding headers should be part of this patch.
>>>
>> Custom flags because MFX GPIOs have several types:
>> - Output open drain with internal pull-up resistor
>> - Output open drain without internal pull-up resistor
>> - Output push pull without internal pull resistor
>> - Input with internal pull-up resistor
>> - Input with internal pull-down resistor
>> - Input floating
>> - Input analog.
>> Standard flags don't have pull up or down information. That's why I am
>> using custom flags, they overloads standard flags.
> 
> This is because pull up/down and tristate/high z (floating)
> also known as PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> is controlled by the pin control subsystem, not GPIO.
> 
> If your hardware does even more pin control (like multiplexing)
> then I suggest that you create a pin control driver back-end for
> this and put the resulting driver in drivers/pinctrl.
> 
> Some recent additions of expanders in drivers/pinctrl makes
> for great inspiration for this work. See for example:
> drivers/pinctrl/pinctrl-sx150x.c
> drivers/pinctrl/pinctrl-mcp23s08.c
> 
> These are both combined pin control and GPIO drivers that
> we moved from drivers/gpio because we concluded that they
> do more than just GPIO. The GPIO lines are matches to
> pins using the GPIO ranges from the call to
> gpiochip_add_pin_range() and using gpiochip_generic_config()
> as the gpiochip .set_config() callback.
> 
> It has been discussed to expose subsets of pin config to
> GPIO. Indeed, we already handle open drain/source and
> debounce by simply calling into the pin control back-end
> or handling it directly in the GPIO driver using the standard
> pin config properties.
> 
> I am reluctant about this, I think the split of responsibilities
> is pretty nice.
> 
> I'd recommend looking at the sx150x pin control driver and
> check if you can maybe take this direction with the patch?
> 

Thanks for highlighting these examples. I will have a look on these!

Regards,
Amelie

> Yours,
> Linus Walleij
> 

Re: [PATCH 1/6] dt-bindings: mfd: Add ST Multi-Function eXpander driver

[Linus Walleij]

On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:

> +Required properties:
> +- compatible: must be "st,mfx"

I bet this should be more specific. Tomorrow there will be a new
version of this expander and then we will wish that we used
a more specific compatible for the first one.

Does this chip have any kind of product number on it?

Else I would be tempted to use the compatible "st,mfx-0000"
or something, indicating it is the first of its kind.

> +- reg: I2C address of the device
> +- interrupts: interrupt triggered by MFX_IRQ_OUT signal
> +- interrupt-parent: interrupt controller MFX is connected to
> +- interrupt-controller: marks the device as an interrupt controller
> +- #interrupt-cells: should be <1>, index of the interrupt within the
> +  controller, in accordance with the "one cell" variant of
> +  <devicetree/bindings/interrupt-controller/interrupt.txt>

Seems fine.

> +Optional nodes:
> +
> +* GPIO eXpander
> +MFX provides 16 programmable GPIOs, and it is also possible to recover 8
> +alternate GPIOs if the main functions are not used (touchscreen controller and
> +IDD measurement not enabled).

Apparenly Rob thinks this should go elsewhere.

> +- gpio-controller: marks the device node as a GPIO controller
> +- #gpio-cells: should be <2>, the first cell is the GPIO offset on this GPIO
> +  controller, the second cell is the gpio flags in accordance with
> +  <dt-bindings/gpio/st-mfx-gpio.h>.

Let's discuss these extra GPIO flag bindings separately.

Yours,
Linus Walleij

Re: [PATCH 1/6] dt-bindings: mfd: Add ST Multi-Function eXpander driver

[Amelie DELAUNAY]

On 02/22/2018 02:11 PM, Linus Walleij wrote:
> On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:
> 
>> +Required properties:
>> +- compatible: must be "st,mfx"
> 
> I bet this should be more specific. Tomorrow there will be a new
> version of this expander and then we will wish that we used
> a more specific compatible for the first one.
> 
> Does this chip have any kind of product number on it?
> 
> Else I would be tempted to use the compatible "st,mfx-0000"
> or something, indicating it is the first of its kind.
> 

This chip has a FW version. So, I agree, I should use a compatible that 
reflects this FW version.

>> +- reg: I2C address of the device
>> +- interrupts: interrupt triggered by MFX_IRQ_OUT signal
>> +- interrupt-parent: interrupt controller MFX is connected to
>> +- interrupt-controller: marks the device as an interrupt controller
>> +- #interrupt-cells: should be <1>, index of the interrupt within the
>> +  controller, in accordance with the "one cell" variant of
>> +  <devicetree/bindings/interrupt-controller/interrupt.txt>
> 
> Seems fine.
> 
>> +Optional nodes:
>> +
>> +* GPIO eXpander
>> +MFX provides 16 programmable GPIOs, and it is also possible to recover 8
>> +alternate GPIOs if the main functions are not used (touchscreen controller and
>> +IDD measurement not enabled).
> 
> Apparenly Rob thinks this should go elsewhere.
> 

It would go with gpio/pinctrl driver, so in dt-bindings/(gpio|pinctrl).

Regards,
Amelie

>> +- gpio-controller: marks the device node as a GPIO controller
>> +- #gpio-cells: should be <2>, the first cell is the GPIO offset on this GPIO
>> +  controller, the second cell is the gpio flags in accordance with
>> +  <dt-bindings/gpio/st-mfx-gpio.h>.
> 
> Let's discuss these extra GPIO flag bindings separately.
> 
> Yours,
> Linus Walleij
> 

[PATCH 6/6] ARM: configs: stm32: enable ST MFX and its GPIO expander feature

[Amelie Delaunay]

Signed-off-by: Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>
---
 arch/arm/configs/stm32_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/stm32_defconfig b/arch/arm/configs/stm32_defconfig
index eb3c2e4..aa9feed 100644
--- a/arch/arm/configs/stm32_defconfig
+++ b/arch/arm/configs/stm32_defconfig
@@ -52,9 +52,11 @@ CONFIG_I2C_CHARDEV=y
 CONFIG_I2C_STM32F4=y
 CONFIG_I2C_STM32F7=y
 CONFIG_GPIO_STMPE=y
+CONFIG_GPIO_ST_MFX=y
 # CONFIG_HWMON is not set
 CONFIG_WATCHDOG=y
 CONFIG_MFD_STMPE=y
+CONFIG_MFD_ST_MFX=y
 CONFIG_REGULATOR=y
 CONFIG_REGULATOR_FIXED_VOLTAGE=y
 # CONFIG_USB_SUPPORT is not set
-- 
2.7.4

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

[PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver

[Amelie Delaunay]

ST Multi-Function eXpander (MFX) is a slave controller using I2C
for communication with the main MCU. Main features are:
- 16 fast GPIOs individually configurable in input/output
- 8 alternate GPIOs individually configurable in input/output
- Main MCU IDD measurement
- Resistive touchscreen controller

Only GPIO expander (16 fast GPIOs + 8 alternate) feature is
supported for the moment.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 drivers/mfd/Kconfig        |  15 ++
 drivers/mfd/Makefile       |   1 +
 drivers/mfd/st-mfx.c       | 526 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/st-mfx.h | 116 ++++++++++
 4 files changed, 658 insertions(+)
 create mode 100644 drivers/mfd/st-mfx.c
 create mode 100644 include/linux/mfd/st-mfx.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1d20a80..e78e818 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1823,6 +1823,21 @@ config MFD_STM32_TIMERS
 	  for PWM and IIO Timer. This driver allow to share the
 	  registers between the others drivers.
 
+config MFD_ST_MFX
+	bool "STMicroelectronics MFX"
+	depends on I2C
+	depends on OF || COMPILE_TEST
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  Support for the STMicroelectronics Multi-Function eXpander.
+
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the functionality
+	  of the device. Currently available sub drivers are:
+
+		GPIO: mfx-gpio
+
 menu "Multimedia Capabilities Port drivers"
 	depends on ARCH_SA1100
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index d9474ad..1379a18 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -230,3 +230,4 @@ obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
 obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
 obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
 obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
+obj-$(CONFIG_MFD_ST_MFX) 	+= st-mfx.o
diff --git a/drivers/mfd/st-mfx.c b/drivers/mfd/st-mfx.c
new file mode 100644
index 0000000..5bee5d3
--- /dev/null
+++ b/drivers/mfd/st-mfx.c
@@ -0,0 +1,526 @@
+/*
+ * STMicroelectronics Multi-Function eXpander (ST-MFX) Core Driver
+ *
+ * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
+ * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.
+ *
+ * License terms: GPL V2.0.
+ *
+ * st-mfx Core Driver is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * st-mfx Core Driver is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * st-mfx Core Driver. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/bitfield.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/st-mfx.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+
+/**
+ * struct mfx_priv - MFX MFD private structure
+ * @dev: device, mostly for logs
+ * @regmap: register map
+ * @mfx: MFX MFD public structure, to share information with subdrivers
+ * @irq_domain: IRQ domain
+ * @irq: IRQ number for mfx
+ * @irq_lock: IRQ bus lock
+ * @irqen: cache of IRQ_SRC_EN register for bus_lock
+ * @oldirqen: cache of IRQ_SRC_EN register for bus_lock
+ */
+struct mfx_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct mfx mfx;
+	struct irq_domain *irq_domain;
+	int irq;
+	struct mutex irq_lock; /* IRQ bus lock */
+	u8 irqen;
+	u8 oldirqen;
+};
+
+#define to_mfx_priv(_mfx) container_of(_mfx, struct mfx_priv, mfx)
+
+/* MFX boot time is around 10ms, so after reset, we have to wait this delay */
+#define MFX_BOOT_TIME 10
+
+static u8 mfx_blocks_to_mask(u32 blocks)
+{
+	u8 mask = 0;
+
+	if (blocks & MFX_BLOCK_GPIO)
+		mask |= MFX_REG_SYS_CTRL_GPIO_EN;
+	else
+		mask &= ~MFX_REG_SYS_CTRL_GPIO_EN;
+
+	if (blocks & MFX_BLOCK_TS)
+		mask |= MFX_REG_SYS_CTRL_TS_EN;
+	else
+		mask &= ~MFX_REG_SYS_CTRL_TS_EN;
+
+	if (blocks & MFX_BLOCK_IDD)
+		mask |= MFX_REG_SYS_CTRL_IDD_EN;
+	else
+		mask &= ~MFX_REG_SYS_CTRL_IDD_EN;
+
+	if (blocks & MFX_BLOCK_ALTGPIO)
+		mask |= MFX_REG_SYS_CTRL_ALTGPIO_EN;
+	else
+		mask &= ~MFX_REG_SYS_CTRL_ALTGPIO_EN;
+
+	return mask;
+}
+
+static int mfx_reset(struct mfx *mfx)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
+	int ret;
+
+	ret = regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
+				 MFX_REG_SYS_CTRL_SWRST,
+				 MFX_REG_SYS_CTRL_SWRST);
+
+	if (ret < 0)
+		return ret;
+
+	msleep(MFX_BOOT_TIME);
+
+	return ret;
+}
+
+int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
+
+	return regmap_bulk_read(mfx_priv->regmap, reg, values, length);
+}
+EXPORT_SYMBOL_GPL(mfx_block_read);
+
+int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
+
+	return regmap_bulk_write(mfx_priv->regmap, reg, values, length);
+}
+EXPORT_SYMBOL_GPL(mfx_block_write);
+
+int mfx_reg_read(struct mfx *mfx, u8 reg)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
+	u32 val;
+	int ret;
+
+	ret = regmap_read(mfx_priv->regmap, reg, &val);
+
+	return ret ? ret : val;
+}
+EXPORT_SYMBOL_GPL(mfx_reg_read);
+
+int mfx_reg_write(struct mfx *mfx, u8 reg, u8 val)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
+
+	return regmap_write(mfx_priv->regmap, reg, val);
+}
+EXPORT_SYMBOL_GPL(mfx_reg_write);
+
+int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
+
+	return regmap_update_bits(mfx_priv->regmap, reg, mask, val);
+}
+EXPORT_SYMBOL_GPL(mfx_set_bits);
+
+int mfx_enable(struct mfx *mfx, u32 blocks)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
+	u8 mask = mfx_blocks_to_mask(blocks);
+
+	return regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
+				  mask, mask);
+}
+EXPORT_SYMBOL_GPL(mfx_enable);
+
+int mfx_disable(struct mfx *mfx, u32 blocks)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
+	u8 mask = mfx_blocks_to_mask(blocks);
+
+	return regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
+				  mask, 0);
+}
+EXPORT_SYMBOL_GPL(mfx_disable);
+
+static void mfx_irq_lock(struct irq_data *data)
+{
+	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
+
+	mutex_lock(&mfx_priv->irq_lock);
+}
+
+static void mfx_irq_sync_unlock(struct irq_data *data)
+{
+	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
+	u8 new = mfx_priv->irqen;
+	u8 old = mfx_priv->oldirqen;
+
+	if (new == old)
+		goto unlock;
+
+	mfx_priv->oldirqen = new;
+	mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_SRC_EN, new);
+unlock:
+	mutex_unlock(&mfx_priv->irq_lock);
+}
+
+static void mfx_irq_mask(struct irq_data *data)
+{
+	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
+
+	mfx_priv->irqen &= ~BIT(data->hwirq % 8);
+}
+
+static void mfx_irq_unmask(struct irq_data *data)
+{
+	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
+
+	mfx_priv->irqen |= BIT(data->hwirq % 8);
+}
+
+static struct irq_chip mfx_irq_chip = {
+	.name			= "mfx",
+	.irq_bus_lock		= mfx_irq_lock,
+	.irq_bus_sync_unlock	= mfx_irq_sync_unlock,
+	.irq_mask		= mfx_irq_mask,
+	.irq_unmask		= mfx_irq_unmask,
+};
+
+static irqreturn_t mfx_irq(int irq, void *data)
+{
+	struct mfx_priv *mfx_priv = data;
+	unsigned long status, bit;
+	u8 ack;
+	int ret;
+
+	ret = mfx_reg_read(&mfx_priv->mfx, MFX_REG_IRQ_PENDING);
+	if (ret < 0) {
+		dev_err(mfx_priv->dev, "can't get IRQ_PENDING: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	/* It can be GPIO, IDD, ERROR, TS* IRQs */
+	status = ret & mfx_priv->irqen;
+
+	/*
+	 * There is no ACK for GPIO, MFX_REG_IRQ_PENDING_GPIO is a logical OR
+	 * of MFX_REG_IRQ_GPI _PENDING1/_PENDING2/_PENDING3
+	 */
+	ack = status & ~MFX_REG_IRQ_PENDING_GPIO;
+
+	for_each_set_bit(bit, &status, 8)
+		handle_nested_irq(irq_find_mapping(mfx_priv->irq_domain, bit));
+
+	if (ack)
+		mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_ACK, ack);
+
+	return IRQ_HANDLED;
+}
+
+static int mfx_irq_map(struct irq_domain *d, unsigned int virq,
+		       irq_hw_number_t hwirq)
+{
+	struct mfx_priv *mfx_priv = d->host_data;
+
+	irq_set_chip_data(virq, mfx_priv);
+	irq_set_chip(virq, &mfx_irq_chip);
+	irq_set_nested_thread(virq, 1);
+	irq_set_parent(virq, mfx_priv->irq);
+	irq_set_noprobe(virq);
+
+	return 0;
+}
+
+static void mfx_irq_unmap(struct irq_domain *d, unsigned int virq)
+{
+	irq_set_chip_and_handler(virq, NULL, NULL);
+	irq_set_chip_data(virq, NULL);
+}
+
+static const struct irq_domain_ops mfx_irq_ops = {
+	.map = mfx_irq_map,
+	.unmap = mfx_irq_unmap,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static int mfx_irq_init(struct mfx_priv *mfx_priv, struct device_node *np)
+{
+	int irqoutpin = MFX_REG_IRQ_OUT_PIN_TYPE; /* Push-Pull */
+	int irqtrigger, ret;
+
+	mfx_priv->irq = of_irq_get(np, 0);
+	if (mfx_priv->irq > 0) {
+		irqtrigger = irq_get_trigger_type(mfx_priv->irq);
+	} else {
+		dev_err(mfx_priv->dev, "failed to get irq: %d\n",
+			mfx_priv->irq);
+		return mfx_priv->irq;
+	}
+
+	if ((irqtrigger & IRQ_TYPE_EDGE_FALLING) ||
+	    (irqtrigger & IRQ_TYPE_LEVEL_LOW))
+		irqoutpin &= ~MFX_REG_IRQ_OUT_PIN_POL; /* Active Low */
+	else
+		irqoutpin |= MFX_REG_IRQ_OUT_PIN_POL; /* Active High */
+
+	mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_OUT_PIN, irqoutpin);
+
+	mfx_priv->irq_domain = irq_domain_add_linear(np, MFX_IRQ_SRC_NR,
+						     &mfx_irq_ops, mfx_priv);
+	if (!mfx_priv->irq_domain) {
+		dev_err(mfx_priv->dev, "failed to create irq domain\n");
+		return -ENOMEM;
+	}
+
+	ret = devm_request_threaded_irq(mfx_priv->dev, mfx_priv->irq,
+					NULL, mfx_irq,
+					irqtrigger | IRQF_ONESHOT, "mfx",
+					mfx_priv);
+	if (ret) {
+		dev_err(mfx_priv->dev, "failed to request irq: %d\n", ret);
+		irq_domain_remove(mfx_priv->irq_domain);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void mfx_irq_remove(struct mfx_priv *mfx_priv)
+{
+	int hwirq;
+
+	for (hwirq = 0; hwirq < MFX_IRQ_SRC_NR; hwirq++)
+		irq_dispose_mapping(irq_find_mapping(mfx_priv->irq_domain,
+						     hwirq));
+	irq_domain_remove(mfx_priv->irq_domain);
+}
+
+static int mfx_chip_init(struct mfx_priv *mfx_priv, u16 i2c_addr)
+{
+	struct mfx *mfx = &mfx_priv->mfx;
+	int ret;
+	int id;
+	u8 version[2];
+
+	id = mfx_reg_read(mfx, MFX_REG_CHIP_ID);
+	if (id < 0) {
+		dev_err(mfx_priv->dev, "error reading chip id: %d\n", id);
+		return id;
+	}
+
+	ret = mfx_block_read(mfx, MFX_REG_FW_VERSION_MSB,
+			     ARRAY_SIZE(version), version);
+	if (ret < 0) {
+		dev_err(mfx_priv->dev, "error reading fw version: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Check that ID is the complement of the I2C address:
+	 * MFX I2C address follows the 7-bit format (MSB), that's why
+	 * i2c_addr is shifted.
+	 *
+	 * MFX_I2C_ADDR |         MFX         |        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(MFX_REG_CHIP_ID_MASK, ~id) != (i2c_addr << 1)) {
+		dev_err(mfx_priv->dev, "unknown chip id: %#x\n", id);
+		return -EINVAL;
+	}
+
+	dev_info(mfx_priv->dev, "ST-MFX chip id: %#x, fw version: %x.%02x\n",
+		 id, version[0], version[1]);
+
+	/* Disable all features, subdrivers should enable what they need */
+	ret = mfx_disable(mfx, ~0);
+	if (ret)
+		return ret;
+
+	return mfx_reset(mfx);
+}
+
+static const struct regmap_config mfx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct of_device_id mfx_of_match[] = {
+	{ .compatible = "st,mfx" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mfx_of_match);
+
+static int mfx_of_probe(struct device_node *np, struct mfx_priv *mfx_priv)
+{
+	struct device_node *child;
+
+	if (!np)
+		return -ENODEV;
+
+	for_each_child_of_node(np, child) {
+		if (of_device_is_compatible(child, "st,mfx-gpio")) {
+			mfx_priv->mfx.blocks |= MFX_BLOCK_GPIO;
+			mfx_priv->mfx.num_gpio = 16;
+		}
+		/*
+		 * Here we could find other children like "st,mfx-ts" or
+		 * "st,mfx-idd.
+		 */
+	}
+
+	if (!(mfx_priv->mfx.blocks & MFX_BLOCK_TS) &&
+	    !(mfx_priv->mfx.blocks & MFX_BLOCK_IDD)) {
+		mfx_priv->mfx.blocks |= MFX_BLOCK_ALTGPIO;
+		mfx_priv->mfx.num_gpio += 8;
+	}
+
+	/*
+	 * TODO: aGPIO[3:0] and aGPIO[7:4] can be used independently:
+	 * - if IDD is used but not TS, aGPIO[3:0] can be used as GPIO,
+	 * - if TS is used but not IDD: aGPIO[7:4] can be used as GPIO.
+	 */
+
+	return of_platform_populate(np, NULL, NULL, mfx_priv->dev);
+}
+
+int mfx_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct device_node *np = client->dev.of_node;
+	struct mfx_priv *mfx_priv;
+	int ret;
+
+	mfx_priv = devm_kzalloc(&client->dev, sizeof(struct mfx_priv),
+				GFP_KERNEL);
+	if (!mfx_priv)
+		return -ENOMEM;
+
+	mfx_priv->regmap = devm_regmap_init_i2c(client, &mfx_regmap_config);
+	if (IS_ERR(mfx_priv->regmap)) {
+		ret = PTR_ERR(mfx_priv->regmap);
+		dev_err(&client->dev, "failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+
+	mfx_priv->dev = &client->dev;
+	i2c_set_clientdata(client, &mfx_priv->mfx);
+
+	mutex_init(&mfx_priv->irq_lock);
+
+	ret = mfx_chip_init(mfx_priv, client->addr);
+	if (ret) {
+		if (ret == -ETIMEDOUT)
+			return -EPROBE_DEFER;
+
+		return ret;
+	}
+
+	ret = mfx_irq_init(mfx_priv, np);
+	if (ret < 0)
+		return ret;
+
+	ret = mfx_of_probe(np, mfx_priv);
+	if (ret < 0)
+		return ret;
+
+	dev_info(mfx_priv->dev, "ST-MFX (CORE) initialized\n");
+
+	return 0;
+}
+
+static int mfx_remove(struct i2c_client *client)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(i2c_get_clientdata(client));
+
+	mfx_irq_remove(mfx_priv);
+	of_platform_depopulate(mfx_priv->dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int mfx_suspend(struct device *dev)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(dev_get_drvdata(dev));
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(mfx_priv->irq);
+
+	/*
+	 * TODO: Do we put MFX in STANDBY mode ?
+	 * (Wakeup by rising edge on MFX_wakeup pin)
+	 */
+
+	return 0;
+}
+
+static int mfx_resume(struct device *dev)
+{
+	struct mfx_priv *mfx_priv = to_mfx_priv(dev_get_drvdata(dev));
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(mfx_priv->irq);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(mfx_dev_pm_ops, mfx_suspend, mfx_resume);
+
+static const struct i2c_device_id mfx_i2c_id[] = {
+	{ "mfx", },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, mfx_id);
+
+static struct i2c_driver mfx_driver = {
+	.driver = {
+		.name = "st-mfx",
+		.pm = &mfx_dev_pm_ops,
+		.of_match_table = mfx_of_match,
+	},
+	.probe = mfx_probe,
+	.remove = mfx_remove,
+	.id_table = mfx_i2c_id,
+};
+
+static int __init mfx_init(void)
+{
+	return i2c_add_driver(&mfx_driver);
+}
+subsys_initcall(mfx_init);
+
+static void __exit mfx_exit(void)
+{
+	i2c_del_driver(&mfx_driver);
+}
+module_exit(mfx_exit);
+
+MODULE_DESCRIPTION("STMicroelectronics Multi-Function eXpander MFD core driver");
+MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/st-mfx.h b/include/linux/mfd/st-mfx.h
new file mode 100644
index 0000000..1bf7e65
--- /dev/null
+++ b/include/linux/mfd/st-mfx.h
@@ -0,0 +1,116 @@
+/*
+ * STMicroelectronics Multi-Function eXpander (ST-MFX)
+ *
+ * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
+ * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.
+ *
+ * License terms: GPL V2.0.
+ *
+ * st-mfx is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * st-mfx is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * st-mfx. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __MFD_ST_MFX_H
+#define __MFD_ST_MFX_H
+
+enum mfx_block {
+	MFX_BLOCK_GPIO		= BIT(0),
+	MFX_BLOCK_TS		= BIT(1),
+	MFX_BLOCK_IDD		= BIT(2),
+	MFX_BLOCK_ALTGPIO	= BIT(3),
+};
+
+/*
+ * 8 events can activate the MFX_IRQ_OUT signal,
+ * but for the moment, only GPIO event is used
+ */
+enum mfx_irq {
+	MFX_IRQ_SRC_GPIO,
+
+	MFX_IRQ_SRC_NR,
+};
+
+/**
+ * struct mfx - MFX MFD structure
+ * @blocks: mask of mfx_block to be enabled
+ * @num_gpio: number of gpios
+ */
+struct mfx {
+	u32 blocks;
+	u32 num_gpio;
+};
+
+int mfx_reg_write(struct mfx *mfx, u8 reg, u8 data);
+int mfx_reg_read(struct mfx *mfx, u8 reg);
+int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values);
+int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values);
+int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val);
+int mfx_enable(struct mfx *mfx, unsigned int blocks);
+int mfx_disable(struct mfx *mfx, unsigned int blocks);
+
+/* General */
+#define MFX_REG_CHIP_ID			0x00 /* R */
+#define MFX_REG_FW_VERSION_MSB		0x01 /* R */
+#define MFX_REG_FW_VERSION_LSB		0x02 /* R */
+#define MFX_REG_SYS_CTRL		0x40 /* RW */
+/* IRQ output management */
+#define MFX_REG_IRQ_OUT_PIN		0x41 /* RW */
+#define MFX_REG_IRQ_SRC_EN		0x42 /* RW */
+#define MFX_REG_IRQ_PENDING		0x08 /* R */
+#define MFX_REG_IRQ_ACK			0x44 /* RW */
+/* GPIOs expander */
+/* GPIO_STATE1 0x10, GPIO_STATE2 0x11, GPIO_STATE3 0x12 */
+#define MFX_REG_GPIO_STATE		0x10 /* R */
+/* GPIO_DIR1 0x60, GPIO_DIR2 0x61, GPIO_DIR3 0x63 */
+#define MFX_REG_GPIO_DIR		0x60 /* RW */
+/* GPIO_TYPE1 0x64, GPIO_TYPE2 0x65, GPIO_TYPE3 0x66 */
+#define MFX_REG_GPIO_TYPE		0x64 /* RW */
+/* GPIO_PUPD1 0x68, GPIO_PUPD2 0x69, GPIO_PUPD3 0x6A */
+#define MFX_REG_GPIO_PUPD		0x68 /* RW */
+/* GPO_SET1 0x6C, GPO_SET2 0x6D, GPO_SET3 0x6E */
+#define MFX_REG_GPO_SET			0x6C /* RW */
+/* GPO_CLR1 0x70, GPO_CLR2 0x71, GPO_CLR3 0x72 */
+#define MFX_REG_GPO_CLR			0x70 /* RW */
+/* IRQ_GPI_SRC1 0x48, IRQ_GPI_SRC2 0x49, IRQ_GPI_SRC3 0x4A */
+#define MFX_REG_IRQ_GPI_SRC		0x48 /* RW */
+/* IRQ_GPI_EVT1 0x4C, IRQ_GPI_EVT2 0x4D, IRQ_GPI_EVT3 0x4E */
+#define MFX_REG_IRQ_GPI_EVT		0x4C /* RW */
+/* IRQ_GPI_TYPE1 0x50, IRQ_GPI_TYPE2 0x51, IRQ_GPI_TYPE3 0x52 */
+#define MFX_REG_IRQ_GPI_TYPE		0x50 /* RW */
+/* IRQ_GPI_PENDING1 0x0C, IRQ_GPI_PENDING2 0x0D, IRQ_GPI_PENDING3 0x0E*/
+#define MFX_REG_IRQ_GPI_PENDING		0x0C /* R */
+/* IRQ_GPI_ACK1 0x54, IRQ_GPI_ACK2 0x55, IRQ_GPI_ACK3 0x56 */
+#define MFX_REG_IRQ_GPI_ACK		0x54 /* RW */
+
+/* MFX_REG_CHIP_ID bitfields */
+#define MFX_REG_CHIP_ID_MASK		GENMASK(7, 0)
+
+/* MFX_REG_SYS_CTRL bitfields */
+#define MFX_REG_SYS_CTRL_GPIO_EN	BIT(0)
+#define MFX_REG_SYS_CTRL_TS_EN		BIT(1)
+#define MFX_REG_SYS_CTRL_IDD_EN		BIT(2)
+#define MFX_REG_SYS_CTRL_ALTGPIO_EN	BIT(3)
+#define MFX_REG_SYS_CTRL_STANDBY	BIT(6)
+#define MFX_REG_SYS_CTRL_SWRST		BIT(7)
+
+/* MFX_REG_IRQ_OUT_PIN bitfields */
+#define MFX_REG_IRQ_OUT_PIN_TYPE	BIT(0) /* 0-OD 1-PP */
+#define MFX_REG_IRQ_OUT_PIN_POL		BIT(1) /* 0-active LOW 1-active HIGH */
+
+/* MFX_REG_IRQ_SRC_EN bitfields */
+#define MFX_REG_IRQ_SRC_EN_GPIO		BIT(0)
+
+/* MFX_REG_IRQ_PENDING bitfields */
+#define MFX_REG_IRQ_PENDING_GPIO	BIT(0)
+
+#endif /* __MFD_ST_MFX_H */
+
-- 
2.7.4

Re: [PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver

[Lee Jones]

On Thu, 08 Feb 2018, Amelie Delaunay wrote:

> ST Multi-Function eXpander (MFX) is a slave controller using I2C
> for communication with the main MCU. Main features are:
> - 16 fast GPIOs individually configurable in input/output
> - 8 alternate GPIOs individually configurable in input/output
> - Main MCU IDD measurement
> - Resistive touchscreen controller
> 
> Only GPIO expander (16 fast GPIOs + 8 alternate) feature is
> supported for the moment.
> 
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> ---
>  drivers/mfd/Kconfig        |  15 ++
>  drivers/mfd/Makefile       |   1 +
>  drivers/mfd/st-mfx.c       | 526 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/st-mfx.h | 116 ++++++++++
>  4 files changed, 658 insertions(+)
>  create mode 100644 drivers/mfd/st-mfx.c
>  create mode 100644 include/linux/mfd/st-mfx.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1d20a80..e78e818 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1823,6 +1823,21 @@ config MFD_STM32_TIMERS
>  	  for PWM and IIO Timer. This driver allow to share the
>  	  registers between the others drivers.
>  
> +config MFD_ST_MFX
> +	bool "STMicroelectronics MFX"
> +	depends on I2C
> +	depends on OF || COMPILE_TEST
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	help
> +	  Support for the STMicroelectronics Multi-Function eXpander.

Is that really what this device is called?

Is there a datasheet?

> +	  This driver provides common support for accessing the device,
> +	  additional drivers must be enabled in order to use the functionality
> +	  of the device. Currently available sub drivers are:
> +
> +		GPIO: mfx-gpio

This driver would be easier to review if I could picture the device as
a whole.  What other functionality does it have?  What is it that
makes this an MFD?

>  menu "Multimedia Capabilities Port drivers"
>  	depends on ARCH_SA1100
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index d9474ad..1379a18 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -230,3 +230,4 @@ obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
>  obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
>  obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
>  obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
> +obj-$(CONFIG_MFD_ST_MFX) 	+= st-mfx.o
> diff --git a/drivers/mfd/st-mfx.c b/drivers/mfd/st-mfx.c
> new file mode 100644
> index 0000000..5bee5d3
> --- /dev/null
> +++ b/drivers/mfd/st-mfx.c
> @@ -0,0 +1,526 @@
> +/*
> + * STMicroelectronics Multi-Function eXpander (ST-MFX) Core Driver
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.

You don't need to put "for STMicroelectronics".  This was something we
made up when submitting from a different (!st.com) email address.

> + * License terms: GPL V2.0.
> + *
> + * st-mfx Core Driver is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * st-mfx Core Driver is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * st-mfx Core Driver. If not, see <http://www.gnu.org/licenses/>.

You should be able to use the short version of the licensing
agreement.  Also, please grep for "SPDX".

> + */
> +#include <linux/bitfield.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/st-mfx.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/regmap.h>
> +
> +/**
> + * struct mfx_priv - MFX MFD private structure
> + * @dev: device, mostly for logs
> + * @regmap: register map
> + * @mfx: MFX MFD public structure, to share information with subdrivers
> + * @irq_domain: IRQ domain
> + * @irq: IRQ number for mfx
> + * @irq_lock: IRQ bus lock
> + * @irqen: cache of IRQ_SRC_EN register for bus_lock
> + * @oldirqen: cache of IRQ_SRC_EN register for bus_lock
> + */

/**
 * struct mfx_priv - MFX MFD private structure
 * @dev:  	   device, mostly for logs
 * @regmap: 	   register map
 * @mfx: 	   MFX MFD public structure, to share information with subdrivers
 * @irq_domain:    IRQ domain
 * @irq: 	   IRQ number for mfx
 * @irq_lock: 	   IRQ bus lock
 * @irqen: 	   cache of IRQ_SRC_EN register for bus_lock
 * @oldirqen: 	   cache of IRQ_SRC_EN register for bus_lock
 */

Easier to read?

> +struct mfx_priv {

mfx_ddata

> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct mfx mfx;
> +	struct irq_domain *irq_domain;
> +	int irq;
> +	struct mutex irq_lock; /* IRQ bus lock */
> +	u8 irqen;
> +	u8 oldirqen;
> +};
> +
> +#define to_mfx_priv(_mfx) container_of(_mfx, struct mfx_priv, mfx)

FYI: I don't like this idea.  More to follow.

> +/* MFX boot time is around 10ms, so after reset, we have to wait this delay */
> +#define MFX_BOOT_TIME 10

MFX_BOOT_TIME_MS

> +static u8 mfx_blocks_to_mask(u32 blocks)

I think it would be better to prepend a vendor tag to everything too.

  st_mfx_*

> +{
> +	u8 mask = 0;
> +
> +	if (blocks & MFX_BLOCK_GPIO)
> +		mask |= MFX_REG_SYS_CTRL_GPIO_EN;
> +	else
> +		mask &= ~MFX_REG_SYS_CTRL_GPIO_EN;
> +
> +	if (blocks & MFX_BLOCK_TS)
> +		mask |= MFX_REG_SYS_CTRL_TS_EN;
> +	else
> +		mask &= ~MFX_REG_SYS_CTRL_TS_EN;
> +
> +	if (blocks & MFX_BLOCK_IDD)
> +		mask |= MFX_REG_SYS_CTRL_IDD_EN;
> +	else
> +		mask &= ~MFX_REG_SYS_CTRL_IDD_EN;
> +
> +	if (blocks & MFX_BLOCK_ALTGPIO)
> +		mask |= MFX_REG_SYS_CTRL_ALTGPIO_EN;
> +	else
> +		mask &= ~MFX_REG_SYS_CTRL_ALTGPIO_EN;
> +
> +	return mask;
> +}
> +
> +static int mfx_reset(struct mfx *mfx)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +	int ret;
> +
> +	ret = regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
> +				 MFX_REG_SYS_CTRL_SWRST,
> +				 MFX_REG_SYS_CTRL_SWRST);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(MFX_BOOT_TIME);
> +
> +	return ret;
> +}
> +
> +int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +
> +	return regmap_bulk_read(mfx_priv->regmap, reg, values, length);
> +}
> +EXPORT_SYMBOL_GPL(mfx_block_read);
> +
> +int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +
> +	return regmap_bulk_write(mfx_priv->regmap, reg, values, length);
> +}
> +EXPORT_SYMBOL_GPL(mfx_block_write);
> +
> +int mfx_reg_read(struct mfx *mfx, u8 reg)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +	u32 val;
> +	int ret;
> +
> +	ret = regmap_read(mfx_priv->regmap, reg, &val);
> +
> +	return ret ? ret : val;
> +}
> +EXPORT_SYMBOL_GPL(mfx_reg_read);
> +
> +int mfx_reg_write(struct mfx *mfx, u8 reg, u8 val)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +
> +	return regmap_write(mfx_priv->regmap, reg, val);
> +}
> +EXPORT_SYMBOL_GPL(mfx_reg_write);
> +
> +int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +
> +	return regmap_update_bits(mfx_priv->regmap, reg, mask, val);
> +}
> +EXPORT_SYMBOL_GPL(mfx_set_bits);
> +
> +int mfx_enable(struct mfx *mfx, u32 blocks)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +	u8 mask = mfx_blocks_to_mask(blocks);
> +
> +	return regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
> +				  mask, mask);
> +}
> +EXPORT_SYMBOL_GPL(mfx_enable);
> +
> +int mfx_disable(struct mfx *mfx, u32 blocks)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
> +	u8 mask = mfx_blocks_to_mask(blocks);
> +
> +	return regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
> +				  mask, 0);
> +}
> +EXPORT_SYMBOL_GPL(mfx_disable);

The remap infrastructure doesn't need further abstraction.  Please
pass the pointer to any child devices and have them use it directly.

> +static void mfx_irq_lock(struct irq_data *data)
> +{
> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
> +
> +	mutex_lock(&mfx_priv->irq_lock);
> +}
> +
> +static void mfx_irq_sync_unlock(struct irq_data *data)
> +{
> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
> +	u8 new = mfx_priv->irqen;
> +	u8 old = mfx_priv->oldirqen;
> +
> +	if (new == old)
> +		goto unlock;
> +
> +	mfx_priv->oldirqen = new;
> +	mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_SRC_EN, new);
> +unlock:
> +	mutex_unlock(&mfx_priv->irq_lock);
> +}
> +
> +static void mfx_irq_mask(struct irq_data *data)
> +{
> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
> +
> +	mfx_priv->irqen &= ~BIT(data->hwirq % 8);
> +}
> +
> +static void mfx_irq_unmask(struct irq_data *data)
> +{
> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
> +
> +	mfx_priv->irqen |= BIT(data->hwirq % 8);
> +}
> +
> +static struct irq_chip mfx_irq_chip = {
> +	.name			= "mfx",
> +	.irq_bus_lock		= mfx_irq_lock,
> +	.irq_bus_sync_unlock	= mfx_irq_sync_unlock,
> +	.irq_mask		= mfx_irq_mask,
> +	.irq_unmask		= mfx_irq_unmask,
> +};
> +
> +static irqreturn_t mfx_irq(int irq, void *data)
> +{
> +	struct mfx_priv *mfx_priv = data;

'ddata' is a more consistent naming approach.

> +	unsigned long status, bit;
> +	u8 ack;
> +	int ret;
> +
> +	ret = mfx_reg_read(&mfx_priv->mfx, MFX_REG_IRQ_PENDING);
> +	if (ret < 0) {
> +		dev_err(mfx_priv->dev, "can't get IRQ_PENDING: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	/* It can be GPIO, IDD, ERROR, TS* IRQs */
> +	status = ret & mfx_priv->irqen;
> +
> +	/*
> +	 * There is no ACK for GPIO, MFX_REG_IRQ_PENDING_GPIO is a logical OR
> +	 * of MFX_REG_IRQ_GPI _PENDING1/_PENDING2/_PENDING3
> +	 */
> +	ack = status & ~MFX_REG_IRQ_PENDING_GPIO;
> +
> +	for_each_set_bit(bit, &status, 8)
> +		handle_nested_irq(irq_find_mapping(mfx_priv->irq_domain, bit));
> +
> +	if (ack)
> +		mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_ACK, ack);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mfx_irq_map(struct irq_domain *d, unsigned int virq,
> +		       irq_hw_number_t hwirq)
> +{
> +	struct mfx_priv *mfx_priv = d->host_data;
> +
> +	irq_set_chip_data(virq, mfx_priv);
> +	irq_set_chip(virq, &mfx_irq_chip);
> +	irq_set_nested_thread(virq, 1);
> +	irq_set_parent(virq, mfx_priv->irq);
> +	irq_set_noprobe(virq);
> +
> +	return 0;
> +}
> +
> +static void mfx_irq_unmap(struct irq_domain *d, unsigned int virq)
> +{
> +	irq_set_chip_and_handler(virq, NULL, NULL);
> +	irq_set_chip_data(virq, NULL);
> +}
> +
> +static const struct irq_domain_ops mfx_irq_ops = {
> +	.map = mfx_irq_map,
> +	.unmap = mfx_irq_unmap,
> +	.xlate = irq_domain_xlate_onecell,
> +};
> +
> +static int mfx_irq_init(struct mfx_priv *mfx_priv, struct device_node *np)
> +{
> +	int irqoutpin = MFX_REG_IRQ_OUT_PIN_TYPE; /* Push-Pull */
> +	int irqtrigger, ret;
> +
> +	mfx_priv->irq = of_irq_get(np, 0);
> +	if (mfx_priv->irq > 0) {
> +		irqtrigger = irq_get_trigger_type(mfx_priv->irq);
> +	} else {
> +		dev_err(mfx_priv->dev, "failed to get irq: %d\n",
> +			mfx_priv->irq);
> +		return mfx_priv->irq;
> +	}
> +
> +	if ((irqtrigger & IRQ_TYPE_EDGE_FALLING) ||
> +	    (irqtrigger & IRQ_TYPE_LEVEL_LOW))
> +		irqoutpin &= ~MFX_REG_IRQ_OUT_PIN_POL; /* Active Low */
> +	else
> +		irqoutpin |= MFX_REG_IRQ_OUT_PIN_POL; /* Active High */
> +
> +	mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_OUT_PIN, irqoutpin);
> +
> +	mfx_priv->irq_domain = irq_domain_add_linear(np, MFX_IRQ_SRC_NR,
> +						     &mfx_irq_ops, mfx_priv);
> +	if (!mfx_priv->irq_domain) {
> +		dev_err(mfx_priv->dev, "failed to create irq domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = devm_request_threaded_irq(mfx_priv->dev, mfx_priv->irq,
> +					NULL, mfx_irq,
> +					irqtrigger | IRQF_ONESHOT, "mfx",
> +					mfx_priv);
> +	if (ret) {
> +		dev_err(mfx_priv->dev, "failed to request irq: %d\n", ret);
> +		irq_domain_remove(mfx_priv->irq_domain);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mfx_irq_remove(struct mfx_priv *mfx_priv)
> +{
> +	int hwirq;
> +
> +	for (hwirq = 0; hwirq < MFX_IRQ_SRC_NR; hwirq++)
> +		irq_dispose_mapping(irq_find_mapping(mfx_priv->irq_domain,
> +						     hwirq));
> +	irq_domain_remove(mfx_priv->irq_domain);
> +}

Is there any reason why this IRQ stuff can't live in the GPIO driver?

> +static int mfx_chip_init(struct mfx_priv *mfx_priv, u16 i2c_addr)
> +{
> +	struct mfx *mfx = &mfx_priv->mfx;
> +	int ret;
> +	int id;
> +	u8 version[2];
> +
> +	id = mfx_reg_read(mfx, MFX_REG_CHIP_ID);
> +	if (id < 0) {
> +		dev_err(mfx_priv->dev, "error reading chip id: %d\n", id);
> +		return id;
> +	}
> +
> +	ret = mfx_block_read(mfx, MFX_REG_FW_VERSION_MSB,
> +			     ARRAY_SIZE(version), version);
> +	if (ret < 0) {
> +		dev_err(mfx_priv->dev, "error reading fw version: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Check that ID is the complement of the I2C address:
> +	 * MFX I2C address follows the 7-bit format (MSB), that's why
> +	 * i2c_addr is shifted.
> +	 *
> +	 * MFX_I2C_ADDR |         MFX         |        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(MFX_REG_CHIP_ID_MASK, ~id) != (i2c_addr << 1)) {
> +		dev_err(mfx_priv->dev, "unknown chip id: %#x\n", id);
> +		return -EINVAL;
> +	}
> +
> +	dev_info(mfx_priv->dev, "ST-MFX chip id: %#x, fw version: %x.%02x\n",
> +		 id, version[0], version[1]);
> +
> +	/* Disable all features, subdrivers should enable what they need */
> +	ret = mfx_disable(mfx, ~0);
> +	if (ret)
> +		return ret;
> +
> +	return mfx_reset(mfx);
> +}
> +
> +static const struct regmap_config mfx_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static const struct of_device_id mfx_of_match[] = {
> +	{ .compatible = "st,mfx" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mfx_of_match);
> +
> +static int mfx_of_probe(struct device_node *np, struct mfx_priv *mfx_priv)
> +{
> +	struct device_node *child;
> +
> +	if (!np)
> +		return -ENODEV;

Is this even possible?

Do you support !OF?

> +	for_each_child_of_node(np, child) {
> +		if (of_device_is_compatible(child, "st,mfx-gpio")) {
> +			mfx_priv->mfx.blocks |= MFX_BLOCK_GPIO;
> +			mfx_priv->mfx.num_gpio = 16;

This is ugly.

Where is num_gpio used?

> +		}
> +		/*
> +		 * Here we could find other children like "st,mfx-ts" or
> +		 * "st,mfx-idd.
> +		 */
> +	}
> +
> +	if (!(mfx_priv->mfx.blocks & MFX_BLOCK_TS) &&
> +	    !(mfx_priv->mfx.blocks & MFX_BLOCK_IDD)) {
> +		mfx_priv->mfx.blocks |= MFX_BLOCK_ALTGPIO;
> +		mfx_priv->mfx.num_gpio += 8;
> +	}

What are TS and IDD?

This is starting to look like a GPIO driver, and nothing more.

> +	/*
> +	 * TODO: aGPIO[3:0] and aGPIO[7:4] can be used independently:
> +	 * - if IDD is used but not TS, aGPIO[3:0] can be used as GPIO,
> +	 * - if TS is used but not IDD: aGPIO[7:4] can be used as GPIO.
> +	 */
> +
> +	return of_platform_populate(np, NULL, NULL, mfx_priv->dev);
> +}
> +
> +int mfx_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct device_node *np = client->dev.of_node;
> +	struct mfx_priv *mfx_priv;
> +	int ret;
> +
> +	mfx_priv = devm_kzalloc(&client->dev, sizeof(struct mfx_priv),
> +				GFP_KERNEL);
> +	if (!mfx_priv)
> +		return -ENOMEM;
> +
> +	mfx_priv->regmap = devm_regmap_init_i2c(client, &mfx_regmap_config);
> +	if (IS_ERR(mfx_priv->regmap)) {
> +		ret = PTR_ERR(mfx_priv->regmap);
> +		dev_err(&client->dev, "failed to allocate register map: %d\n",
> +			ret);

Nit: Prefer if you broke the line after '&client->dev,'.

> +		return ret;
> +	}
> +
> +	mfx_priv->dev = &client->dev;


> +	i2c_set_clientdata(client, &mfx_priv->mfx);
> +
> +	mutex_init(&mfx_priv->irq_lock);
> +
> +	ret = mfx_chip_init(mfx_priv, client->addr);
> +	if (ret) {
> +		if (ret == -ETIMEDOUT)
> +			return -EPROBE_DEFER;

Return -EPROBE_DEFER from reset() instead.

> +		return ret;
> +	}
> +
> +	ret = mfx_irq_init(mfx_priv, np);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mfx_of_probe(np, mfx_priv);
> +	if (ret < 0)
> +		return ret;

Is it possible to build with !OF?

If not, move all probe code into here.

> +	dev_info(mfx_priv->dev, "ST-MFX (CORE) initialized\n");

These kinds of messages are usually frowned upon.

> +	return 0;
> +}
> +
> +static int mfx_remove(struct i2c_client *client)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(i2c_get_clientdata(client));
> +
> +	mfx_irq_remove(mfx_priv);
> +	of_platform_depopulate(mfx_priv->dev);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mfx_suspend(struct device *dev)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(dev_get_drvdata(dev));
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(mfx_priv->irq);
> +
> +	/*
> +	 * TODO: Do we put MFX in STANDBY mode ?
> +	 * (Wakeup by rising edge on MFX_wakeup pin)
> +	 */

How long before you answer this question?

Better do just enable it right away or make a mental note and remove
the comment from the upstream driver.

> +	return 0;
> +}
> +
> +static int mfx_resume(struct device *dev)
> +{
> +	struct mfx_priv *mfx_priv = to_mfx_priv(dev_get_drvdata(dev));
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(mfx_priv->irq);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(mfx_dev_pm_ops, mfx_suspend, mfx_resume);
> +
> +static const struct i2c_device_id mfx_i2c_id[] = {
> +	{ "mfx", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, mfx_id);

I don't think this is required anymore.

> +static struct i2c_driver mfx_driver = {
> +	.driver = {
> +		.name = "st-mfx",
> +		.pm = &mfx_dev_pm_ops,
> +		.of_match_table = mfx_of_match,
> +	},
> +	.probe = mfx_probe,
> +	.remove = mfx_remove,
> +	.id_table = mfx_i2c_id,
> +};
> +
> +static int __init mfx_init(void)
> +{
> +	return i2c_add_driver(&mfx_driver);
> +}
> +subsys_initcall(mfx_init);
> +
> +static void __exit mfx_exit(void)
> +{
> +	i2c_del_driver(&mfx_driver);
> +}
> +module_exit(mfx_exit);
> +
> +MODULE_DESCRIPTION("STMicroelectronics Multi-Function eXpander MFD core driver");
> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/st-mfx.h b/include/linux/mfd/st-mfx.h
> new file mode 100644
> index 0000000..1bf7e65
> --- /dev/null
> +++ b/include/linux/mfd/st-mfx.h
> @@ -0,0 +1,116 @@
> +/*
> + * STMicroelectronics Multi-Function eXpander (ST-MFX)
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.
> + *
> + * License terms: GPL V2.0.
> + *
> + * st-mfx is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * st-mfx is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * st-mfx. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __MFD_ST_MFX_H
> +#define __MFD_ST_MFX_H
> +
> +enum mfx_block {
> +	MFX_BLOCK_GPIO		= BIT(0),
> +	MFX_BLOCK_TS		= BIT(1),
> +	MFX_BLOCK_IDD		= BIT(2),
> +	MFX_BLOCK_ALTGPIO	= BIT(3),
> +};
> +
> +/*
> + * 8 events can activate the MFX_IRQ_OUT signal,
> + * but for the moment, only GPIO event is used
> + */
> +enum mfx_irq {
> +	MFX_IRQ_SRC_GPIO,
> +
> +	MFX_IRQ_SRC_NR,
> +};
> +
> +/**
> + * struct mfx - MFX MFD structure
> + * @blocks: mask of mfx_block to be enabled
> + * @num_gpio: number of gpios
> + */
> +struct mfx {
> +	u32 blocks;
> +	u32 num_gpio;
> +};
> +
> +int mfx_reg_write(struct mfx *mfx, u8 reg, u8 data);
> +int mfx_reg_read(struct mfx *mfx, u8 reg);
> +int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values);
> +int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values);
> +int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val);
> +int mfx_enable(struct mfx *mfx, unsigned int blocks);
> +int mfx_disable(struct mfx *mfx, unsigned int blocks);
> +
> +/* General */
> +#define MFX_REG_CHIP_ID			0x00 /* R */
> +#define MFX_REG_FW_VERSION_MSB		0x01 /* R */
> +#define MFX_REG_FW_VERSION_LSB		0x02 /* R */
> +#define MFX_REG_SYS_CTRL		0x40 /* RW */
> +/* IRQ output management */
> +#define MFX_REG_IRQ_OUT_PIN		0x41 /* RW */
> +#define MFX_REG_IRQ_SRC_EN		0x42 /* RW */
> +#define MFX_REG_IRQ_PENDING		0x08 /* R */
> +#define MFX_REG_IRQ_ACK			0x44 /* RW */
> +/* GPIOs expander */
> +/* GPIO_STATE1 0x10, GPIO_STATE2 0x11, GPIO_STATE3 0x12 */
> +#define MFX_REG_GPIO_STATE		0x10 /* R */
> +/* GPIO_DIR1 0x60, GPIO_DIR2 0x61, GPIO_DIR3 0x63 */
> +#define MFX_REG_GPIO_DIR		0x60 /* RW */
> +/* GPIO_TYPE1 0x64, GPIO_TYPE2 0x65, GPIO_TYPE3 0x66 */
> +#define MFX_REG_GPIO_TYPE		0x64 /* RW */
> +/* GPIO_PUPD1 0x68, GPIO_PUPD2 0x69, GPIO_PUPD3 0x6A */
> +#define MFX_REG_GPIO_PUPD		0x68 /* RW */
> +/* GPO_SET1 0x6C, GPO_SET2 0x6D, GPO_SET3 0x6E */
> +#define MFX_REG_GPO_SET			0x6C /* RW */
> +/* GPO_CLR1 0x70, GPO_CLR2 0x71, GPO_CLR3 0x72 */
> +#define MFX_REG_GPO_CLR			0x70 /* RW */
> +/* IRQ_GPI_SRC1 0x48, IRQ_GPI_SRC2 0x49, IRQ_GPI_SRC3 0x4A */
> +#define MFX_REG_IRQ_GPI_SRC		0x48 /* RW */
> +/* IRQ_GPI_EVT1 0x4C, IRQ_GPI_EVT2 0x4D, IRQ_GPI_EVT3 0x4E */
> +#define MFX_REG_IRQ_GPI_EVT		0x4C /* RW */
> +/* IRQ_GPI_TYPE1 0x50, IRQ_GPI_TYPE2 0x51, IRQ_GPI_TYPE3 0x52 */
> +#define MFX_REG_IRQ_GPI_TYPE		0x50 /* RW */
> +/* IRQ_GPI_PENDING1 0x0C, IRQ_GPI_PENDING2 0x0D, IRQ_GPI_PENDING3 0x0E*/
> +#define MFX_REG_IRQ_GPI_PENDING		0x0C /* R */
> +/* IRQ_GPI_ACK1 0x54, IRQ_GPI_ACK2 0x55, IRQ_GPI_ACK3 0x56 */
> +#define MFX_REG_IRQ_GPI_ACK		0x54 /* RW */
> +
> +/* MFX_REG_CHIP_ID bitfields */
> +#define MFX_REG_CHIP_ID_MASK		GENMASK(7, 0)
> +
> +/* MFX_REG_SYS_CTRL bitfields */
> +#define MFX_REG_SYS_CTRL_GPIO_EN	BIT(0)
> +#define MFX_REG_SYS_CTRL_TS_EN		BIT(1)
> +#define MFX_REG_SYS_CTRL_IDD_EN		BIT(2)
> +#define MFX_REG_SYS_CTRL_ALTGPIO_EN	BIT(3)
> +#define MFX_REG_SYS_CTRL_STANDBY	BIT(6)
> +#define MFX_REG_SYS_CTRL_SWRST		BIT(7)
> +
> +/* MFX_REG_IRQ_OUT_PIN bitfields */
> +#define MFX_REG_IRQ_OUT_PIN_TYPE	BIT(0) /* 0-OD 1-PP */
> +#define MFX_REG_IRQ_OUT_PIN_POL		BIT(1) /* 0-active LOW 1-active HIGH */
> +
> +/* MFX_REG_IRQ_SRC_EN bitfields */
> +#define MFX_REG_IRQ_SRC_EN_GPIO		BIT(0)
> +
> +/* MFX_REG_IRQ_PENDING bitfields */
> +#define MFX_REG_IRQ_PENDING_GPIO	BIT(0)
> +
> +#endif /* __MFD_ST_MFX_H */
> +

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

Re: [PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver

[Philippe Ombredanne]

Amelie,

On Mon, Feb 12, 2018 at 1:06 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Thu, 08 Feb 2018, Amelie Delaunay wrote:
>
>> ST Multi-Function eXpander (MFX) is a slave controller using I2C
>> for communication with the main MCU. Main features are:
>> - 16 fast GPIOs individually configurable in input/output
>> - 8 alternate GPIOs individually configurable in input/output
>> - Main MCU IDD measurement
>> - Resistive touchscreen controller
>>
>> Only GPIO expander (16 fast GPIOs + 8 alternate) feature is
>> supported for the moment.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org>

<snip>

>> --- /dev/null
>> +++ b/drivers/mfd/st-mfx.c
>> @@ -0,0 +1,526 @@
>> +/*
>> + * STMicroelectronics Multi-Function eXpander (ST-MFX) Core Driver
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Author(s): Amelie Delaunay <amelie.delaunay-qxv4g6HH51o@public.gmane.org> for STMicroelectronics.
>
> You don't need to put "for STMicroelectronics".  This was something we
> made up when submitting from a different (!st.com) email address.
>
>> + * License terms: GPL V2.0.
>> + *
>> + * st-mfx Core Driver is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * st-mfx Core Driver is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
>> + * details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * st-mfx Core Driver. If not, see <http://www.gnu.org/licenses/>.
>
> You should be able to use the short version of the licensing
> agreement.  Also, please grep for "SPDX".

You can check the doc for the (fairly new) way to remove legalese
boilerplate at Documentation/process/license-rules.rst or [1]
It helps keep the focus on the code and less on licensing!

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
-- 
Cordially
Philippe Ombredanne
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver

[Amelie DELAUNAY]

Thanks Philippe,

I will take this into account for V2.

Regards,
Amelie

On 02/12/2018 03:15 PM, Philippe Ombredanne wrote:
> Amelie,
> 
> On Mon, Feb 12, 2018 at 1:06 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> On Thu, 08 Feb 2018, Amelie Delaunay wrote:
>>
>>> ST Multi-Function eXpander (MFX) is a slave controller using I2C
>>> for communication with the main MCU. Main features are:
>>> - 16 fast GPIOs individually configurable in input/output
>>> - 8 alternate GPIOs individually configurable in input/output
>>> - Main MCU IDD measurement
>>> - Resistive touchscreen controller
>>>
>>> Only GPIO expander (16 fast GPIOs + 8 alternate) feature is
>>> supported for the moment.
>>>
>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> 
> <snip>
> 
>>> --- /dev/null
>>> +++ b/drivers/mfd/st-mfx.c
>>> @@ -0,0 +1,526 @@
>>> +/*
>>> + * STMicroelectronics Multi-Function eXpander (ST-MFX) Core Driver
>>> + *
>>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>>> + * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.
>>
>> You don't need to put "for STMicroelectronics".  This was something we
>> made up when submitting from a different (!st.com) email address.
>>
>>> + * License terms: GPL V2.0.
>>> + *
>>> + * st-mfx Core Driver is free software; you can redistribute it and/or modify it
>>> + * under the terms of the GNU General Public License version 2 as published by
>>> + * the Free Software Foundation.
>>> + *
>>> + * st-mfx Core Driver is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
>>> + * details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * st-mfx Core Driver. If not, see <http://www.gnu.org/licenses/>.
>>
>> You should be able to use the short version of the licensing
>> agreement.  Also, please grep for "SPDX".
> 
> You can check the doc for the (fairly new) way to remove legalese
> boilerplate at Documentation/process/license-rules.rst or [1]
> It helps keep the focus on the code and less on licensing!
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
> 

Re: [PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver

[Amelie DELAUNAY]

On 02/12/2018 01:06 PM, Lee Jones wrote:
> On Thu, 08 Feb 2018, Amelie Delaunay wrote:
> 
>> ST Multi-Function eXpander (MFX) is a slave controller using I2C
>> for communication with the main MCU. Main features are:
>> - 16 fast GPIOs individually configurable in input/output
>> - 8 alternate GPIOs individually configurable in input/output
>> - Main MCU IDD measurement
>> - Resistive touchscreen controller
>>
>> Only GPIO expander (16 fast GPIOs + 8 alternate) feature is
>> supported for the moment.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
>> ---
>>   drivers/mfd/Kconfig        |  15 ++
>>   drivers/mfd/Makefile       |   1 +
>>   drivers/mfd/st-mfx.c       | 526 +++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/mfd/st-mfx.h | 116 ++++++++++
>>   4 files changed, 658 insertions(+)
>>   create mode 100644 drivers/mfd/st-mfx.c
>>   create mode 100644 include/linux/mfd/st-mfx.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 1d20a80..e78e818 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1823,6 +1823,21 @@ config MFD_STM32_TIMERS
>>   	  for PWM and IIO Timer. This driver allow to share the
>>   	  registers between the others drivers.
>>   
>> +config MFD_ST_MFX
>> +	bool "STMicroelectronics MFX"
>> +	depends on I2C
>> +	depends on OF || COMPILE_TEST
>> +	select MFD_CORE
>> +	select REGMAP_I2C
>> +	help
>> +	  Support for the STMicroelectronics Multi-Function eXpander.
> 
> Is that really what this device is called?
Yes, all STMicroelectronics Evaluation boards and Discovery kits (using 
an expansion device) user manuals refer to "MFX (Multi Function eXpander)".
> 
> Is there a datasheet?
> 
>> +	  This driver provides common support for accessing the device,
>> +	  additional drivers must be enabled in order to use the functionality
>> +	  of the device. Currently available sub drivers are:
>> +
>> +		GPIO: mfx-gpio
> 
> This driver would be easier to review if I could picture the device as
> a whole.  What other functionality does it have?  What is it that
> makes this an MFD?
>
MFX is a slave controller based on the STM32L152CCT6 MCU.
MFX firmware integrates 3 features:
- main MCU IDD measurement: this allows measurement of the IDD current 
consumed by the main MCU, especially for static measurement in low power 
mode.
- resistive touchscreen controller, using ADC inputs of STM32L152CCT6.
- GPIO expander: 16 programmable input/output + 8 alternate GPIO 
(Touchscreen controller uses the four first gpios of this bank when 
enabled, IDD used the 4 last ones when enabled).

IDD measurement driver and touchscreen driver have not been developed 
yet because I have no hardware to test it. But anyway, I will send a V2 
with gpio driver only.

>>   menu "Multimedia Capabilities Port drivers"
>>   	depends on ARCH_SA1100
>>   
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index d9474ad..1379a18 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -230,3 +230,4 @@ obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
>>   obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
>>   obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
>>   obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>> +obj-$(CONFIG_MFD_ST_MFX) 	+= st-mfx.o
>> diff --git a/drivers/mfd/st-mfx.c b/drivers/mfd/st-mfx.c
>> new file mode 100644
>> index 0000000..5bee5d3
>> --- /dev/null
>> +++ b/drivers/mfd/st-mfx.c
>> @@ -0,0 +1,526 @@
>> +/*
>> + * STMicroelectronics Multi-Function eXpander (ST-MFX) Core Driver
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.
> 
> You don't need to put "for STMicroelectronics".  This was something we
> made up when submitting from a different (!st.com) email address.
> 
OK, I will fix it in gpio driver for V2.
>> + * License terms: GPL V2.0.
>> + *
>> + * st-mfx Core Driver is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * st-mfx Core Driver is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
>> + * details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * st-mfx Core Driver. If not, see <http://www.gnu.org/licenses/>.
> 
> You should be able to use the short version of the licensing
> agreement.  Also, please grep for "SPDX".
> 
OK.
>> + */
>> +#include <linux/bitfield.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/st-mfx.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/regmap.h>
>> +
>> +/**
>> + * struct mfx_priv - MFX MFD private structure
>> + * @dev: device, mostly for logs
>> + * @regmap: register map
>> + * @mfx: MFX MFD public structure, to share information with subdrivers
>> + * @irq_domain: IRQ domain
>> + * @irq: IRQ number for mfx
>> + * @irq_lock: IRQ bus lock
>> + * @irqen: cache of IRQ_SRC_EN register for bus_lock
>> + * @oldirqen: cache of IRQ_SRC_EN register for bus_lock
>> + */
> 
> /**
>   * struct mfx_priv - MFX MFD private structure
>   * @dev:  	   device, mostly for logs
>   * @regmap: 	   register map
>   * @mfx: 	   MFX MFD public structure, to share information with subdrivers
>   * @irq_domain:    IRQ domain
>   * @irq: 	   IRQ number for mfx
>   * @irq_lock: 	   IRQ bus lock
>   * @irqen: 	   cache of IRQ_SRC_EN register for bus_lock
>   * @oldirqen: 	   cache of IRQ_SRC_EN register for bus_lock
>   */
> 
> Easier to read?
> 
I will pay attention to the indentation in gpio driver V2.
>> +struct mfx_priv {
> 
> mfx_ddata
> 
OK.
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	struct mfx mfx;
>> +	struct irq_domain *irq_domain;
>> +	int irq;
>> +	struct mutex irq_lock; /* IRQ bus lock */
>> +	u8 irqen;
>> +	u8 oldirqen;
>> +};
>> +
>> +#define to_mfx_priv(_mfx) container_of(_mfx, struct mfx_priv, mfx)
> 
> FYI: I don't like this idea.  More to follow.
> 
>> +/* MFX boot time is around 10ms, so after reset, we have to wait this delay */
>> +#define MFX_BOOT_TIME 10
> 
> MFX_BOOT_TIME_MS
> 
OK.
>> +static u8 mfx_blocks_to_mask(u32 blocks)
> 
> I think it would be better to prepend a vendor tag to everything too.
> 
>    st_mfx_*
> 
OK.
>> +{
>> +	u8 mask = 0;
>> +
>> +	if (blocks & MFX_BLOCK_GPIO)
>> +		mask |= MFX_REG_SYS_CTRL_GPIO_EN;
>> +	else
>> +		mask &= ~MFX_REG_SYS_CTRL_GPIO_EN;
>> +
>> +	if (blocks & MFX_BLOCK_TS)
>> +		mask |= MFX_REG_SYS_CTRL_TS_EN;
>> +	else
>> +		mask &= ~MFX_REG_SYS_CTRL_TS_EN;
>> +
>> +	if (blocks & MFX_BLOCK_IDD)
>> +		mask |= MFX_REG_SYS_CTRL_IDD_EN;
>> +	else
>> +		mask &= ~MFX_REG_SYS_CTRL_IDD_EN;
>> +
>> +	if (blocks & MFX_BLOCK_ALTGPIO)
>> +		mask |= MFX_REG_SYS_CTRL_ALTGPIO_EN;
>> +	else
>> +		mask &= ~MFX_REG_SYS_CTRL_ALTGPIO_EN;
>> +
>> +	return mask;
>> +}
>> +
>> +static int mfx_reset(struct mfx *mfx)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
>> +				 MFX_REG_SYS_CTRL_SWRST,
>> +				 MFX_REG_SYS_CTRL_SWRST);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	msleep(MFX_BOOT_TIME);
>> +
>> +	return ret;
>> +}
>> +
>> +int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
>> +
>> +	return regmap_bulk_read(mfx_priv->regmap, reg, values, length);
>> +}
>> +EXPORT_SYMBOL_GPL(mfx_block_read);
>> +
>> +int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
>> +
>> +	return regmap_bulk_write(mfx_priv->regmap, reg, values, length);
>> +}
>> +EXPORT_SYMBOL_GPL(mfx_block_write);
>> +
>> +int mfx_reg_read(struct mfx *mfx, u8 reg)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = regmap_read(mfx_priv->regmap, reg, &val);
>> +
>> +	return ret ? ret : val;
>> +}
>> +EXPORT_SYMBOL_GPL(mfx_reg_read);
>> +
>> +int mfx_reg_write(struct mfx *mfx, u8 reg, u8 val)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
>> +
>> +	return regmap_write(mfx_priv->regmap, reg, val);
>> +}
>> +EXPORT_SYMBOL_GPL(mfx_reg_write);
>> +
>> +int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
>> +
>> +	return regmap_update_bits(mfx_priv->regmap, reg, mask, val);
>> +}
>> +EXPORT_SYMBOL_GPL(mfx_set_bits);
>> +
>> +int mfx_enable(struct mfx *mfx, u32 blocks)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
>> +	u8 mask = mfx_blocks_to_mask(blocks);
>> +
>> +	return regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
>> +				  mask, mask);
>> +}
>> +EXPORT_SYMBOL_GPL(mfx_enable);
>> +
>> +int mfx_disable(struct mfx *mfx, u32 blocks)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(mfx);
>> +	u8 mask = mfx_blocks_to_mask(blocks);
>> +
>> +	return regmap_update_bits(mfx_priv->regmap, MFX_REG_SYS_CTRL,
>> +				  mask, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(mfx_disable);
> 
> The remap infrastructure doesn't need further abstraction.  Please
> pass the pointer to any child devices and have them use it directly.
> 
I will keep it in mind if the need of other features aruses, justifying 
the use of an MFD.
>> +static void mfx_irq_lock(struct irq_data *data)
>> +{
>> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
>> +
>> +	mutex_lock(&mfx_priv->irq_lock);
>> +}
>> +
>> +static void mfx_irq_sync_unlock(struct irq_data *data)
>> +{
>> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
>> +	u8 new = mfx_priv->irqen;
>> +	u8 old = mfx_priv->oldirqen;
>> +
>> +	if (new == old)
>> +		goto unlock;
>> +
>> +	mfx_priv->oldirqen = new;
>> +	mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_SRC_EN, new);
>> +unlock:
>> +	mutex_unlock(&mfx_priv->irq_lock);
>> +}
>> +
>> +static void mfx_irq_mask(struct irq_data *data)
>> +{
>> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
>> +
>> +	mfx_priv->irqen &= ~BIT(data->hwirq % 8);
>> +}
>> +
>> +static void mfx_irq_unmask(struct irq_data *data)
>> +{
>> +	struct mfx_priv *mfx_priv = irq_data_get_irq_chip_data(data);
>> +
>> +	mfx_priv->irqen |= BIT(data->hwirq % 8);
>> +}
>> +
>> +static struct irq_chip mfx_irq_chip = {
>> +	.name			= "mfx",
>> +	.irq_bus_lock		= mfx_irq_lock,
>> +	.irq_bus_sync_unlock	= mfx_irq_sync_unlock,
>> +	.irq_mask		= mfx_irq_mask,
>> +	.irq_unmask		= mfx_irq_unmask,
>> +};
>> +
>> +static irqreturn_t mfx_irq(int irq, void *data)
>> +{
>> +	struct mfx_priv *mfx_priv = data;
> 
> 'ddata' is a more consistent naming approach.
> 
OK.
>> +	unsigned long status, bit;
>> +	u8 ack;
>> +	int ret;
>> +
>> +	ret = mfx_reg_read(&mfx_priv->mfx, MFX_REG_IRQ_PENDING);
>> +	if (ret < 0) {
>> +		dev_err(mfx_priv->dev, "can't get IRQ_PENDING: %d\n", ret);
>> +		return IRQ_NONE;
>> +	}
>> +
>> +	/* It can be GPIO, IDD, ERROR, TS* IRQs */
>> +	status = ret & mfx_priv->irqen;
>> +
>> +	/*
>> +	 * There is no ACK for GPIO, MFX_REG_IRQ_PENDING_GPIO is a logical OR
>> +	 * of MFX_REG_IRQ_GPI _PENDING1/_PENDING2/_PENDING3
>> +	 */
>> +	ack = status & ~MFX_REG_IRQ_PENDING_GPIO;
>> +
>> +	for_each_set_bit(bit, &status, 8)
>> +		handle_nested_irq(irq_find_mapping(mfx_priv->irq_domain, bit));
>> +
>> +	if (ack)
>> +		mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_ACK, ack);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int mfx_irq_map(struct irq_domain *d, unsigned int virq,
>> +		       irq_hw_number_t hwirq)
>> +{
>> +	struct mfx_priv *mfx_priv = d->host_data;
>> +
>> +	irq_set_chip_data(virq, mfx_priv);
>> +	irq_set_chip(virq, &mfx_irq_chip);
>> +	irq_set_nested_thread(virq, 1);
>> +	irq_set_parent(virq, mfx_priv->irq);
>> +	irq_set_noprobe(virq);
>> +
>> +	return 0;
>> +}
>> +
>> +static void mfx_irq_unmap(struct irq_domain *d, unsigned int virq)
>> +{
>> +	irq_set_chip_and_handler(virq, NULL, NULL);
>> +	irq_set_chip_data(virq, NULL);
>> +}
>> +
>> +static const struct irq_domain_ops mfx_irq_ops = {
>> +	.map = mfx_irq_map,
>> +	.unmap = mfx_irq_unmap,
>> +	.xlate = irq_domain_xlate_onecell,
>> +};
>> +
>> +static int mfx_irq_init(struct mfx_priv *mfx_priv, struct device_node *np)
>> +{
>> +	int irqoutpin = MFX_REG_IRQ_OUT_PIN_TYPE; /* Push-Pull */
>> +	int irqtrigger, ret;
>> +
>> +	mfx_priv->irq = of_irq_get(np, 0);
>> +	if (mfx_priv->irq > 0) {
>> +		irqtrigger = irq_get_trigger_type(mfx_priv->irq);
>> +	} else {
>> +		dev_err(mfx_priv->dev, "failed to get irq: %d\n",
>> +			mfx_priv->irq);
>> +		return mfx_priv->irq;
>> +	}
>> +
>> +	if ((irqtrigger & IRQ_TYPE_EDGE_FALLING) ||
>> +	    (irqtrigger & IRQ_TYPE_LEVEL_LOW))
>> +		irqoutpin &= ~MFX_REG_IRQ_OUT_PIN_POL; /* Active Low */
>> +	else
>> +		irqoutpin |= MFX_REG_IRQ_OUT_PIN_POL; /* Active High */
>> +
>> +	mfx_reg_write(&mfx_priv->mfx, MFX_REG_IRQ_OUT_PIN, irqoutpin);
>> +
>> +	mfx_priv->irq_domain = irq_domain_add_linear(np, MFX_IRQ_SRC_NR,
>> +						     &mfx_irq_ops, mfx_priv);
>> +	if (!mfx_priv->irq_domain) {
>> +		dev_err(mfx_priv->dev, "failed to create irq domain\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ret = devm_request_threaded_irq(mfx_priv->dev, mfx_priv->irq,
>> +					NULL, mfx_irq,
>> +					irqtrigger | IRQF_ONESHOT, "mfx",
>> +					mfx_priv);
>> +	if (ret) {
>> +		dev_err(mfx_priv->dev, "failed to request irq: %d\n", ret);
>> +		irq_domain_remove(mfx_priv->irq_domain);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void mfx_irq_remove(struct mfx_priv *mfx_priv)
>> +{
>> +	int hwirq;
>> +
>> +	for (hwirq = 0; hwirq < MFX_IRQ_SRC_NR; hwirq++)
>> +		irq_dispose_mapping(irq_find_mapping(mfx_priv->irq_domain,
>> +						     hwirq));
>> +	irq_domain_remove(mfx_priv->irq_domain);
>> +}
> 
> Is there any reason why this IRQ stuff can't live in the GPIO driver?
> 
MFX has a first level of interrupts (8 sources), with touscreen 
interrupts, idd interrupt and gpio interrupt. Then gpio has its own 
level of interrupts, with 24 possible sources.
GPIO interrupts are raised through MFX_IRQ_OUT pin only if at least one 
source is enabled at gpio level, and gpio interrupt is enabled at MFX level.
               _TS_OVF
              |_TS_FULL
              |_TS_TH
              |_TS_NE
MFX_IRQ_OUT < _TS_DET
              |_ERROR(IDD)     _ GPIO0
              |_IDD           |
              |_GPIO---------<...
                              |_ GPIO23
>> +static int mfx_chip_init(struct mfx_priv *mfx_priv, u16 i2c_addr)
>> +{
>> +	struct mfx *mfx = &mfx_priv->mfx;
>> +	int ret;
>> +	int id;
>> +	u8 version[2];
>> +
>> +	id = mfx_reg_read(mfx, MFX_REG_CHIP_ID);
>> +	if (id < 0) {
>> +		dev_err(mfx_priv->dev, "error reading chip id: %d\n", id);
>> +		return id;
>> +	}
>> +
>> +	ret = mfx_block_read(mfx, MFX_REG_FW_VERSION_MSB,
>> +			     ARRAY_SIZE(version), version);
>> +	if (ret < 0) {
>> +		dev_err(mfx_priv->dev, "error reading fw version: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * Check that ID is the complement of the I2C address:
>> +	 * MFX I2C address follows the 7-bit format (MSB), that's why
>> +	 * i2c_addr is shifted.
>> +	 *
>> +	 * MFX_I2C_ADDR |         MFX         |        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(MFX_REG_CHIP_ID_MASK, ~id) != (i2c_addr << 1)) {
>> +		dev_err(mfx_priv->dev, "unknown chip id: %#x\n", id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	dev_info(mfx_priv->dev, "ST-MFX chip id: %#x, fw version: %x.%02x\n",
>> +		 id, version[0], version[1]);
>> +
>> +	/* Disable all features, subdrivers should enable what they need */
>> +	ret = mfx_disable(mfx, ~0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return mfx_reset(mfx);
>> +}
>> +
>> +static const struct regmap_config mfx_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +};
>> +
>> +static const struct of_device_id mfx_of_match[] = {
>> +	{ .compatible = "st,mfx" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, mfx_of_match);
>> +
>> +static int mfx_of_probe(struct device_node *np, struct mfx_priv *mfx_priv)
>> +{
>> +	struct device_node *child;
>> +
>> +	if (!np)
>> +		return -ENODEV;
> 
> Is this even possible?
> 
> Do you support !OF?
> 
No.
>> +	for_each_child_of_node(np, child) {
>> +		if (of_device_is_compatible(child, "st,mfx-gpio")) {
>> +			mfx_priv->mfx.blocks |= MFX_BLOCK_GPIO;
>> +			mfx_priv->mfx.num_gpio = 16;
> 
> This is ugly.
> 
> Where is num_gpio used?
> 
Was used in gpio driver. But anyway this will disappear in V2.
>> +		}
>> +		/*
>> +		 * Here we could find other children like "st,mfx-ts" or
>> +		 * "st,mfx-idd.
>> +		 */
>> +	}
>> +
>> +	if (!(mfx_priv->mfx.blocks & MFX_BLOCK_TS) &&
>> +	    !(mfx_priv->mfx.blocks & MFX_BLOCK_IDD)) {
>> +		mfx_priv->mfx.blocks |= MFX_BLOCK_ALTGPIO;
>> +		mfx_priv->mfx.num_gpio += 8;
>> +	}
> 
> What are TS and IDD?
> 
> This is starting to look like a GPIO driver, and nothing more.
> 
Without touschreen controller driver and idd measurement driver, 
definitely yes.
>> +	/*
>> +	 * TODO: aGPIO[3:0] and aGPIO[7:4] can be used independently:
>> +	 * - if IDD is used but not TS, aGPIO[3:0] can be used as GPIO,
>> +	 * - if TS is used but not IDD: aGPIO[7:4] can be used as GPIO.
>> +	 */
>> +
>> +	return of_platform_populate(np, NULL, NULL, mfx_priv->dev);
>> +}
>> +
>> +int mfx_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> +{
>> +	struct device_node *np = client->dev.of_node;
>> +	struct mfx_priv *mfx_priv;
>> +	int ret;
>> +
>> +	mfx_priv = devm_kzalloc(&client->dev, sizeof(struct mfx_priv),
>> +				GFP_KERNEL);
>> +	if (!mfx_priv)
>> +		return -ENOMEM;
>> +
>> +	mfx_priv->regmap = devm_regmap_init_i2c(client, &mfx_regmap_config);
>> +	if (IS_ERR(mfx_priv->regmap)) {
>> +		ret = PTR_ERR(mfx_priv->regmap);
>> +		dev_err(&client->dev, "failed to allocate register map: %d\n",
>> +			ret);
> 
> Nit: Prefer if you broke the line after '&client->dev,'.
> 
OK.
>> +		return ret;
>> +	}
>> +
>> +	mfx_priv->dev = &client->dev;
> 
> 
>> +	i2c_set_clientdata(client, &mfx_priv->mfx);
>> +
>> +	mutex_init(&mfx_priv->irq_lock);
>> +
>> +	ret = mfx_chip_init(mfx_priv, client->addr);
>> +	if (ret) {
>> +		if (ret == -ETIMEDOUT)
>> +			return -EPROBE_DEFER;
> 
> Return -EPROBE_DEFER from reset() instead.
> 
OK.
>> +		return ret;
>> +	}
>> +
>> +	ret = mfx_irq_init(mfx_priv, np);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = mfx_of_probe(np, mfx_priv);
>> +	if (ret < 0)
>> +		return ret;
> 
> Is it possible to build with !OF?
> 
> If not, move all probe code into here.
> 
>> +	dev_info(mfx_priv->dev, "ST-MFX (CORE) initialized\n");
> 
> These kinds of messages are usually frowned upon.
> 
OK.
>> +	return 0;
>> +}
>> +
>> +static int mfx_remove(struct i2c_client *client)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(i2c_get_clientdata(client));
>> +
>> +	mfx_irq_remove(mfx_priv);
>> +	of_platform_depopulate(mfx_priv->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int mfx_suspend(struct device *dev)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(dev_get_drvdata(dev));
>> +
>> +	if (device_may_wakeup(dev))
>> +		enable_irq_wake(mfx_priv->irq);
>> +
>> +	/*
>> +	 * TODO: Do we put MFX in STANDBY mode ?
>> +	 * (Wakeup by rising edge on MFX_wakeup pin)
>> +	 */
> 
> How long before you answer this question?
> 
> Better do just enable it right away or make a mental note and remove
> the comment from the upstream driver.
> 
>> +	return 0;
>> +}
>> +
>> +static int mfx_resume(struct device *dev)
>> +{
>> +	struct mfx_priv *mfx_priv = to_mfx_priv(dev_get_drvdata(dev));
>> +
>> +	if (device_may_wakeup(dev))
>> +		disable_irq_wake(mfx_priv->irq);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(mfx_dev_pm_ops, mfx_suspend, mfx_resume);
>> +
>> +static const struct i2c_device_id mfx_i2c_id[] = {
>> +	{ "mfx", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(i2c, mfx_id);
> 
> I don't think this is required anymore.
> 
You're right.

Thanks for review,
Amelie
>> +static struct i2c_driver mfx_driver = {
>> +	.driver = {
>> +		.name = "st-mfx",
>> +		.pm = &mfx_dev_pm_ops,
>> +		.of_match_table = mfx_of_match,
>> +	},
>> +	.probe = mfx_probe,
>> +	.remove = mfx_remove,
>> +	.id_table = mfx_i2c_id,
>> +};
>> +
>> +static int __init mfx_init(void)
>> +{
>> +	return i2c_add_driver(&mfx_driver);
>> +}
>> +subsys_initcall(mfx_init);
>> +
>> +static void __exit mfx_exit(void)
>> +{
>> +	i2c_del_driver(&mfx_driver);
>> +}
>> +module_exit(mfx_exit);
>> +
>> +MODULE_DESCRIPTION("STMicroelectronics Multi-Function eXpander MFD core driver");
>> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/st-mfx.h b/include/linux/mfd/st-mfx.h
>> new file mode 100644
>> index 0000000..1bf7e65
>> --- /dev/null
>> +++ b/include/linux/mfd/st-mfx.h
>> @@ -0,0 +1,116 @@
>> +/*
>> + * STMicroelectronics Multi-Function eXpander (ST-MFX)
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.
>> + *
>> + * License terms: GPL V2.0.
>> + *
>> + * st-mfx is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * st-mfx is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
>> + * details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * st-mfx. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef __MFD_ST_MFX_H
>> +#define __MFD_ST_MFX_H
>> +
>> +enum mfx_block {
>> +	MFX_BLOCK_GPIO		= BIT(0),
>> +	MFX_BLOCK_TS		= BIT(1),
>> +	MFX_BLOCK_IDD		= BIT(2),
>> +	MFX_BLOCK_ALTGPIO	= BIT(3),
>> +};
>> +
>> +/*
>> + * 8 events can activate the MFX_IRQ_OUT signal,
>> + * but for the moment, only GPIO event is used
>> + */
>> +enum mfx_irq {
>> +	MFX_IRQ_SRC_GPIO,
>> +
>> +	MFX_IRQ_SRC_NR,
>> +};
>> +
>> +/**
>> + * struct mfx - MFX MFD structure
>> + * @blocks: mask of mfx_block to be enabled
>> + * @num_gpio: number of gpios
>> + */
>> +struct mfx {
>> +	u32 blocks;
>> +	u32 num_gpio;
>> +};
>> +
>> +int mfx_reg_write(struct mfx *mfx, u8 reg, u8 data);
>> +int mfx_reg_read(struct mfx *mfx, u8 reg);
>> +int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values);
>> +int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values);
>> +int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val);
>> +int mfx_enable(struct mfx *mfx, unsigned int blocks);
>> +int mfx_disable(struct mfx *mfx, unsigned int blocks);
>> +
>> +/* General */
>> +#define MFX_REG_CHIP_ID			0x00 /* R */
>> +#define MFX_REG_FW_VERSION_MSB		0x01 /* R */
>> +#define MFX_REG_FW_VERSION_LSB		0x02 /* R */
>> +#define MFX_REG_SYS_CTRL		0x40 /* RW */
>> +/* IRQ output management */
>> +#define MFX_REG_IRQ_OUT_PIN		0x41 /* RW */
>> +#define MFX_REG_IRQ_SRC_EN		0x42 /* RW */
>> +#define MFX_REG_IRQ_PENDING		0x08 /* R */
>> +#define MFX_REG_IRQ_ACK			0x44 /* RW */
>> +/* GPIOs expander */
>> +/* GPIO_STATE1 0x10, GPIO_STATE2 0x11, GPIO_STATE3 0x12 */
>> +#define MFX_REG_GPIO_STATE		0x10 /* R */
>> +/* GPIO_DIR1 0x60, GPIO_DIR2 0x61, GPIO_DIR3 0x63 */
>> +#define MFX_REG_GPIO_DIR		0x60 /* RW */
>> +/* GPIO_TYPE1 0x64, GPIO_TYPE2 0x65, GPIO_TYPE3 0x66 */
>> +#define MFX_REG_GPIO_TYPE		0x64 /* RW */
>> +/* GPIO_PUPD1 0x68, GPIO_PUPD2 0x69, GPIO_PUPD3 0x6A */
>> +#define MFX_REG_GPIO_PUPD		0x68 /* RW */
>> +/* GPO_SET1 0x6C, GPO_SET2 0x6D, GPO_SET3 0x6E */
>> +#define MFX_REG_GPO_SET			0x6C /* RW */
>> +/* GPO_CLR1 0x70, GPO_CLR2 0x71, GPO_CLR3 0x72 */
>> +#define MFX_REG_GPO_CLR			0x70 /* RW */
>> +/* IRQ_GPI_SRC1 0x48, IRQ_GPI_SRC2 0x49, IRQ_GPI_SRC3 0x4A */
>> +#define MFX_REG_IRQ_GPI_SRC		0x48 /* RW */
>> +/* IRQ_GPI_EVT1 0x4C, IRQ_GPI_EVT2 0x4D, IRQ_GPI_EVT3 0x4E */
>> +#define MFX_REG_IRQ_GPI_EVT		0x4C /* RW */
>> +/* IRQ_GPI_TYPE1 0x50, IRQ_GPI_TYPE2 0x51, IRQ_GPI_TYPE3 0x52 */
>> +#define MFX_REG_IRQ_GPI_TYPE		0x50 /* RW */
>> +/* IRQ_GPI_PENDING1 0x0C, IRQ_GPI_PENDING2 0x0D, IRQ_GPI_PENDING3 0x0E*/
>> +#define MFX_REG_IRQ_GPI_PENDING		0x0C /* R */
>> +/* IRQ_GPI_ACK1 0x54, IRQ_GPI_ACK2 0x55, IRQ_GPI_ACK3 0x56 */
>> +#define MFX_REG_IRQ_GPI_ACK		0x54 /* RW */
>> +
>> +/* MFX_REG_CHIP_ID bitfields */
>> +#define MFX_REG_CHIP_ID_MASK		GENMASK(7, 0)
>> +
>> +/* MFX_REG_SYS_CTRL bitfields */
>> +#define MFX_REG_SYS_CTRL_GPIO_EN	BIT(0)
>> +#define MFX_REG_SYS_CTRL_TS_EN		BIT(1)
>> +#define MFX_REG_SYS_CTRL_IDD_EN		BIT(2)
>> +#define MFX_REG_SYS_CTRL_ALTGPIO_EN	BIT(3)
>> +#define MFX_REG_SYS_CTRL_STANDBY	BIT(6)
>> +#define MFX_REG_SYS_CTRL_SWRST		BIT(7)
>> +
>> +/* MFX_REG_IRQ_OUT_PIN bitfields */
>> +#define MFX_REG_IRQ_OUT_PIN_TYPE	BIT(0) /* 0-OD 1-PP */
>> +#define MFX_REG_IRQ_OUT_PIN_POL		BIT(1) /* 0-active LOW 1-active HIGH */
>> +
>> +/* MFX_REG_IRQ_SRC_EN bitfields */
>> +#define MFX_REG_IRQ_SRC_EN_GPIO		BIT(0)
>> +
>> +/* MFX_REG_IRQ_PENDING bitfields */
>> +#define MFX_REG_IRQ_PENDING_GPIO	BIT(0)
>> +
>> +#endif /* __MFD_ST_MFX_H */
>> +
> 

Re: [PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver

[Linus Walleij]

On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:

Thanks for working on this complex expander driver.
It is a bit daunting. Sorry if there are lots of comments and
considerations, but it reflects the complexity of the hardware.

> +enum mfx_block {
> +       MFX_BLOCK_GPIO          = BIT(0),
> +       MFX_BLOCK_TS            = BIT(1),
> +       MFX_BLOCK_IDD           = BIT(2),
> +       MFX_BLOCK_ALTGPIO       = BIT(3),
> +};

This looks suspiciously similar to this:

enum stmpe_block {
        STMPE_BLOCK_GPIO        = 1 << 0,
        STMPE_BLOCK_KEYPAD      = 1 << 1,
        STMPE_BLOCK_TOUCHSCREEN = 1 << 2,
        STMPE_BLOCK_ADC         = 1 << 3,
        STMPE_BLOCK_PWM         = 1 << 4,
        STMPE_BLOCK_ROTATOR     = 1 << 5,
};

Apparently the same hardware designers are involved.

> +int mfx_reg_write(struct mfx *mfx, u8 reg, u8 data);
> +int mfx_reg_read(struct mfx *mfx, u8 reg);
> +int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values);
> +int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values);
> +int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val);

Do you need this? Can't you just use regmap and pass
around a struct regmap *map to access registers?

You don't necessarily need to use the default I2C regmap
(like, e.g. drivers/mfd/stw481x.c) but even if a more
complex access pattern is used to read/write registers
I am pretty sure you can use regmap for it.

> +int mfx_enable(struct mfx *mfx, unsigned int blocks);
> +int mfx_disable(struct mfx *mfx, unsigned int blocks);

This is similar to
extern int stmpe_enable(struct stmpe *stmpe, unsigned int blocks);
extern int stmpe_disable(struct stmpe *stmpe, unsigned int blocks);

So again I am suspicious about duplication of driver code.

It even looks a bit like this driver started as a copy of
the STMPE driver, which is not a good sign. It might be
that it was copied from there because the hardware is
actually very similar.

> +/* General */
> +#define MFX_REG_CHIP_ID                        0x00 /* R */
> +#define MFX_REG_FW_VERSION_MSB         0x01 /* R */
> +#define MFX_REG_FW_VERSION_LSB         0x02 /* R */
> +#define MFX_REG_SYS_CTRL               0x40 /* RW */

The STMPE driver defines enumerated registers in
include/linux/mfd/stmpe.h
then assign each variant using the model specifics in
drivers/mfd/stmpe.h

This doesn't seem super much different.

Even if the old STMPE driver may be a bad fit, is is better
to improve that (e.g. migrate it to use regmap and rewrite the
stmpe-gpio.c driver to use pin control) and use also for this
driver, or write a new driver from scratch like this?

I'm not so sure.

I do know that developers not always like to take out old
hardware and old development boards and start hacking
away before they can get some nice new hardware going
but I am worried that this may be one of those cases where
a serious cleanup of the aging STMPE driver may be a
first necessary step.

> +/* IRQ output management */
> +#define MFX_REG_IRQ_OUT_PIN            0x41 /* RW */
> +#define MFX_REG_IRQ_SRC_EN             0x42 /* RW */
> +#define MFX_REG_IRQ_PENDING            0x08 /* R */
> +#define MFX_REG_IRQ_ACK                        0x44 /* RW */

Very similar to STMPE it seems.

> +/* MFX_REG_SYS_CTRL bitfields */
> +#define MFX_REG_SYS_CTRL_GPIO_EN       BIT(0)
> +#define MFX_REG_SYS_CTRL_TS_EN         BIT(1)
> +#define MFX_REG_SYS_CTRL_IDD_EN                BIT(2)
> +#define MFX_REG_SYS_CTRL_ALTGPIO_EN    BIT(3)

I guess these blocks works the same as with STMPE,
that you can only use one of them at the time?

What is altgpio?

> +/* MFX_REG_IRQ_OUT_PIN bitfields */
> +#define MFX_REG_IRQ_OUT_PIN_TYPE       BIT(0) /* 0-OD 1-PP */
> +#define MFX_REG_IRQ_OUT_PIN_POL                BIT(1) /* 0-active LOW 1-active HIGH */

I have not read the patch yet. But just for notice:
This output IRQ type needs to be handled as well.

Check the code in
drivers/iio/common/st_sensors/st_sensors_trigger.c

To see how you can detect the properties of an IRQ
to set the right polarity, and handling of open drain
IRQ lines.

Yours,
Linus Walleij

Re: [PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver

[Amelie DELAUNAY]

On 02/22/2018 02:44 PM, Linus Walleij wrote:
> On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:
> 
> Thanks for working on this complex expander driver.
> It is a bit daunting. Sorry if there are lots of comments and
> considerations, but it reflects the complexity of the hardware.
> 
>> +enum mfx_block {
>> +       MFX_BLOCK_GPIO          = BIT(0),
>> +       MFX_BLOCK_TS            = BIT(1),
>> +       MFX_BLOCK_IDD           = BIT(2),
>> +       MFX_BLOCK_ALTGPIO       = BIT(3),
>> +};
> 
> This looks suspiciously similar to this:
> 
> enum stmpe_block {
>          STMPE_BLOCK_GPIO        = 1 << 0,
>          STMPE_BLOCK_KEYPAD      = 1 << 1,
>          STMPE_BLOCK_TOUCHSCREEN = 1 << 2,
>          STMPE_BLOCK_ADC         = 1 << 3,
>          STMPE_BLOCK_PWM         = 1 << 4,
>          STMPE_BLOCK_ROTATOR     = 1 << 5,
> };
> 
> Apparently the same hardware designers are involved.
> 

Or the firmware developers were heavely inspired by STMPE!

>> +int mfx_reg_write(struct mfx *mfx, u8 reg, u8 data);
>> +int mfx_reg_read(struct mfx *mfx, u8 reg);
>> +int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values);
>> +int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values);
>> +int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val);
> 
> Do you need this? Can't you just use regmap and pass
> around a struct regmap *map to access registers?
> 
> You don't necessarily need to use the default I2C regmap
> (like, e.g. drivers/mfd/stw481x.c) but even if a more
> complex access pattern is used to read/write registers
> I am pretty sure you can use regmap for it.
> 

Yes, Lee has also raised this point.

>> +int mfx_enable(struct mfx *mfx, unsigned int blocks);
>> +int mfx_disable(struct mfx *mfx, unsigned int blocks);
> 
> This is similar to
> extern int stmpe_enable(struct stmpe *stmpe, unsigned int blocks);
> extern int stmpe_disable(struct stmpe *stmpe, unsigned int blocks);
> 
> So again I am suspicious about duplication of driver code.
> 
> It even looks a bit like this driver started as a copy of
> the STMPE driver, which is not a good sign. It might be
> that it was copied from there because the hardware is
> actually very similar.
> 

HW is different, FW looks like similar.

>> +/* General */
>> +#define MFX_REG_CHIP_ID                        0x00 /* R */
>> +#define MFX_REG_FW_VERSION_MSB         0x01 /* R */
>> +#define MFX_REG_FW_VERSION_LSB         0x02 /* R */
>> +#define MFX_REG_SYS_CTRL               0x40 /* RW */
> 
> The STMPE driver defines enumerated registers in
> include/linux/mfd/stmpe.h
> then assign each variant using the model specifics in
> drivers/mfd/stmpe.h
> 
> This doesn't seem super much different.
> 
> Even if the old STMPE driver may be a bad fit, is is better
> to improve that (e.g. migrate it to use regmap and rewrite the
> stmpe-gpio.c driver to use pin control) and use also for this
> driver, or write a new driver from scratch like this?
> 
> I'm not so sure.
> 
> I do know that developers not always like to take out old
> hardware and old development boards and start hacking
> away before they can get some nice new hardware going
> but I am worried that this may be one of those cases where
> a serious cleanup of the aging STMPE driver may be a
> first necessary step.
> 
Just looking at [1], we see that the STMPE remaining active are the 
STMPE1600 and STMPE1801. All the others are obsolete.

[1] 
http://www.st.com/en/interfaces-and-transceivers/i-o-expanders.html?querycriteria=productId=SC1027

>> +/* IRQ output management */
>> +#define MFX_REG_IRQ_OUT_PIN            0x41 /* RW */
>> +#define MFX_REG_IRQ_SRC_EN             0x42 /* RW */
>> +#define MFX_REG_IRQ_PENDING            0x08 /* R */
>> +#define MFX_REG_IRQ_ACK                        0x44 /* RW */
> 
> Very similar to STMPE it seems.
> 

IRQ_OUT_PIN is different (edge not supported, open drain or push pull 
output) and IRQ_ACK new.

>> +/* MFX_REG_SYS_CTRL bitfields */
>> +#define MFX_REG_SYS_CTRL_GPIO_EN       BIT(0)
>> +#define MFX_REG_SYS_CTRL_TS_EN         BIT(1)
>> +#define MFX_REG_SYS_CTRL_IDD_EN                BIT(2)
>> +#define MFX_REG_SYS_CTRL_ALTGPIO_EN    BIT(3)
> 
> I guess these blocks works the same as with STMPE,
> that you can only use one of them at the time?
> 
> What is altgpio?
> 

ALTGPIO enables the use of GPIO[23:16] only if IDD and/or TS is/are 
disabled. TS and IDD have priority, if IDD and TS are enabled, ALTGPIO 
is forced to 0 by MFX FW.
When IDD is used GPIO[19:16] can be used as GPIO.
When TS is used GPIO[24:20] can be used as GPIO.

>> +/* MFX_REG_IRQ_OUT_PIN bitfields */
>> +#define MFX_REG_IRQ_OUT_PIN_TYPE       BIT(0) /* 0-OD 1-PP */
>> +#define MFX_REG_IRQ_OUT_PIN_POL                BIT(1) /* 0-active LOW 1-active HIGH */
> 
> I have not read the patch yet. But just for notice:
> This output IRQ type needs to be handled as well.
> 
> Check the code in
> drivers/iio/common/st_sensors/st_sensors_trigger.c
> 
> To see how you can detect the properties of an IRQ
> to set the right polarity, and handling of open drain
> IRQ lines.
> 

Thanks, I will have a look on this driver.
Regards,
Amelie

> Yours,
> Linus Walleij
> 

[PATCH 3/6] gpio: Add GPIO support for the ST Multi-Function eXpander

[Amelie Delaunay]

ST Multi-Function eXpander (MFX) can be used as GPIO expander.
It has 16 fast GPIOs and can have 8 extra alternate GPIOs
when other MFX features are not enabled.

Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
---
 drivers/gpio/Kconfig                   |  10 +
 drivers/gpio/Makefile                  |   1 +
 drivers/gpio/gpio-st-mfx.c             | 589 +++++++++++++++++++++++++++++++++
 include/dt-bindings/gpio/st-mfx-gpio.h |  24 ++
 4 files changed, 624 insertions(+)
 create mode 100644 drivers/gpio/gpio-st-mfx.c
 create mode 100644 include/dt-bindings/gpio/st-mfx-gpio.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d6a8e85..7827af3 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1034,6 +1034,16 @@ config GPIO_STMPE
 	  This enables support for the GPIOs found on the STMPE I/O
 	  Expanders.
 
+config GPIO_ST_MFX
+	bool "ST-MFX GPIOs"
+	depends on MFD_ST_MFX
+	depends on OF_GPIO
+	select GPIOLIB_IRQCHIP
+	help
+	  GPIO driver for STMicroelectronics Multi-Function eXpander.
+
+	  This provides GPIO interface supporting inputs and outputs.
+
 config GPIO_TC3589X
 	bool "TC3589X GPIOs"
 	depends on MFD_TC3589X
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 4bc24fe..7013bfa 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_GPIO_SODAVILLE)	+= gpio-sodaville.o
 obj-$(CONFIG_GPIO_SPEAR_SPICS)	+= gpio-spear-spics.o
 obj-$(CONFIG_GPIO_STA2X11)	+= gpio-sta2x11.o
 obj-$(CONFIG_GPIO_STMPE)	+= gpio-stmpe.o
+obj-$(CONFIG_GPIO_ST_MFX) 	+= gpio-st-mfx.o
 obj-$(CONFIG_GPIO_STP_XWAY)	+= gpio-stp-xway.o
 obj-$(CONFIG_GPIO_SYSCON)	+= gpio-syscon.o
 obj-$(CONFIG_GPIO_TB10X)	+= gpio-tb10x.o
diff --git a/drivers/gpio/gpio-st-mfx.c b/drivers/gpio/gpio-st-mfx.c
new file mode 100644
index 0000000..4e5235f
--- /dev/null
+++ b/drivers/gpio/gpio-st-mfx.c
@@ -0,0 +1,589 @@
+/*
+ * STMicroelectronics Multi-Function eXpander (ST-MFX) - GPIO expander driver.
+ *
+ * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
+ * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.
+ *
+ * License terms: GPL V2.0.
+ *
+ * st-mfx-gpio is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * st-mfx-gpio is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * st-mfx-gpio. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/mfd/st-mfx.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+
+/* MFX has a maximum of 24 gpios, with 8 gpios per bank, so 3 banks maximum */
+#define NR_MAX_GPIOS		24
+#define NR_GPIOS_PER_BANK	8
+#define NR_MAX_BANKS		(NR_MAX_GPIOS / NR_GPIOS_PER_BANK)
+#define get_bank(offset)	((u8)((offset) / NR_GPIOS_PER_BANK))
+#define get_mask(offset)	((u8)BIT((offset) % NR_GPIOS_PER_BANK))
+
+/*
+ * These registers are modified under the .irq_bus_lock and cached to avoid
+ * unnecessary writes in .irq_bus_sync_unlock.
+ */
+enum { REG_IRQ_SRC, REG_IRQ_EVT, REG_IRQ_TYPE, NR_CACHE_IRQ_REGS };
+
+/*
+ * This is MFX-specific flags, overloading Linux-specific of_gpio_flags,
+ * needed in of_xlate callback.
+ * on MFX: - if GPIO is output:
+ *		* (0) means PUSH_PULL
+ *		* OF_GPIO_SINGLE_ENDED (=2) means OPEN-DRAIN
+ *	   - if GPIO is input:
+ *		* (0) means NO_PULL
+ *		* OF_MFX_GPI_PUSH_PULL (=2) means PUSH_PULL
+ *
+ *		* OF_MFX_GPIO_PULL_UP programs pull up resistor on the GPIO,
+ *		  whatever its direction, without this flag, depending on
+ *		  GPIO type and direction, it programs either no pull or
+ *		  pull down resistor.
+ */
+enum of_mfx_gpio_flags {
+	OF_MFX_GPI_PUSH_PULL = 0x2,
+	OF_MFX_GPIO_PULL_UP = 0x4,
+};
+
+struct mfx_gpio {
+	struct gpio_chip gc;
+	struct mfx *mfx;
+	struct device *dev;
+	struct mutex irq_lock; /* IRQ bus lock */
+	u32 flags[NR_MAX_GPIOS];
+	/* Caches of interrupt control registers for bus_lock */
+	u8 regs[NR_CACHE_IRQ_REGS][NR_MAX_BANKS];
+	u8 oldregs[NR_CACHE_IRQ_REGS][NR_MAX_BANKS];
+};
+
+static int mfx_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	struct mfx *mfx = mfx_gpio->mfx;
+	u8 reg = MFX_REG_GPIO_STATE + get_bank(offset);
+	u8 mask = get_mask(offset);
+	int ret;
+
+	ret = mfx_reg_read(mfx, reg);
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & mask);
+}
+
+static void mfx_gpio_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	struct mfx *mfx = mfx_gpio->mfx;
+	int which = val ? MFX_REG_GPO_SET : MFX_REG_GPO_CLR;
+	u8 reg = which + get_bank(offset);
+	u8 mask = get_mask(offset);
+
+	mfx_reg_write(mfx, reg, mask);
+}
+
+static int mfx_gpio_get_direction(struct gpio_chip *gc,
+				  unsigned int offset)
+{
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	struct mfx *mfx = mfx_gpio->mfx;
+	u8 reg = MFX_REG_GPIO_DIR + get_bank(offset);
+	u8 mask = get_mask(offset);
+	int ret;
+
+	ret = mfx_reg_read(mfx, reg);
+	if (ret < 0)
+		return ret;
+
+	return !(ret & mask);
+}
+
+static int mfx_gpio_direction_input(struct gpio_chip *gc,
+				    unsigned int offset)
+{
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	struct mfx *mfx = mfx_gpio->mfx;
+	u8 reg_type = MFX_REG_GPIO_TYPE + get_bank(offset);
+	u8 reg_pupd = MFX_REG_GPIO_PUPD + get_bank(offset);
+	u8 reg_dir = MFX_REG_GPIO_DIR + get_bank(offset);
+	u8 mask = get_mask(offset);
+	int ret;
+
+	/*
+	 * In case of input direction, there is actually no way to apply some
+	 * configuration in hardware, as it can be done with
+	 * .set_config in case of output direction. That's why we apply
+	 * here the MFX specific-flags.
+	 */
+	if (mfx_gpio->flags[offset] & OF_MFX_GPI_PUSH_PULL)
+		ret = mfx_set_bits(mfx, reg_type, mask, mask);
+	else
+		ret = mfx_set_bits(mfx, reg_type, mask, 0);
+
+	if (ret)
+		return ret;
+
+	if (mfx_gpio->flags[offset] & OF_MFX_GPIO_PULL_UP)
+		ret = mfx_set_bits(mfx, reg_pupd, mask, mask);
+	else
+		ret = mfx_set_bits(mfx, reg_pupd, mask, 0);
+
+	if (ret)
+		return ret;
+
+	return mfx_set_bits(mfx, reg_dir, mask, 0);
+}
+
+static int mfx_gpio_direction_output(struct gpio_chip *gc,
+				     unsigned int offset, int val)
+{
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	struct mfx *mfx = mfx_gpio->mfx;
+	u8 reg = MFX_REG_GPIO_DIR + get_bank(offset);
+	u8 mask = get_mask(offset);
+
+	mfx_gpio_set(gc, offset, val);
+
+	return mfx_set_bits(mfx, reg, mask, mask);
+}
+
+static int mfx_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
+			       unsigned long config)
+{
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	struct mfx *mfx = mfx_gpio->mfx;
+	u8 reg_type = MFX_REG_GPIO_TYPE + get_bank(offset);
+	u8 reg_pupd = MFX_REG_GPIO_PUPD + get_bank(offset);
+	u8 mask = get_mask(offset);
+	int ret;
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		ret = mfx_set_bits(mfx, reg_type, mask, mask);
+		if (ret)
+			return ret;
+		return mfx_set_bits(mfx, reg_pupd, mask, 0);
+	case PIN_CONFIG_DRIVE_OPEN_SOURCE:
+		ret = mfx_set_bits(mfx, reg_type, mask, mask);
+		if (ret)
+			return ret;
+		return mfx_set_bits(mfx, reg_pupd, mask, mask);
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		ret = mfx_set_bits(mfx, reg_type, mask, 0);
+		if (ret)
+			return ret;
+		return mfx_set_bits(mfx, reg_pupd, mask, 0);
+	default:
+		break;
+	}
+
+	return -ENOTSUPP;
+}
+
+static void mfx_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
+{
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	struct mfx *mfx = mfx_gpio->mfx;
+	u16 i;
+
+	for (i = 0; i < gc->ngpio; i++) {
+		int gpio = i + gc->base;
+		const char *label = gpiochip_is_requested(gc, i);
+		int dir = mfx_gpio_get_direction(gc, i);
+		int val = mfx_gpio_get(gc, i);
+		u8 mask = get_mask(i);
+		u8 reg;
+		int type, pupd;
+		int irqsrc, irqevt, irqtype, irqpending;
+
+		if (!label)
+			label = "Unrequested";
+
+		seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, label);
+
+		reg = MFX_REG_GPIO_TYPE + get_bank(i);
+		type = mfx_reg_read(mfx, reg);
+		if (type < 0)
+			return;
+		type = !!(type & mask);
+
+		reg = MFX_REG_GPIO_PUPD + get_bank(i);
+		pupd = mfx_reg_read(mfx, reg);
+		if (pupd < 0)
+			return;
+		pupd = !!(pupd & mask);
+
+		reg = MFX_REG_IRQ_GPI_SRC + get_bank(i);
+		irqsrc = mfx_reg_read(mfx, reg);
+		if (irqsrc < 0)
+			return;
+		irqsrc = !!(irqsrc & mask);
+
+		reg = MFX_REG_IRQ_GPI_EVT + get_bank(i);
+		irqevt = mfx_reg_read(mfx, reg);
+		if (irqevt < 0)
+			return;
+		irqevt = !!(irqevt & mask);
+
+		reg = MFX_REG_IRQ_GPI_TYPE + get_bank(i);
+		irqtype = mfx_reg_read(mfx, reg);
+		if (irqtype < 0)
+			return;
+		irqtype = !!(irqtype & mask);
+
+		reg = MFX_REG_IRQ_GPI_PENDING + get_bank(i);
+		irqpending = mfx_reg_read(mfx, reg);
+		if (irqpending < 0)
+			return;
+		irqpending = !!(irqpending & mask);
+
+		if (!dir) {
+			seq_printf(s, "out %s ", val ? "high" : "low");
+			if (type)
+				seq_puts(s, "open drain ");
+			else
+				seq_puts(s, "push pull ");
+			if (pupd && type)
+				seq_puts(s, "with internal pull-up ");
+			else
+				seq_puts(s, "without internal pull-up ");
+		} else {
+			seq_printf(s, "in %s ", val ? "high" : "low");
+			if (type)
+				seq_printf(s, "with internal pull-%s ",
+					   pupd ? "up" : "down");
+			else
+				seq_printf(s, "%s ",
+					   pupd ? "floating" : "analog");
+		}
+
+		if (irqsrc) {
+			seq_printf(s, "IRQ generated on %s %s",
+				   !irqevt ?
+				   (!irqtype ? "Low level" : "High level") :
+				   (!irqtype ? "Falling edge" : "Rising edge"),
+				   irqpending ? "(pending)" : "");
+		}
+
+		seq_puts(s, "\n");
+	}
+}
+
+int mfx_gpio_of_xlate(struct gpio_chip *gc,
+		      const struct of_phandle_args *gpiospec, u32 *flags)
+{
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	int ret;
+
+	ret = of_gpio_simple_xlate(gc, gpiospec, flags);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * In atomic context here, we can't access registers over I2C,
+	 * that's why gpio flags are stored to be applied later.
+	 */
+	mfx_gpio->flags[gpiospec->args[0]] = gpiospec->args[1];
+
+	return ret;
+}
+
+static const struct gpio_chip mfx_gpio_chip = {
+	.label			= "MFX_GPIO",
+	.owner			= THIS_MODULE,
+	.get_direction		= mfx_gpio_get_direction,
+	.direction_input	= mfx_gpio_direction_input,
+	.direction_output	= mfx_gpio_direction_output,
+	.get			= mfx_gpio_get,
+	.set			= mfx_gpio_set,
+	.set_config		= mfx_gpio_set_config,
+	.dbg_show		= mfx_gpio_dbg_show,
+	.can_sleep		= true,
+	.of_gpio_n_cells	= 2,
+	.of_xlate		= mfx_gpio_of_xlate,
+};
+
+static void mfx_gpio_irq_toggle_trigger(struct gpio_chip *gc, int offset)
+{
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	struct mfx *mfx = mfx_gpio->mfx;
+	u8 bank = get_bank(offset);
+	u8 mask = get_mask(offset);
+	int val = mfx_gpio_get(gc, offset);
+
+	if (val < 0) {
+		dev_err(mfx_gpio->dev, "can't get value of gpio%d: ret=%d\n",
+			offset, val);
+		return;
+	}
+
+	if (val) {
+		mfx_gpio->regs[REG_IRQ_TYPE][bank] &= ~mask;
+		mfx_set_bits(mfx, MFX_REG_IRQ_GPI_TYPE + bank, mask, 0);
+	} else {
+		mfx_gpio->regs[REG_IRQ_TYPE][bank] |= mask;
+		mfx_set_bits(mfx, MFX_REG_IRQ_GPI_TYPE + bank, mask, mask);
+	}
+}
+
+static int mfx_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	int bank = get_bank(d->hwirq);
+	int mask = get_mask(d->hwirq);
+
+	if ((type & IRQ_TYPE_LEVEL_LOW) || (type & IRQ_TYPE_LEVEL_HIGH))
+		mfx_gpio->regs[REG_IRQ_EVT][bank] &= ~mask;
+	else
+		mfx_gpio->regs[REG_IRQ_EVT][bank] |= mask;
+
+	if ((type & IRQ_TYPE_EDGE_RISING) || (type & IRQ_TYPE_LEVEL_HIGH))
+		mfx_gpio->regs[REG_IRQ_TYPE][bank] |= mask;
+	else
+		mfx_gpio->regs[REG_IRQ_TYPE][bank] &= ~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.
+	 */
+
+	return 0;
+}
+
+static void mfx_gpio_irq_lock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+
+	mutex_lock(&mfx_gpio->irq_lock);
+}
+
+static void mfx_gpio_irq_sync_unlock(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	struct mfx *mfx = mfx_gpio->mfx;
+	static const u8 cache_regs[] = {
+		[REG_IRQ_SRC] = MFX_REG_IRQ_GPI_SRC,
+		[REG_IRQ_EVT] = MFX_REG_IRQ_GPI_EVT,
+		[REG_IRQ_TYPE] = MFX_REG_IRQ_GPI_TYPE,
+	};
+	u8 i, bank;
+
+	/*
+	 * In case of (type & 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 (irqd_get_trigger_type(d) & IRQ_TYPE_EDGE_BOTH) {
+		int type;
+
+		if (mfx_gpio_get(gc, d->hwirq))
+			type = IRQ_TYPE_EDGE_FALLING;
+		else
+			type = IRQ_TYPE_EDGE_RISING;
+
+		mfx_gpio_irq_set_type(d, type);
+	}
+
+	for (i = 0; i < NR_CACHE_IRQ_REGS; i++) {
+		for (bank = 0; bank < NR_MAX_BANKS; bank++) {
+			u8 old = mfx_gpio->oldregs[i][bank];
+			u8 new = mfx_gpio->regs[i][bank];
+
+			if (new == old)
+				continue;
+
+			mfx_gpio->oldregs[i][bank] = new;
+			mfx_reg_write(mfx, cache_regs[i] + bank, new);
+		}
+	}
+
+	mutex_unlock(&mfx_gpio->irq_lock);
+}
+
+static void mfx_gpio_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	int offset = d->hwirq;
+	u8 bank = get_bank(offset);
+	u8 mask = get_mask(offset);
+
+	mfx_gpio->regs[REG_IRQ_SRC][bank] &= ~mask;
+}
+
+static void mfx_gpio_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
+	int offset = d->hwirq;
+	u8 bank = get_bank(offset);
+	u8 mask = get_mask(offset);
+
+	mfx_gpio->regs[REG_IRQ_SRC][bank] |= mask;
+}
+
+static struct irq_chip mfx_gpio_irq_chip = {
+	.name			= "mfx-gpio",
+	.irq_bus_lock		= mfx_gpio_irq_lock,
+	.irq_bus_sync_unlock	= mfx_gpio_irq_sync_unlock,
+	.irq_mask		= mfx_gpio_irq_mask,
+	.irq_unmask		= mfx_gpio_irq_unmask,
+	.irq_set_type		= mfx_gpio_irq_set_type,
+};
+
+static irqreturn_t mfx_gpio_irq(int irq, void *dev)
+{
+	struct mfx_gpio *mfx_gpio = dev;
+	struct mfx *mfx = mfx_gpio->mfx;
+	int num_banks = mfx->num_gpio / 8;
+	u8 status[num_banks];
+	int ret;
+	u8 bank;
+
+	ret = mfx_block_read(mfx, MFX_REG_IRQ_GPI_PENDING, ARRAY_SIZE(status),
+			     status);
+	if (ret < 0) {
+		dev_err(mfx_gpio->dev, "can't get IRQ_GPI_PENDING: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	for (bank = 0; bank < ARRAY_SIZE(status); bank++) {
+		u8 stat = status[bank];
+
+		if (!stat)
+			continue;
+
+		while (stat) {
+			int bit = __ffs(stat);
+			int offset = bank * NR_GPIOS_PER_BANK + bit;
+			int gpio_irq = irq_find_mapping(mfx_gpio->gc.irq.domain,
+							offset);
+			int irq_trigger = irq_get_trigger_type(gpio_irq);
+
+			handle_nested_irq(gpio_irq);
+			stat &= ~(BIT(bit));
+
+			if (irq_trigger & IRQ_TYPE_EDGE_BOTH)
+				mfx_gpio_irq_toggle_trigger(&mfx_gpio->gc,
+							    offset);
+		}
+
+		mfx_reg_write(mfx, MFX_REG_IRQ_GPI_ACK + bank, status[bank]);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int mfx_gpio_probe(struct platform_device *pdev)
+{
+	struct mfx *mfx = dev_get_drvdata(pdev->dev.parent);
+	struct mfx_gpio *mfx_gpio;
+	int irq;
+	int ret;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "failed to get irq: %d\n", irq);
+
+		return irq;
+	}
+
+	mfx_gpio = devm_kzalloc(&pdev->dev, sizeof(struct mfx_gpio),
+				GFP_KERNEL);
+	if (!mfx_gpio)
+		return -ENOMEM;
+
+	mutex_init(&mfx_gpio->irq_lock);
+
+	mfx_gpio->dev = &pdev->dev;
+	mfx_gpio->mfx = mfx;
+
+	mfx_gpio->gc = mfx_gpio_chip;
+	mfx_gpio->gc.ngpio = mfx->num_gpio;
+	mfx_gpio->gc.parent = &pdev->dev;
+	mfx_gpio->gc.base = -1;
+	mfx_gpio->gc.of_node = pdev->dev.of_node;
+
+	if (mfx->blocks & MFX_BLOCK_ALTGPIO)
+		ret = mfx_enable(mfx, MFX_BLOCK_GPIO | MFX_BLOCK_ALTGPIO);
+	else
+		ret = mfx_enable(mfx, MFX_BLOCK_GPIO);
+
+	if (ret < 0)
+		return ret;
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq,
+					NULL, mfx_gpio_irq, IRQF_ONESHOT,
+					"mfx-gpio", mfx_gpio);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to request irq: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &mfx_gpio->gc, mfx_gpio);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to add gpiochip: %d\n", ret);
+		return ret;
+	}
+
+	ret = gpiochip_irqchip_add_nested(&mfx_gpio->gc, &mfx_gpio_irq_chip, 0,
+					  handle_simple_irq, IRQ_TYPE_NONE);
+	if (ret) {
+		dev_err(&pdev->dev, "could not connect irqchip to gpiochip\n");
+		return ret;
+	}
+
+	gpiochip_set_nested_irqchip(&mfx_gpio->gc, &mfx_gpio_irq_chip, irq);
+
+	platform_set_drvdata(pdev, mfx_gpio);
+
+	dev_info(&pdev->dev, "ST-MFX (GPIO) initialized\n");
+
+	return 0;
+}
+
+static int mfx_gpio_remove(struct platform_device *pdev)
+{
+	struct mfx *mfx = dev_get_drvdata(pdev->dev.parent);
+
+	return mfx_disable(mfx, MFX_BLOCK_GPIO | MFX_BLOCK_ALTGPIO);
+}
+
+static const struct of_device_id mfx_gpio_of_match[] = {
+	{ .compatible = "st,mfx-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mfx_of_match);
+
+static struct platform_driver mfx_gpio_driver = {
+	.driver = {
+		.name	= "st-mfx-gpio",
+		.of_match_table = mfx_gpio_of_match,
+	},
+	.probe		= mfx_gpio_probe,
+	.remove		= mfx_gpio_remove,
+};
+module_platform_driver(mfx_gpio_driver);
+
+MODULE_DESCRIPTION("STMicroelectronics Multi-Function eXpander GPIO driver");
+MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@st.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/dt-bindings/gpio/st-mfx-gpio.h b/include/dt-bindings/gpio/st-mfx-gpio.h
new file mode 100644
index 0000000..78dc7c5
--- /dev/null
+++ b/include/dt-bindings/gpio/st-mfx-gpio.h
@@ -0,0 +1,24 @@
+/*
+ * This header provides constants for binding st,mfx-gpio.
+ *
+ * The first cell in ST MFX GPIO specifier is the gpio number.
+ *
+ * The second cell contains standard flag values specified in gpio.h and
+ * specific flag values described here.
+ */
+
+#ifndef _DT_BINDINGS_GPIO_ST_MFX_GPIO_H
+#define _DT_BINDINGS_GPIO_ST_MFX_GPIO_H
+
+#include <dt-bindings/gpio/gpio.h>
+
+#define GPIO_IN_NO_PULL		0
+#define GPIO_IN_PUSH_PULL	2
+#define GPIO_OUT_PUSH_PULL	GPIO_PUSH_PULL
+#define GPIO_OUT_OPEN_DRAIN	GPIO_SINGLE_ENDED
+
+#define GPIO_NO_PULL		0
+#define GPIO_PULL_DOWN		0
+#define GPIO_PULL_UP		4
+
+#endif
-- 
2.7.4

Re: [PATCH 3/6] gpio: Add GPIO support for the ST Multi-Function eXpander

[Andy Shevchenko]

On Thu, Feb 8, 2018 at 4:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:
> ST Multi-Function eXpander (MFX) can be used as GPIO expander.
> It has 16 fast GPIOs and can have 8 extra alternate GPIOs
> when other MFX features are not enabled.

> +config GPIO_ST_MFX
> +       bool "ST-MFX GPIOs"
> +       depends on MFD_ST_MFX
> +       depends on OF_GPIO
> +       select GPIOLIB_IRQCHIP
> +       help
> +         GPIO driver for STMicroelectronics Multi-Function eXpander.
> +
> +         This provides GPIO interface supporting inputs and outputs.

> +/*
> + * STMicroelectronics Multi-Function eXpander (ST-MFX) - GPIO expander driver.
> + *
> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
> + * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.

> + * License terms: GPL V2.0.
> + *
> + * st-mfx-gpio is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * st-mfx-gpio is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * st-mfx-gpio. If not, see <http://www.gnu.org/licenses/>.

Use SPDX instead.

> + */

> +#include <linux/of_gpio.h>

> +/* MFX has a maximum of 24 gpios, with 8 gpios per bank, so 3 banks maximum */
> +#define NR_MAX_GPIOS           24
> +#define NR_GPIOS_PER_BANK      8
> +#define NR_MAX_BANKS           (NR_MAX_GPIOS / NR_GPIOS_PER_BANK)

> +#define get_bank(offset)       ((u8)((offset) / NR_GPIOS_PER_BANK))
> +#define get_mask(offset)       ((u8)BIT((offset) % NR_GPIOS_PER_BANK))

I would rather keep it consistent, i.e.
get_bank() [as is]
get_mask -> get_shift() [w/o BIT() macro]

> +enum { REG_IRQ_SRC, REG_IRQ_EVT, REG_IRQ_TYPE, NR_CACHE_IRQ_REGS };

Please, do one item per line.

> +/*
> + * This is MFX-specific flags, overloading Linux-specific of_gpio_flags,
> + * needed in of_xlate callback.

> + * on MFX: - if GPIO is output:

Split to two lines.

> + *             * (0) means PUSH_PULL
> + *             * OF_GPIO_SINGLE_ENDED (=2) means OPEN-DRAIN
> + *        - if GPIO is input:
> + *             * (0) means NO_PULL
> + *             * OF_MFX_GPI_PUSH_PULL (=2) means PUSH_PULL
> + *
> + *             * OF_MFX_GPIO_PULL_UP programs pull up resistor on the GPIO,
> + *               whatever its direction, without this flag, depending on
> + *               GPIO type and direction, it programs either no pull or
> + *               pull down resistor.
> + */
> +enum of_mfx_gpio_flags {
> +       OF_MFX_GPI_PUSH_PULL = 0x2,
> +       OF_MFX_GPIO_PULL_UP = 0x4,

These have misleading prefix. OF_ is reserved for general OF stuff.

> +};
> +
> +struct mfx_gpio {
> +       struct gpio_chip gc;
> +       struct mfx *mfx;

> +       struct device *dev;

Don't you have it in gc above as a parent device?

> +       struct mutex irq_lock; /* IRQ bus lock */
> +       u32 flags[NR_MAX_GPIOS];
> +       /* Caches of interrupt control registers for bus_lock */
> +       u8 regs[NR_CACHE_IRQ_REGS][NR_MAX_BANKS];
> +       u8 oldregs[NR_CACHE_IRQ_REGS][NR_MAX_BANKS];
> +};

> +static int mfx_gpio_direction_input(struct gpio_chip *gc,
> +                                   unsigned int offset)
> +{
> +       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
> +       struct mfx *mfx = mfx_gpio->mfx;
> +       u8 reg_type = MFX_REG_GPIO_TYPE + get_bank(offset);
> +       u8 reg_pupd = MFX_REG_GPIO_PUPD + get_bank(offset);
> +       u8 reg_dir = MFX_REG_GPIO_DIR + get_bank(offset);
> +       u8 mask = get_mask(offset);
> +       int ret;
> +
> +       /*
> +        * In case of input direction, there is actually no way to apply some
> +        * configuration in hardware, as it can be done with
> +        * .set_config in case of output direction. That's why we apply
> +        * here the MFX specific-flags.
> +        */
> +       if (mfx_gpio->flags[offset] & OF_MFX_GPI_PUSH_PULL)
> +               ret = mfx_set_bits(mfx, reg_type, mask, mask);
> +       else
> +               ret = mfx_set_bits(mfx, reg_type, mask, 0);

> +

Redundant empty line.

> +       if (ret)
> +               return ret;
> +
> +       if (mfx_gpio->flags[offset] & OF_MFX_GPIO_PULL_UP)
> +               ret = mfx_set_bits(mfx, reg_pupd, mask, mask);
> +       else
> +               ret = mfx_set_bits(mfx, reg_pupd, mask, 0);

> +

Ditto.

> +       if (ret)
> +               return ret;
> +
> +       return mfx_set_bits(mfx, reg_dir, mask, 0);
> +}

> +static void mfx_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
> +{
> +       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
> +       struct mfx *mfx = mfx_gpio->mfx;
> +       u16 i;
> +
> +       for (i = 0; i < gc->ngpio; i++) {
> +               int gpio = i + gc->base;
> +               const char *label = gpiochip_is_requested(gc, i);
> +               int dir = mfx_gpio_get_direction(gc, i);
> +               int val = mfx_gpio_get(gc, i);
> +               u8 mask = get_mask(i);
> +               u8 reg;
> +               int type, pupd;
> +               int irqsrc, irqevt, irqtype, irqpending;

> +               if (!label)
> +                       label = "Unrequested";

I would rather put label = gpiochip_...(); immediately before this
condition for better readability.

> +
> +               seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, label);
> +

> +               reg = MFX_REG_IRQ_GPI_PENDING + get_bank(i);
> +               irqpending = mfx_reg_read(mfx, reg);
> +               if (irqpending < 0)
> +                       return;
> +               irqpending = !!(irqpending & mask);
> +

> +               if (!dir) {

Why not to use

if (dir) {

?

> +                       seq_printf(s, "out %s ", val ? "high" : "low");
> +                       if (type)
> +                               seq_puts(s, "open drain ");
> +                       else
> +                               seq_puts(s, "push pull ");
> +                       if (pupd && type)
> +                               seq_puts(s, "with internal pull-up ");
> +                       else
> +                               seq_puts(s, "without internal pull-up ");
> +               } else {
> +                       seq_printf(s, "in %s ", val ? "high" : "low");
> +                       if (type)
> +                               seq_printf(s, "with internal pull-%s ",
> +                                          pupd ? "up" : "down");
> +                       else
> +                               seq_printf(s, "%s ",
> +                                          pupd ? "floating" : "analog");
> +               }
> +
> +               if (irqsrc) {
> +                       seq_printf(s, "IRQ generated on %s %s",
> +                                  !irqevt ?
> +                                  (!irqtype ? "Low level" : "High level") :
> +                                  (!irqtype ? "Falling edge" : "Rising edge"),
> +                                  irqpending ? "(pending)" : "");

Why do you use negative conditions? Use positive ones.

> +               }
> +
> +               seq_puts(s, "\n");
> +       }
> +}

> +static void mfx_gpio_irq_toggle_trigger(struct gpio_chip *gc, int offset)
> +{
> +       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
> +       struct mfx *mfx = mfx_gpio->mfx;
> +       u8 bank = get_bank(offset);
> +       u8 mask = get_mask(offset);
> +       int val = mfx_gpio_get(gc, offset);
> +

> +       if (val < 0) {
> +               dev_err(mfx_gpio->dev, "can't get value of gpio%d: ret=%d\n",
> +                       offset, val);

Is it somehow useful on err level?

> +               return;
> +       }

> +}

> +static int mfx_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
> +       int bank = get_bank(d->hwirq);
> +       int mask = get_mask(d->hwirq);
> +

> +       if ((type & IRQ_TYPE_LEVEL_LOW) || (type & IRQ_TYPE_LEVEL_HIGH))

IRQ_TYPE_LEVEL_MASK?

> +               mfx_gpio->regs[REG_IRQ_EVT][bank] &= ~mask;
> +       else
> +               mfx_gpio->regs[REG_IRQ_EVT][bank] |= mask;
> +
> +       if ((type & IRQ_TYPE_EDGE_RISING) || (type & IRQ_TYPE_LEVEL_HIGH))

IRQ_TYPE_EDGE_BOTH ?

> +               mfx_gpio->regs[REG_IRQ_TYPE][bank] |= mask;
> +       else
> +               mfx_gpio->regs[REG_IRQ_TYPE][bank] &= ~mask;
> +

> +}

> +static void mfx_gpio_irq_lock(struct irq_data *d)

Missed annotation that this acquires a lock.

> +static void mfx_gpio_irq_sync_unlock(struct irq_data *d)

Ditto for releasing lock.

> +static irqreturn_t mfx_gpio_irq(int irq, void *dev)
> +{
> +       struct mfx_gpio *mfx_gpio = dev;
> +       struct mfx *mfx = mfx_gpio->mfx;
> +       int num_banks = mfx->num_gpio / 8;
> +       u8 status[num_banks];

> +       int ret;
> +       u8 bank;

Swap lines.

> +
> +       ret = mfx_block_read(mfx, MFX_REG_IRQ_GPI_PENDING, ARRAY_SIZE(status),
> +                            status);
> +       if (ret < 0) {

> +               dev_err(mfx_gpio->dev, "can't get IRQ_GPI_PENDING: %d\n", ret);

In IRQ context on err level? Hmm...

> +               return IRQ_NONE;
> +       }
> +
> +       for (bank = 0; bank < ARRAY_SIZE(status); bank++) {
> +               u8 stat = status[bank];
> +
> +               if (!stat)
> +                       continue;
> +

> +               while (stat) {
> +                       int bit = __ffs(stat);

for_each_set_bit() ?

> +                       int offset = bank * NR_GPIOS_PER_BANK + bit;
> +                       int gpio_irq = irq_find_mapping(mfx_gpio->gc.irq.domain,
> +                                                       offset);
> +                       int irq_trigger = irq_get_trigger_type(gpio_irq);
> +
> +                       handle_nested_irq(gpio_irq);
> +                       stat &= ~(BIT(bit));
> +
> +                       if (irq_trigger & IRQ_TYPE_EDGE_BOTH)
> +                               mfx_gpio_irq_toggle_trigger(&mfx_gpio->gc,
> +                                                           offset);
> +               }
> +
> +               mfx_reg_write(mfx, MFX_REG_IRQ_GPI_ACK + bank, status[bank]);
> +       }
> +
> +       return IRQ_HANDLED;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] gpio: Add GPIO support for the ST Multi-Function eXpander

[Amelie DELAUNAY]

On 02/14/2018 04:30 PM, Andy Shevchenko wrote:
> On Thu, Feb 8, 2018 at 4:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:
>> ST Multi-Function eXpander (MFX) can be used as GPIO expander.
>> It has 16 fast GPIOs and can have 8 extra alternate GPIOs
>> when other MFX features are not enabled.
> 
>> +config GPIO_ST_MFX
>> +       bool "ST-MFX GPIOs"
>> +       depends on MFD_ST_MFX
>> +       depends on OF_GPIO
>> +       select GPIOLIB_IRQCHIP
>> +       help
>> +         GPIO driver for STMicroelectronics Multi-Function eXpander.
>> +
>> +         This provides GPIO interface supporting inputs and outputs.
> 
>> +/*
>> + * STMicroelectronics Multi-Function eXpander (ST-MFX) - GPIO expander driver.
>> + *
>> + * Copyright (C) 2017, STMicroelectronics - All Rights Reserved
>> + * Author(s): Amelie Delaunay <amelie.delaunay@st.com> for STMicroelectronics.
> 
>> + * License terms: GPL V2.0.
>> + *
>> + * st-mfx-gpio is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * st-mfx-gpio is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
>> + * details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * st-mfx-gpio. If not, see <http://www.gnu.org/licenses/>.
> 
> Use SPDX instead.
> 
OK.
>> + */
> 
>> +#include <linux/of_gpio.h>
> 
>> +/* MFX has a maximum of 24 gpios, with 8 gpios per bank, so 3 banks maximum */
>> +#define NR_MAX_GPIOS           24
>> +#define NR_GPIOS_PER_BANK      8
>> +#define NR_MAX_BANKS           (NR_MAX_GPIOS / NR_GPIOS_PER_BANK)
> 
>> +#define get_bank(offset)       ((u8)((offset) / NR_GPIOS_PER_BANK))
>> +#define get_mask(offset)       ((u8)BIT((offset) % NR_GPIOS_PER_BANK))
> 
> I would rather keep it consistent, i.e.
> get_bank() [as is]
> get_mask -> get_shift() [w/o BIT() macro]
> 
#define get_shift(offset)       ((u8)((offset) % NR_GPIOS_PER_BANK))
u8 mask = (u8)BIT(get_shift(offset));
I'm not sure consistency is more important than readability and 
convenience ?

>> +enum { REG_IRQ_SRC, REG_IRQ_EVT, REG_IRQ_TYPE, NR_CACHE_IRQ_REGS };
> 
> Please, do one item per line.
> 
OK.

>> +/*
>> + * This is MFX-specific flags, overloading Linux-specific of_gpio_flags,
>> + * needed in of_xlate callback.
> 
>> + * on MFX: - if GPIO is output:
> 
> Split to two lines.
> 
OK.

>> + *             * (0) means PUSH_PULL
>> + *             * OF_GPIO_SINGLE_ENDED (=2) means OPEN-DRAIN
>> + *        - if GPIO is input:
>> + *             * (0) means NO_PULL
>> + *             * OF_MFX_GPI_PUSH_PULL (=2) means PUSH_PULL
>> + *
>> + *             * OF_MFX_GPIO_PULL_UP programs pull up resistor on the GPIO,
>> + *               whatever its direction, without this flag, depending on
>> + *               GPIO type and direction, it programs either no pull or
>> + *               pull down resistor.
>> + */
>> +enum of_mfx_gpio_flags {
>> +       OF_MFX_GPI_PUSH_PULL = 0x2,
>> +       OF_MFX_GPIO_PULL_UP = 0x4,
> 
> These have misleading prefix. OF_ is reserved for general OF stuff.
> 
You're right. I will fix it in V2.

>> +};
>> +
>> +struct mfx_gpio {
>> +       struct gpio_chip gc;
>> +       struct mfx *mfx;
> 
>> +       struct device *dev;
> 
> Don't you have it in gc above as a parent device?
> 
Yes, I agree to remove it.

>> +       struct mutex irq_lock; /* IRQ bus lock */
>> +       u32 flags[NR_MAX_GPIOS];
>> +       /* Caches of interrupt control registers for bus_lock */
>> +       u8 regs[NR_CACHE_IRQ_REGS][NR_MAX_BANKS];
>> +       u8 oldregs[NR_CACHE_IRQ_REGS][NR_MAX_BANKS];
>> +};
> 
>> +static int mfx_gpio_direction_input(struct gpio_chip *gc,
>> +                                   unsigned int offset)
>> +{
>> +       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
>> +       struct mfx *mfx = mfx_gpio->mfx;
>> +       u8 reg_type = MFX_REG_GPIO_TYPE + get_bank(offset);
>> +       u8 reg_pupd = MFX_REG_GPIO_PUPD + get_bank(offset);
>> +       u8 reg_dir = MFX_REG_GPIO_DIR + get_bank(offset);
>> +       u8 mask = get_mask(offset);
>> +       int ret;
>> +
>> +       /*
>> +        * In case of input direction, there is actually no way to apply some
>> +        * configuration in hardware, as it can be done with
>> +        * .set_config in case of output direction. That's why we apply
>> +        * here the MFX specific-flags.
>> +        */
>> +       if (mfx_gpio->flags[offset] & OF_MFX_GPI_PUSH_PULL)
>> +               ret = mfx_set_bits(mfx, reg_type, mask, mask);
>> +       else
>> +               ret = mfx_set_bits(mfx, reg_type, mask, 0);
> 
>> +
> 
> Redundant empty line.
> 
OK.

>> +       if (ret)
>> +               return ret;
>> +
>> +       if (mfx_gpio->flags[offset] & OF_MFX_GPIO_PULL_UP)
>> +               ret = mfx_set_bits(mfx, reg_pupd, mask, mask);
>> +       else
>> +               ret = mfx_set_bits(mfx, reg_pupd, mask, 0);
> 
>> +
> 
> Ditto.
> 
OK.

>> +       if (ret)
>> +               return ret;
>> +
>> +       return mfx_set_bits(mfx, reg_dir, mask, 0);
>> +}
> 
>> +static void mfx_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
>> +{
>> +       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
>> +       struct mfx *mfx = mfx_gpio->mfx;
>> +       u16 i;
>> +
>> +       for (i = 0; i < gc->ngpio; i++) {
>> +               int gpio = i + gc->base;
>> +               const char *label = gpiochip_is_requested(gc, i);
>> +               int dir = mfx_gpio_get_direction(gc, i);
>> +               int val = mfx_gpio_get(gc, i);
>> +               u8 mask = get_mask(i);
>> +               u8 reg;
>> +               int type, pupd;
>> +               int irqsrc, irqevt, irqtype, irqpending;
> 
>> +               if (!label)
>> +                       label = "Unrequested";
> 
> I would rather put label = gpiochip_...(); immediately before this
> condition for better readability.
> 
You're right.

>> +
>> +               seq_printf(s, " gpio-%-3d (%-20.20s) ", gpio, label);
>> +
> 
>> +               reg = MFX_REG_IRQ_GPI_PENDING + get_bank(i);
>> +               irqpending = mfx_reg_read(mfx, reg);
>> +               if (irqpending < 0)
>> +                       return;
>> +               irqpending = !!(irqpending & mask);
>> +
> 
>> +               if (!dir) {
> 
> Why not to use
> 
> if (dir) {
> 
> ?
> 
My brain orders things in ascending order, I think it is the reason why 
this code manages false cases (=0) before the others.

>> +                       seq_printf(s, "out %s ", val ? "high" : "low");
>> +                       if (type)
>> +                               seq_puts(s, "open drain ");
>> +                       else
>> +                               seq_puts(s, "push pull ");
>> +                       if (pupd && type)
>> +                               seq_puts(s, "with internal pull-up ");
>> +                       else
>> +                               seq_puts(s, "without internal pull-up ");
>> +               } else {
>> +                       seq_printf(s, "in %s ", val ? "high" : "low");
>> +                       if (type)
>> +                               seq_printf(s, "with internal pull-%s ",
>> +                                          pupd ? "up" : "down");
>> +                       else
>> +                               seq_printf(s, "%s ",
>> +                                          pupd ? "floating" : "analog");
>> +               }
>> +
>> +               if (irqsrc) {
>> +                       seq_printf(s, "IRQ generated on %s %s",
>> +                                  !irqevt ?
>> +                                  (!irqtype ? "Low level" : "High level") :
>> +                                  (!irqtype ? "Falling edge" : "Rising edge"),
>> +                                  irqpending ? "(pending)" : "");
> 
> Why do you use negative conditions? Use positive ones.
> 
OK, I will make the effort in V2.
>> +               }
>> +
>> +               seq_puts(s, "\n");
>> +       }
>> +}
> 
>> +static void mfx_gpio_irq_toggle_trigger(struct gpio_chip *gc, int offset)
>> +{
>> +       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
>> +       struct mfx *mfx = mfx_gpio->mfx;
>> +       u8 bank = get_bank(offset);
>> +       u8 mask = get_mask(offset);
>> +       int val = mfx_gpio_get(gc, offset);
>> +
> 
>> +       if (val < 0) {
>> +               dev_err(mfx_gpio->dev, "can't get value of gpio%d: ret=%d\n",
>> +                       offset, val);
> 
> Is it somehow useful on err level?
> 
I can drop it.
>> +               return;
>> +       }
> 
>> +}
> 
>> +static int mfx_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>> +{
>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +       struct mfx_gpio *mfx_gpio = gpiochip_get_data(gc);
>> +       int bank = get_bank(d->hwirq);
>> +       int mask = get_mask(d->hwirq);
>> +
> 
>> +       if ((type & IRQ_TYPE_LEVEL_LOW) || (type & IRQ_TYPE_LEVEL_HIGH))
> 
> IRQ_TYPE_LEVEL_MASK?
> 
You're right.
>> +               mfx_gpio->regs[REG_IRQ_EVT][bank] &= ~mask;
>> +       else
>> +               mfx_gpio->regs[REG_IRQ_EVT][bank] |= mask;
>> +
>> +       if ((type & IRQ_TYPE_EDGE_RISING) || (type & IRQ_TYPE_LEVEL_HIGH))
> 
> IRQ_TYPE_EDGE_BOTH ?
> 
No, here I test EDGE_RISING and LEVEL_HIGH, so mixing EDGE and LEVEL type.
>> +               mfx_gpio->regs[REG_IRQ_TYPE][bank] |= mask;
>> +       else
>> +               mfx_gpio->regs[REG_IRQ_TYPE][bank] &= ~mask;
>> +
> 
>> +}
> 
>> +static void mfx_gpio_irq_lock(struct irq_data *d)
> 
> Missed annotation that this acquires a lock.
> 
It is .irq_bus_lock and .irq_bus_sync_unlock. Description is available 
in include/linux/irq.h:
  * @irq_bus_lock:	function to lock access to slow bus (i2c) chips
  * @irq_bus_sync_unlock:function to sync and unlock slow bus (i2c) chips
>> +static void mfx_gpio_irq_sync_unlock(struct irq_data *d)
> 
> Ditto for releasing lock.
> 
>> +static irqreturn_t mfx_gpio_irq(int irq, void *dev)
>> +{
>> +       struct mfx_gpio *mfx_gpio = dev;
>> +       struct mfx *mfx = mfx_gpio->mfx;
>> +       int num_banks = mfx->num_gpio / 8;
>> +       u8 status[num_banks];
> 
>> +       int ret;
>> +       u8 bank;
> 
> Swap lines.
> 
OK.
>> +
>> +       ret = mfx_block_read(mfx, MFX_REG_IRQ_GPI_PENDING, ARRAY_SIZE(status),
>> +                            status);
>> +       if (ret < 0) {
> 
>> +               dev_err(mfx_gpio->dev, "can't get IRQ_GPI_PENDING: %d\n", ret);
> 
> In IRQ context on err level? Hmm...
> 
OK I will remove it in V2.
>> +               return IRQ_NONE;
>> +       }
>> +
>> +       for (bank = 0; bank < ARRAY_SIZE(status); bank++) {
>> +               u8 stat = status[bank];
>> +
>> +               if (!stat)
>> +                       continue;
>> +
> 
>> +               while (stat) {
>> +                       int bit = __ffs(stat);
> 
> for_each_set_bit() ?
> 
You're right.
>> +                       int offset = bank * NR_GPIOS_PER_BANK + bit;
>> +                       int gpio_irq = irq_find_mapping(mfx_gpio->gc.irq.domain,
>> +                                                       offset);
>> +                       int irq_trigger = irq_get_trigger_type(gpio_irq);
>> +
>> +                       handle_nested_irq(gpio_irq);
>> +                       stat &= ~(BIT(bit));
>> +
>> +                       if (irq_trigger & IRQ_TYPE_EDGE_BOTH)
>> +                               mfx_gpio_irq_toggle_trigger(&mfx_gpio->gc,
>> +                                                           offset);
>> +               }
>> +
>> +               mfx_reg_write(mfx, MFX_REG_IRQ_GPI_ACK + bank, status[bank]);
>> +       }
>> +
>> +       return IRQ_HANDLED;
>> +}
> 

Thanks for review,
Amelie

Re: [PATCH 3/6] gpio: Add GPIO support for the ST Multi-Function eXpander

[Linus Walleij]

On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:

> ST Multi-Function eXpander (MFX) can be used as GPIO expander.
> It has 16 fast GPIOs and can have 8 extra alternate GPIOs
> when other MFX features are not enabled.
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>

As mentioned I think this driver needs to:

- Move to drivers/pinctrl/pinctrl-stmfx.c similar to the sx150x controller.
- Pass a regmap around instead of the custom accessor functions.
- Maybe refactor and reuse STMPE infrastructure

Yours,
Linus Walleij

Re: [PATCH 3/6] gpio: Add GPIO support for the ST Multi-Function eXpander

[Amelie DELAUNAY]

On 02/22/2018 02:47 PM, Linus Walleij wrote:
> On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:
> 
>> ST Multi-Function eXpander (MFX) can be used as GPIO expander.
>> It has 16 fast GPIOs and can have 8 extra alternate GPIOs
>> when other MFX features are not enabled.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> 
> As mentioned I think this driver needs to:
> 
> - Move to drivers/pinctrl/pinctrl-stmfx.c similar to the sx150x controller.
> - Pass a regmap around instead of the custom accessor functions.

I study this right now!
Thanks,
Amelie

> - Maybe refactor and reuse STMPE infrastructure
> 
> Yours,
> Linus Walleij
> 

[PATCH 4/6] ARM: dts: stm32: add MFX support on I2C1 on stm32746g-eval

[Amelie Delaunay]

MFX is used as gpio expander on stm32746g-eval.

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

diff --git a/arch/arm/boot/dts/stm32746g-eval.dts b/arch/arm/boot/dts/stm32746g-eval.dts
index e74ae59..99739f7 100644
--- a/arch/arm/boot/dts/stm32746g-eval.dts
+++ b/arch/arm/boot/dts/stm32746g-eval.dts
@@ -128,6 +128,23 @@
 	i2c-scl-rising-time-ns = <185>;
 	i2c-scl-falling-time-ns = <20>;
 	status = "okay";
+
+	mfx: mfx@42 {
+		compatible = "st,mfx";
+		reg = <0x42>;
+		interrupts = <8 1>;
+		interrupt-parent = <&gpioi>;
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		mfxgpio: mfx_gpio {
+			compatible = "st,mfx-gpio";
+			interrupts = <0>;
+			interrupt-parent = <&mfx>;
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
 };
 
 &mac {
-- 
2.7.4

Re: [PATCH 4/6] ARM: dts: stm32: add MFX support on I2C1 on stm32746g-eval

[Linus Walleij]

On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:

> MFX is used as gpio expander on stm32746g-eval.
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>

(...)
> +       mfx: mfx@42 {
> +               compatible = "st,mfx";
> +               reg = <0x42>;
> +               interrupts = <8 1>;
> +               interrupt-parent = <&gpioi>;
> +               interrupt-controller;
> +               #interrupt-cells = <1>;
> +
> +               mfxgpio: mfx_gpio {
> +                       compatible = "st,mfx-gpio";
> +                       interrupts = <0>;
> +                       interrupt-parent = <&mfx>;
> +                       gpio-controller;
> +                       #gpio-cells = <2>;

So I think this node should contain some pin config states
that can  be referenced by the drivers to set up push/pull
etc. Probably it can use just standard pin config properties
like sx150x.

Yours,
Linus Walleij

Re: [PATCH 4/6] ARM: dts: stm32: add MFX support on I2C1 on stm32746g-eval

[Amelie DELAUNAY]

On 02/22/2018 02:54 PM, Linus Walleij wrote:
> On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:
> 
>> MFX is used as gpio expander on stm32746g-eval.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> 
> (...)
>> +       mfx: mfx@42 {
>> +               compatible = "st,mfx";
>> +               reg = <0x42>;
>> +               interrupts = <8 1>;
>> +               interrupt-parent = <&gpioi>;
>> +               interrupt-controller;
>> +               #interrupt-cells = <1>;
>> +
>> +               mfxgpio: mfx_gpio {
>> +                       compatible = "st,mfx-gpio";
>> +                       interrupts = <0>;
>> +                       interrupt-parent = <&mfx>;
>> +                       gpio-controller;
>> +                       #gpio-cells = <2>;
> 
> So I think this node should contain some pin config states
> that can  be referenced by the drivers to set up push/pull
> etc. Probably it can use just standard pin config properties
> like sx150x.
> 

I will check that point.

Regards,
Amelie

> Yours,
> Linus Walleij
> 

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

[Amelie Delaunay]

The joystick on stm32746g-eval uses gpios on MFX gpio expander.

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

diff --git a/arch/arm/boot/dts/stm32746g-eval.dts b/arch/arm/boot/dts/stm32746g-eval.dts
index 99739f7..eb5f43b 100644
--- a/arch/arm/boot/dts/stm32746g-eval.dts
+++ b/arch/arm/boot/dts/stm32746g-eval.dts
@@ -43,6 +43,7 @@
 /dts-v1/;
 #include "stm32f746.dtsi"
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/gpio/st-mfx-gpio.h>
 
 / {
 	model = "STMicroelectronics STM32746g-EVAL board";
@@ -98,6 +99,36 @@
 		};
 	};
 
+	joystick {
+		compatible = "gpio-keys";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		button@1 {
+			label = "JoySel";
+			linux,code = <KEY_ENTER>;
+			gpios = <&mfxgpio 0 (GPIO_ACTIVE_LOW | GPIO_IN_PUSH_PULL | GPIO_PULL_UP)>;
+		};
+		button@2 {
+			label = "JoyDown";
+			linux,code = <KEY_DOWN>;
+			gpios = <&mfxgpio 1 (GPIO_ACTIVE_LOW | GPIO_IN_PUSH_PULL | GPIO_PULL_UP)>;
+		};
+		button@3 {
+			label = "JoyLeft";
+			linux,code = <KEY_LEFT>;
+			gpios = <&mfxgpio 2 (GPIO_ACTIVE_LOW | GPIO_IN_PUSH_PULL | GPIO_PULL_UP)>;
+		};
+		button@4 {
+			label = "JoyRight";
+			linux,code = <KEY_RIGHT>;
+			gpios = <&mfxgpio 3 (GPIO_ACTIVE_LOW | GPIO_IN_PUSH_PULL | GPIO_PULL_UP)>;
+		};
+		button@5 {
+			label = "JoyUp";
+			linux,code = <KEY_UP>;
+			gpios = <&mfxgpio 4 (GPIO_ACTIVE_LOW | GPIO_IN_PUSH_PULL | GPIO_PULL_UP)>;
+		};
+	};
 
 	mmc_vcard: mmc_vcard {
 		compatible = "regulator-fixed";
-- 
2.7.4

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

[Linus Walleij]

Hi Amelie,

thanks for your patch.

On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:

> The joystick on stm32746g-eval uses gpios on MFX gpio expander.
>
> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
(...)
> +       joystick {
> +               compatible = "gpio-keys";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               button@1 {
> +                       label = "JoySel";
> +                       linux,code = <KEY_ENTER>;
> +                       gpios = <&mfxgpio 0 (GPIO_ACTIVE_LOW | GPIO_IN_PUSH_PULL | GPIO_PULL_UP)>;
> +               };

As I think this should not all be handled by GPIO, the joystick on
gpio-keys needs a pin control handle pointing back to pin control
states on the respective pins in the pin control/GPIO driver,
where it can set up these properties.

For the individual lines the pushing and pulling flags should be
removed.

Yours,
Linus Walleij

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

[Amelie DELAUNAY]

On 02/22/2018 02:52 PM, Linus Walleij wrote:
> Hi Amelie,
> 
> thanks for your patch.
> 
> On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:
> 
>> The joystick on stm32746g-eval uses gpios on MFX gpio expander.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>
> (...)
>> +       joystick {
>> +               compatible = "gpio-keys";
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               button@1 {
>> +                       label = "JoySel";
>> +                       linux,code = <KEY_ENTER>;
>> +                       gpios = <&mfxgpio 0 (GPIO_ACTIVE_LOW | GPIO_IN_PUSH_PULL | GPIO_PULL_UP)>;
>> +               };
> 
> As I think this should not all be handled by GPIO, the joystick on
> gpio-keys needs a pin control handle pointing back to pin control
> states on the respective pins in the pin control/GPIO driver,
> where it can set up these properties.
> 
> For the individual lines the pushing and pulling flags should be
> removed.
> 

Sure! I keep this in mind, thanks!

Regards,
Amelie

> Yours,
> Linus Walleij
> 

Re: [PATCH 0/6] Introduce STMicroelectronics MultiFunction eXpander

[Linus Walleij]

Hi Amelie,

thanks a lot for your patches!

On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:

> This series adds support for STMicroelectronics MultiFunction eXpander
> (ST MFX), used on some STM32 discovery and evaluation boards.

So I take it that the department creating the STMPE
"ST MPE" or "ST Microelectronics MultiPurpose Expander"
Now created a new set of circuits?

Can we first establish whether this new family is really so
different from STMPE that it really needs a new driver in
MFD and GPIO (and I guess pin control as well)?

I would be annoyed to see later that it is just a few bytes
separating it from the STMPE, in that case I think it is
better to just reuse/improve the good old stmpe driver
and take it from there.

Yours,
Linus Walleij

Re: [PATCH 0/6] Introduce STMicroelectronics MultiFunction eXpander

[Amelie DELAUNAY]

Hi Linus,

Thanks for your review on the whole series.

On 02/22/2018 02:06 PM, Linus Walleij wrote:
> Hi Amelie,
> 
> thanks a lot for your patches!
> 
> On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:
> 
>> This series adds support for STMicroelectronics MultiFunction eXpander
>> (ST MFX), used on some STM32 discovery and evaluation boards.
> 
> So I take it that the department creating the STMPE
> "ST MPE" or "ST Microelectronics MultiPurpose Expander"
> Now created a new set of circuits?
> 

ST MFX is based on a STM32L152 with a firmware offering features similar 
to the STMPE (GPIO and TS controller) but also IDD measurement to 
measure the power consumption of the MCU to which MFX is connected to.

> Can we first establish whether this new family is really so
> different from STMPE that it really needs a new driver in
> MFD and GPIO (and I guess pin control as well)?
> 

ST MFX is different from STMPE as far as the HW is completely different.
IDD is a new feature.
TS management is completely different.
GPIO management looks like but is also rather different.
ST MFX counts a first level of 8 interrupts (acked by writing in the ACK 
register), then a second level of 24 interrupts for GPIOs. GPIO IRQ can 
be triggered on low level, falling edge, high level, rising edge. GPIO 
IRQ have to be acked by writing in the GPI_ACK register, GPIO can be 
output open-drain with/without internal pull-up, output push-pull, input 
with pull-up/down, input floating or analog.

> I would be annoyed to see later that it is just a few bytes
> separating it from the STMPE, in that case I think it is
> better to just reuse/improve the good old stmpe driver
> and take it from there.
> 

Sure, but if it was a new STMPE variant, it would be called STMPE24xx or 
something like that (24 GPIOs maximum, 20 if IDD or TS is enabled, 16 if 
IDD and TS are enabled).

Regards,
Amelie

> Yours,
> Linus Walleij
> 

Re: [PATCH 0/6] Introduce STMicroelectronics MultiFunction eXpander

[Linus Walleij]

On Thu, Feb 22, 2018 at 4:13 PM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:

> ST MFX is different from STMPE as far as the HW is completely different.
> IDD is a new feature.
> TS management is completely different.
> GPIO management looks like but is also rather different.
> ST MFX counts a first level of 8 interrupts (acked by writing in the ACK
> register), then a second level of 24 interrupts for GPIOs. GPIO IRQ can
> be triggered on low level, falling edge, high level, rising edge. GPIO
> IRQ have to be acked by writing in the GPI_ACK register, GPIO can be
> output open-drain with/without internal pull-up, output push-pull, input
> with pull-up/down, input floating or analog.

Okay I am convinced that we need to model this as its own
driver if it is as different as you say.

But let's keep an eye on STMPE's approaches and what it does
right and wrong.

If I rewrote the STMPE driver today, the major changes would be
to use regmap to pass around to subdrivers, and implement a
combined pin control and GPIO driver, so I think those things should
be nice to have for the next iteration.

Yours,
Linus Walleij