diff mbox series

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

Message ID 1523440025-18077-2-git-send-email-amelie.delaunay@st.com
State New
Headers show
Series Introduce STMFX I2C GPIO expander | expand

Commit Message

Amelie DELAUNAY April 11, 2018, 9:47 a.m. UTC 
This patch adds documentation of device tree bindings for the
STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander.

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

Comments

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

s/property/properties/

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

Don't show status in examples.

> +
> +		stmfxgpio: gpio {

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

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

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

> +			pins = "gpio0", "gpio1", "gpio2", "gpio3", "gpio4";
> +			drive-push-pull;
> +			bias-pull-down;
> +		};
> +	};
> +
> +	joystick {
> +		compatible = "gpio-keys";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-0 = <&joystick_pins>;
> +		pinctrl-names = "default";
> +		button@0 {
> +			label = "JoySel";
> +			linux,code = <KEY_ENTER>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <0 IRQ_TYPE_EDGE_RISING>;
> +		};
> +		button@1 {
> +			label = "JoyDown";
> +			linux,code = <KEY_DOWN>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <1 IRQ_TYPE_EDGE_RISING>;
> +		};
> +		button@2 {
> +			label = "JoyLeft";
> +			linux,code = <KEY_LEFT>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <2 IRQ_TYPE_EDGE_RISING>;
> +		};
> +		button@3 {
> +			label = "JoyRight";
> +			linux,code = <KEY_RIGHT>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <3 IRQ_TYPE_EDGE_RISING>;
> +		};
> +		button@4 {
> +			label = "JoyUp";
> +			linux,code = <KEY_UP>;
> +			interrupt-parent = <&stmfxgpio>;
> +			interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +		};
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		orange {
> +			gpios = <&stmfxgpio 17 1>;
> +		};
> +
> +		blue {
> +			gpios = <&stmfxgpio 19 1>;
> +		};
> +	}
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij April 26, 2018, 12:49 p.m. UTC | #2 
On Mon, Apr 16, 2018 at 8:19 PM, Rob Herring <robh@kernel.org> wrote:
> On Wed, Apr 11, 2018 at 11:47:01AM +0200, Amelie Delaunay wrote:
>> This patch adds documentation of device tree bindings for the
>> STMicroelectronics Multi-Function eXpander (STMFX) GPIO expander.
>>
>> Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com>

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

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

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

I'll fix it.

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

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

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

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

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

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

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

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

Thanks,
Amelie

[1] https://lkml.org/lkml/2018/2/12/265
[2] https://lkml.org/lkml/2018/2/21/1503
Linus Walleij May 16, 2018, 2:20 p.m. UTC | #5 
On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:

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

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

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

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

Hi Linus,

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

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

Lee, are you OK with that ?

Regards,
Amelie
Lee Jones May 17, 2018, 6:36 a.m. UTC | #7 
On Wed, 16 May 2018, Amelie DELAUNAY wrote:

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

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

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

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

Thanks,
Amelie
Lee Jones May 18, 2018, 1:52 p.m. UTC | #9 
On Fri, 18 May 2018, Amelie DELAUNAY wrote:

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

It is starting to sound like there will only ever be one child device,
which starts to cross the line into "this is not an MFD" (M = Multi)
territory.
Amelie DELAUNAY May 18, 2018, 3:13 p.m. UTC | #10 
T24gMDUvMTgvMjAxOCAwMzo1MiBQTSwgTGVlIEpvbmVzIHdyb3RlOg0KPiBPbiBGcmksIDE4IE1h
eSAyMDE4LCBBbWVsaWUgREVMQVVOQVkgd3JvdGU6DQo+IA0KPj4gT24gMDUvMTcvMjAxOCAwODoz
NiBBTSwgTGVlIEpvbmVzIHdyb3RlOg0KPj4+IE9uIFdlZCwgMTYgTWF5IDIwMTgsIEFtZWxpZSBE
RUxBVU5BWSB3cm90ZToNCj4+Pg0KPj4+Pg0KPj4+Pg0KPj4+PiBPbiAwNS8xNi8yMDE4IDA0OjIw
IFBNLCBMaW51cyBXYWxsZWlqIHdyb3RlOg0KPj4+Pj4gT24gV2VkLCBNYXkgOSwgMjAxOCBhdCA5
OjU2IEFNLCBBbWVsaWUgREVMQVVOQVkgPGFtZWxpZS5kZWxhdW5heUBzdC5jb20+IHdyb3RlOg0K
Pj4+Pj4NCj4+Pj4+PiBJbmRlZWQsIHN0bWZ4IGhhcyBvdGhlciBmdW5jdGlvbnMgdGhhbiBHUElP
LiBCdXQsIGFmdGVyIGNvbW1lbnRzIGRvbmUNCj4+Pj4+PiBoZXJlOiBbMV0gYW5kIHRoZXJlOiBb
Ml0sIGl0IGhhcyBiZWVuIGRlY2lkZWQgdG8gbW92ZSBNRkQgcGFyZW50L0dQSU8NCj4+Pj4+PiBj
aGlsZCBkcml2ZXJzIGludG8gYSBzaW5nbGUgUElOQ1RSTC9HUElPIGRyaXZlciBiZWNhdXNlIG9m
IHRoZSBmb2xsb3dpbmcNCj4+Pj4+PiByZWFzb25zOg0KPj4+Pj4+IC0gT3RoZXIgc3RtZnggZnVu
Y3Rpb25zIChJREQgbWVhc3VyZW1lbnQgYW5kIFRvdWNoU2NyZWVuIGNvbnRyb2xsZXIpIGFyZQ0K
Pj4+Pj4+IG5vdCB1c2VkIG9uIGFueSBvZiB0aGUgYm9hcmRzIHVzaW5nIGFuIHN0bWZ4IGFuZCBz
dXBwb3J0ZWQgYnkgTGludXgsIHNvDQo+Pj4+Pj4gbm8gd2F5IHRvIHRlc3QgdGhlc2UgZnVuY3Rp
b25zLCBhbmQgbm8gbmVlZCB0byBtYWludGFpbiB0aGVtIHdoaWxlIHRoZXkNCj4+Pj4+PiBhcmUg
bm90IGJlaW5nIHVzZWQuDQo+Pj4+Pj4gLSBCdXQsIGluIHRoZSBjYXNlIGEgbmV3IGJvYXJkIHdp
bGwgdXNlIG1vcmUgdGhhbiBHUElPIGZ1bmN0aW9uIG9uDQo+Pj4+Pj4gc3RtZngsIHRoZSBhY3R1
YWwgaW1wbGVtZW50YXRpb24gYWxsb3cgdG8gZWFzaWx5IGV4dHJhY3QgY29tbW9uIGluaXQNCj4+
Pj4+PiBwYXJ0IG9mIHN0bWZ4IGFuZCBwdXQgaXQgaW4gYW4gTUZEIGRyaXZlci4NCj4+Pj4+Pg0K
Pj4+Pj4+IFNvIEkgY291bGQgcmVtb3ZlIGdwaW8gc3ViLW5vZGUgYW5kIHB1dCBpdHMgY29udGVu
dHMgaW4gc3RtZnggbm9kZSBhbmQNCj4+Pj4+PiBrZWVwIHNpbmdsZSBQSU5DVFJML0dQSU8gZHJp
dmVyIGZvciB0aGUgdGltZSBiZWluZy4NCj4+Pj4+PiBQbGVhc2UgYWR2aXNlLA0KPj4+Pj4NCj4+
Pj4+IEkgd291bGQgbm9ybWFsbHkgYWR2aWNlIHRvIHVzZSB0aGUgcmlnaHQgbW9kZWxpbmcgZnJv
bSB0aGUgc3RhcnQsIGNyZWF0ZQ0KPj4+Pj4gdGhlIE1GRCBkcml2ZXIgYW5kIHNwYXduIHRoZSBk
ZXZpY2VzIGZyb20gdGhlcmUuIEl0IGlzIGNvbmZ1c2luZw0KPj4+Pj4gaWYgdGhlIGxheW91dCBv
ZiB0aGUgZHJpdmVyKHMpIGRvZXNuJ3QgcmVhbGx5IG1hdGNoIHRoZSBsYXlvdXQgb2YgdGhlDQo+
Pj4+PiBoYXJkd2FyZS4NCj4+Pj4+DQo+Pj4+PiBJIHVuZGVyc3RhbmQgdGhhdCBpdCBpcyBhIHBh
aW4gdG8gd3JpdGUgbmV3IE1GRCBkcml2ZXJzIHRvIGdldCB5b3VyDQo+Pj4+PiB0aGluZ3MgZ29p
bmcgYW5kIGl0IHdvdWxkIGJlICJuaWNlIHRvIGdldCB0aGlzIHdvcmtpbmcgcmVhbGx5IHF1aWNr
DQo+Pj4+PiBub3ciIGJ1dCBpbiBteSBleHBlcmllbmNlIGl0IGlzIGJldHRlciB0byBkbyBpdCBy
aWdodCBmcm9tIHRoZSBzdGFydC4NCj4+Pj4+DQo+Pj4+DQo+Pj4+IEhpIExpbnVzLA0KPj4+Pg0K
Pj4+PiBUaGFua3MgZm9yIHlvdXIgYWR2aWNlLiBJIHVuZGVyc3RhbmQgdGhlIHBvaW50Lg0KPj4+
PiBTbywgdGhlIHJpZ2h0IG1vZGVsaW5nIHdvdWxkIGJlIHRvOg0KPj4+PiAtIGNyZWF0ZSBhbiBN
RkQgZHJpdmVyIHdpdGggdGhlIGNvbW1vbiBpbml0IHBhcnQgb2Ygc3RtZngNCj4+Pj4gLSByZW1v
dmUgYWxsIGNvbW1vbiBpbml0IHBhcnQgb2Ygc3RtZngtcGluY3RybCBkcml2ZXIgYW5kIGtlZXAg
b25seSBhbGwNCj4+Pj4gZ3Bpby9waW5jdHJsIGZ1bmN0aW9ucy4NCj4+Pj4NCj4+Pj4gSSB3aWxs
IG5vdCBkZXZlbG9wIHRoZSBvdGhlciBzdG1meCBmdW5jdGlvbnMgKElERCBtZWFzdXJlbWVudCBk
cml2ZXIgYW5kDQo+Pj4+IFRvdWNoU2NyZWVuIGNvbnRyb2xsZXIgZHJpdmVyKSBiZWNhdXNlLCBh
cyBleHBsYWluZWQgZWFsaWVyLCB0aGV5IGFyZQ0KPj4+PiBub3QgdXNlZCBvbiBhbnkgb2YgdGhl
IGJvYXJkcyB1c2luZyBhbiBzdG1meCBhbmQgc3VwcG9ydGVkIGJ5IExpbnV4LCBzbw0KPj4+PiBu
byB3YXkgdG8gdGVzdCB0aGVzZSBmdW5jdGlvbnMsIGFuZCBubyBuZWVkIHRvIG1haW50YWluIHRo
ZW0gd2hpbGUgdGhleQ0KPj4+PiBhcmUgbm90IGJlaW5nIHVzZWQuDQo+Pj4+DQo+Pj4+IExlZSwg
YXJlIHlvdSBPSyB3aXRoIHRoYXQgPw0KPj4+DQo+Pj4gSSBtaXNzZWQgYSBsb3Qgb2YgdGhpcyBj
b252ZXJzYXRpb24gSSB0aGluaywgYnV0IGZyb20gd2hhdCBJJ3ZlIHJlYWQsDQo+Pj4gaXQgc291
bmRzIGZpbmUuDQo+Pj4NCj4+DQo+PiBJIHN1bW1hcml6ZSB0aGUgc2l0dWF0aW9uOg0KPj4gLSBJ
IHN0aWxsIGRvbid0IGhhdmUgYW4gb2ZmaWNpYWwgZGF0YXNoZWV0IGZvciBTVE1GWCBkZXZpY2Ug
d2hpY2ggY291bGQNCj4+IGp1c3RpZnkgdGhlIHVzZSBvZiBhbiBNRkQgZHJpdmVyOw0KPj4gLSB0
aGUgTUZEIGRyaXZlciB3aWxsIGNvbnRhaW4gdGhlIFNUTUZYIGNoaXAgaW5pdGlhbGl6YXRpb24g
c3R1ZmYgc3VjaA0KPj4gYXMgcmVnbWFwIGluaXRpYWxpemF0aW9uIChyZWdtYXAgc3RydWN0dXJl
IHdpbGwgYmUgc2hhcmVkIHdpdGggdGhlDQo+PiBjaGlsZCksIGNoaXAgaW5pdGlhbGl6YXRpb24s
IGdsb2JhbCBpbnRlcnJ1cHQgbWFuYWdlbWVudDsNCj4+IC0gdGhlcmUgd2lsbCBiZSBvbmx5IG9u
ZSBjaGlsZCAoR1BJTy9QSU5DVFJMIG5vZGUpIGZvciB0aGUgdGltZSBiZWluZy4NCj4+DQo+PiBT
bywgaXMgIk1GRCBkcml2ZXIgKyBHUElPL1BJTkNUUkwgZHJpdmVyIiB0aGUgcmlnaHQgbW9kZWxp
bmcsIGFuZCBkb2VzDQo+PiBpdCBzdGlsbCBzb3VuZCBmaW5lIGFmdGVyIHRoaXMgc3VtbWFyeSA/
IDopDQo+IA0KPiBJdCBpcyBzdGFydGluZyB0byBzb3VuZCBsaWtlIHRoZXJlIHdpbGwgb25seSBl
dmVyIGJlIG9uZSBjaGlsZCBkZXZpY2UsDQo+IHdoaWNoIHN0YXJ0cyB0byBjcm9zcyB0aGUgbGlu
ZSBpbnRvICJ0aGlzIGlzIG5vdCBhbiBNRkQiIChNID0gTXVsdGkpDQo+IHRlcnJpdG9yeS4NCj4g
DQoNCi4uLiBmb3IgdGhlIHRpbWUgYmVpbmcuIFNvLCBMaW51cywgTGVlLCBpcyBpdCBwb3NzaWJs
ZSB0byBmaW5kIGNvbW1vbiANCmdyb3VuZCA/
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij May 24, 2018, 7:13 a.m. UTC | #11 
On Fri, May 18, 2018 at 9:29 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
> On 05/17/2018 08:36 AM, Lee Jones wrote:
>> On Wed, 16 May 2018, Amelie DELAUNAY wrote:
>>> On 05/16/2018 04:20 PM, Linus Walleij wrote:
>>>> On Wed, May 9, 2018 at 9:56 AM, Amelie DELAUNAY <amelie.delaunay@st.com> wrote:
>>>>
>>>>> Indeed, stmfx has other functions than GPIO. But, after comments done
>>>>> here: [1] and there: [2], it has been decided to move MFD parent/GPIO
>>>>> child drivers into a single PINCTRL/GPIO driver because of the following
>>>>> reasons:
>>>>> - Other stmfx functions (IDD measurement and TouchScreen controller) are
>>>>> not used on any of the boards using an stmfx and supported by Linux, so
>>>>> no way to test these functions, and no need to maintain them while they
>>>>> are not being used.
>>>>> - But, in the case a new board will use more than GPIO function on
>>>>> stmfx, the actual implementation allow to easily extract common init
>>>>> part of stmfx and put it in an MFD driver.
>>>>>
>>>>> So I could remove gpio sub-node and put its contents in stmfx node and
>>>>> keep single PINCTRL/GPIO driver for the time being.
>>>>> Please advise,
>>>>
>>>> I would normally advice to use the right modeling from the start, create
>>>> the MFD driver and spawn the devices from there. It is confusing
>>>> if the layout of the driver(s) doesn't really match the layout of the
>>>> hardware.
>>>>
>>>> I understand that it is a pain to write new MFD drivers to get your
>>>> things going and it would be "nice to get this working really quick
>>>> now" but in my experience it is better to do it right from the start.
>>>>
>>>
>>> Hi Linus,
>>>
>>> Thanks for your advice. I understand the point.
>>> So, the right modeling would be to:
>>> - create an MFD driver with the common init part of stmfx
>>> - remove all common init part of stmfx-pinctrl driver and keep only all
>>> gpio/pinctrl functions.
>>>
>>> I will not develop the other stmfx functions (IDD measurement driver and
>>> TouchScreen controller driver) because, as explained ealier, they are
>>> not used on any of the boards using an stmfx and supported by Linux, so
>>> no way to test these functions, and no need to maintain them while they
>>> are not being used.
>>>
>>> Lee, are you OK with that ?
>>
>> I missed a lot of this conversation I think, but from what I've read,
>> it sounds fine.
>>
>
> I summarize the situation:
> - I still don't have an official datasheet for STMFX device which could
> justify the use of an MFD driver;
> - the MFD driver will contain the STMFX chip initialization stuff such
> as regmap initialization (regmap structure will be shared with the
> child), chip initialization, global interrupt management;
> - there will be only one child (GPIO/PINCTRL node) for the time being.

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

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

No I think it should use an MFD core.

Mainly because of device tree concerns.

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

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

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amelie DELAUNAY May 28, 2018, 11:39 a.m. UTC | #12 
T24gMDUvMjQvMjAxOCAwOToxMyBBTSwgTGludXMgV2FsbGVpaiB3cm90ZToNCj4gT24gRnJpLCBN
YXkgMTgsIDIwMTggYXQgOToyOSBBTSwgQW1lbGllIERFTEFVTkFZIDxhbWVsaWUuZGVsYXVuYXlA
c3QuY29tPiB3cm90ZToNCj4+IE9uIDA1LzE3LzIwMTggMDg6MzYgQU0sIExlZSBKb25lcyB3cm90
ZToNCj4+PiBPbiBXZWQsIDE2IE1heSAyMDE4LCBBbWVsaWUgREVMQVVOQVkgd3JvdGU6DQo+Pj4+
IE9uIDA1LzE2LzIwMTggMDQ6MjAgUE0sIExpbnVzIFdhbGxlaWogd3JvdGU6DQo+Pj4+PiBPbiBX
ZWQsIE1heSA5LCAyMDE4IGF0IDk6NTYgQU0sIEFtZWxpZSBERUxBVU5BWSA8YW1lbGllLmRlbGF1
bmF5QHN0LmNvbT4gd3JvdGU6DQo+Pj4+Pg0KPj4+Pj4+IEluZGVlZCwgc3RtZnggaGFzIG90aGVy
IGZ1bmN0aW9ucyB0aGFuIEdQSU8uIEJ1dCwgYWZ0ZXIgY29tbWVudHMgZG9uZQ0KPj4+Pj4+IGhl
cmU6IFsxXSBhbmQgdGhlcmU6IFsyXSwgaXQgaGFzIGJlZW4gZGVjaWRlZCB0byBtb3ZlIE1GRCBw
YXJlbnQvR1BJTw0KPj4+Pj4+IGNoaWxkIGRyaXZlcnMgaW50byBhIHNpbmdsZSBQSU5DVFJML0dQ
SU8gZHJpdmVyIGJlY2F1c2Ugb2YgdGhlIGZvbGxvd2luZw0KPj4+Pj4+IHJlYXNvbnM6DQo+Pj4+
Pj4gLSBPdGhlciBzdG1meCBmdW5jdGlvbnMgKElERCBtZWFzdXJlbWVudCBhbmQgVG91Y2hTY3Jl
ZW4gY29udHJvbGxlcikgYXJlDQo+Pj4+Pj4gbm90IHVzZWQgb24gYW55IG9mIHRoZSBib2FyZHMg
dXNpbmcgYW4gc3RtZnggYW5kIHN1cHBvcnRlZCBieSBMaW51eCwgc28NCj4+Pj4+PiBubyB3YXkg
dG8gdGVzdCB0aGVzZSBmdW5jdGlvbnMsIGFuZCBubyBuZWVkIHRvIG1haW50YWluIHRoZW0gd2hp
bGUgdGhleQ0KPj4+Pj4+IGFyZSBub3QgYmVpbmcgdXNlZC4NCj4+Pj4+PiAtIEJ1dCwgaW4gdGhl
IGNhc2UgYSBuZXcgYm9hcmQgd2lsbCB1c2UgbW9yZSB0aGFuIEdQSU8gZnVuY3Rpb24gb24NCj4+
Pj4+PiBzdG1meCwgdGhlIGFjdHVhbCBpbXBsZW1lbnRhdGlvbiBhbGxvdyB0byBlYXNpbHkgZXh0
cmFjdCBjb21tb24gaW5pdA0KPj4+Pj4+IHBhcnQgb2Ygc3RtZnggYW5kIHB1dCBpdCBpbiBhbiBN
RkQgZHJpdmVyLg0KPj4+Pj4+DQo+Pj4+Pj4gU28gSSBjb3VsZCByZW1vdmUgZ3BpbyBzdWItbm9k
ZSBhbmQgcHV0IGl0cyBjb250ZW50cyBpbiBzdG1meCBub2RlIGFuZA0KPj4+Pj4+IGtlZXAgc2lu
Z2xlIFBJTkNUUkwvR1BJTyBkcml2ZXIgZm9yIHRoZSB0aW1lIGJlaW5nLg0KPj4+Pj4+IFBsZWFz
ZSBhZHZpc2UsDQo+Pj4+Pg0KPj4+Pj4gSSB3b3VsZCBub3JtYWxseSBhZHZpY2UgdG8gdXNlIHRo
ZSByaWdodCBtb2RlbGluZyBmcm9tIHRoZSBzdGFydCwgY3JlYXRlDQo+Pj4+PiB0aGUgTUZEIGRy
aXZlciBhbmQgc3Bhd24gdGhlIGRldmljZXMgZnJvbSB0aGVyZS4gSXQgaXMgY29uZnVzaW5nDQo+
Pj4+PiBpZiB0aGUgbGF5b3V0IG9mIHRoZSBkcml2ZXIocykgZG9lc24ndCByZWFsbHkgbWF0Y2gg
dGhlIGxheW91dCBvZiB0aGUNCj4+Pj4+IGhhcmR3YXJlLg0KPj4+Pj4NCj4+Pj4+IEkgdW5kZXJz
dGFuZCB0aGF0IGl0IGlzIGEgcGFpbiB0byB3cml0ZSBuZXcgTUZEIGRyaXZlcnMgdG8gZ2V0IHlv
dXINCj4+Pj4+IHRoaW5ncyBnb2luZyBhbmQgaXQgd291bGQgYmUgIm5pY2UgdG8gZ2V0IHRoaXMg
d29ya2luZyByZWFsbHkgcXVpY2sNCj4+Pj4+IG5vdyIgYnV0IGluIG15IGV4cGVyaWVuY2UgaXQg
aXMgYmV0dGVyIHRvIGRvIGl0IHJpZ2h0IGZyb20gdGhlIHN0YXJ0Lg0KPj4+Pj4NCj4+Pj4NCj4+
Pj4gSGkgTGludXMsDQo+Pj4+DQo+Pj4+IFRoYW5rcyBmb3IgeW91ciBhZHZpY2UuIEkgdW5kZXJz
dGFuZCB0aGUgcG9pbnQuDQo+Pj4+IFNvLCB0aGUgcmlnaHQgbW9kZWxpbmcgd291bGQgYmUgdG86
DQo+Pj4+IC0gY3JlYXRlIGFuIE1GRCBkcml2ZXIgd2l0aCB0aGUgY29tbW9uIGluaXQgcGFydCBv
ZiBzdG1meA0KPj4+PiAtIHJlbW92ZSBhbGwgY29tbW9uIGluaXQgcGFydCBvZiBzdG1meC1waW5j
dHJsIGRyaXZlciBhbmQga2VlcCBvbmx5IGFsbA0KPj4+PiBncGlvL3BpbmN0cmwgZnVuY3Rpb25z
Lg0KPj4+Pg0KPj4+PiBJIHdpbGwgbm90IGRldmVsb3AgdGhlIG90aGVyIHN0bWZ4IGZ1bmN0aW9u
cyAoSUREIG1lYXN1cmVtZW50IGRyaXZlciBhbmQNCj4+Pj4gVG91Y2hTY3JlZW4gY29udHJvbGxl
ciBkcml2ZXIpIGJlY2F1c2UsIGFzIGV4cGxhaW5lZCBlYWxpZXIsIHRoZXkgYXJlDQo+Pj4+IG5v
dCB1c2VkIG9uIGFueSBvZiB0aGUgYm9hcmRzIHVzaW5nIGFuIHN0bWZ4IGFuZCBzdXBwb3J0ZWQg
YnkgTGludXgsIHNvDQo+Pj4+IG5vIHdheSB0byB0ZXN0IHRoZXNlIGZ1bmN0aW9ucywgYW5kIG5v
IG5lZWQgdG8gbWFpbnRhaW4gdGhlbSB3aGlsZSB0aGV5DQo+Pj4+IGFyZSBub3QgYmVpbmcgdXNl
ZC4NCj4+Pj4NCj4+Pj4gTGVlLCBhcmUgeW91IE9LIHdpdGggdGhhdCA/DQo+Pj4NCj4+PiBJIG1p
c3NlZCBhIGxvdCBvZiB0aGlzIGNvbnZlcnNhdGlvbiBJIHRoaW5rLCBidXQgZnJvbSB3aGF0IEkn
dmUgcmVhZCwNCj4+PiBpdCBzb3VuZHMgZmluZS4NCj4+Pg0KPj4NCj4+IEkgc3VtbWFyaXplIHRo
ZSBzaXR1YXRpb246DQo+PiAtIEkgc3RpbGwgZG9uJ3QgaGF2ZSBhbiBvZmZpY2lhbCBkYXRhc2hl
ZXQgZm9yIFNUTUZYIGRldmljZSB3aGljaCBjb3VsZA0KPj4ganVzdGlmeSB0aGUgdXNlIG9mIGFu
IE1GRCBkcml2ZXI7DQo+PiAtIHRoZSBNRkQgZHJpdmVyIHdpbGwgY29udGFpbiB0aGUgU1RNRlgg
Y2hpcCBpbml0aWFsaXphdGlvbiBzdHVmZiBzdWNoDQo+PiBhcyByZWdtYXAgaW5pdGlhbGl6YXRp
b24gKHJlZ21hcCBzdHJ1Y3R1cmUgd2lsbCBiZSBzaGFyZWQgd2l0aCB0aGUNCj4+IGNoaWxkKSwg
Y2hpcCBpbml0aWFsaXphdGlvbiwgZ2xvYmFsIGludGVycnVwdCBtYW5hZ2VtZW50Ow0KPj4gLSB0
aGVyZSB3aWxsIGJlIG9ubHkgb25lIGNoaWxkIChHUElPL1BJTkNUUkwgbm9kZSkgZm9yIHRoZSB0
aW1lIGJlaW5nLg0KPiANCj4gQnV0IHRoZXJlIHdpbGwgYmUgbW9yZSBkZXZpY2VzIGluIGl0LiBB
bmQgdGhleSB3aWxsIGludmFyaWFibHkgYmUgcHV0DQo+IHRvIHVzZSBsYXRlciwgYW5kIHRoZXJl
IHdpbGwgYmUgbmV3IHZlcnNpb25zIG9mIHRoZSBjaGlwIGFzIHdlbGwsIGFuZA0KPiB0aGVuIHlv
dSB3aWxsIGJlIGhhcHB5IGFib3V0IGRvaW5nIHRoZSBNRkQgY29yZSwgd2hpY2ggbWFrZXMgaXQN
Cj4gZWFzeSB0byBhZGQgbmV3IHZhcmlhbnRzIHdpdGggZGlmZmVyZW50IHN1YmRldmljZXMuDQo+
IA0KPj4gU28sIGlzICJNRkQgZHJpdmVyICsgR1BJTy9QSU5DVFJMIGRyaXZlciIgdGhlIHJpZ2h0
IG1vZGVsaW5nLCBhbmQgZG9lcw0KPj4gaXQgc3RpbGwgc291bmQgZmluZSBhZnRlciB0aGlzIHN1
bW1hcnkgPyA6KQ0KPiANCj4gTm8gSSB0aGluayBpdCBzaG91bGQgdXNlIGFuIE1GRCBjb3JlLg0K
PiANCj4gTWFpbmx5IGJlY2F1c2Ugb2YgZGV2aWNlIHRyZWUgY29uY2VybnMuDQo+IA0KPiBUaGUg
bWFpbiByZWFzb24gaXMgdGhhdCB0aGUgZGV2aWNlIHRyZWUgYmluZGluZ3Mgd2lsbCBiZSBkaWZm
ZXJlbnQgaWYNCj4geW91IGFkZCBhbiBNRkQgY29yZSBsYXRlciwgdGhlIEdQSU8gYW5kIHBpbmN0
cmwgZHJpdmVyIHdpbGwNCj4gbW92ZSB0byBhIGNoaWxkIG5vZGUsIG1ha2luZyBvbGQgZGV2aWNl
IHRyZWVzIGluY29tcGF0aWJsZS4NCj4gDQo+IFdlIGNvdWxkIGhhdmUgYSBzaW5nbGUgZHJpdmVy
IGluIEdQSU8rcGluIGNvbnRyb2wgaWYgaXQgaXMgYSBjaGlsZA0KPiBvZiBhbiBNRkQgbm9kZSBp
biB0aGUgZGV2aWNlIHRyZWUsIGJ1dCBpdCBkb2Vzbid0IG1ha2UgbXVjaA0KPiBzZW5zZSBhcyB0
aGUgSTJDIGRldmljZSBuZWVkIHRvIGJlIHByb2JpbmcgdG8gdGhlIE1GRCBjb3JlLg0KPiANCg0K
SSBhZ3JlZSB3aXRoIHlvdSBMaW51cywgYW5kIHRoYXQncyB3aHkgYWxsIFNUTUZYIGNoaXAgaW5p
dGlhbGl6YXRpb24gDQpzdHVmZiB3YXMgZGVjb3JyZWxhdGVkIGluIHBpbmN0cmwtc3RtZnguIFRo
aXMgc2hvd3MgdGhhdCB0aGlzIHN0dWZmIA0KbmVlZHMgdG8gYmUgaW4gYW4gTUZEIGNvcmUuDQoN
CkJ1dCBhcyB0aGVyZSBpcyBvbmx5IG9uZSBjaGlsZCBmb3Igbm93IChkdWUgdG8gdGhlIHJlYXNv
bnMgbWVudGlvbmVkIA0KZWFybGllciksIGl0IGNhbiBzdWdnZXN0IHRoYXQgaXQgaXMgbm90IGEg
TXVsdGktRnVuY3Rpb24gRGV2aWNlLg0KDQpJJ20gbm90IGFibGUgdG8gdGFyZ2V0IHdoZW4gSURE
IG9yIFRTIGZ1bmN0aW9ucyB3aWxsIGJlIHJlcXVpcmVkIG9uIGEgDQpMaW51eCBwcm9kdWN0LCBi
dXQgaXQgc3RpbGwgbWFrZXMgc2Vuc2UgdG8gY29uc2lkZXIgdGhhdCB0aGVzZSBmdW5jdGlvbnMg
DQp3aWxsIGJlIHVzZWQgb24gYSBMaW51eCBwcm9kdWN0Lg0KDQpTbywgSSB0aGluayBNRkQgY29y
ZSArIEdQSU8vcGluY3RybCBkcml2ZXIgaXMgdGhlIHJpZ2h0IG1vZGVsaW5nLCBidXQgSSANCndh
bnRlZCB0byBiZSBzdXJlIHRoYXQgdGhpcyBpcyBva2F5IGZvciBldmVyeW9uZS4gSSBkb24ndCB3
YW50IHRvIHNwZW5kIA0KdGltZSBvbiBzb21ldGhpbmcgdGhhdCB3aWxsIG5vdCBiZSBhY2NlcHRl
ZCBkdWUgdG8gaXRzIG1vZGVsaW5nLg0KDQpSZWdhcmRzLA0KQW1lbGll
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones May 29, 2018, 7:36 a.m. UTC | #13 
On Mon, 28 May 2018, Amelie DELAUNAY wrote:

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

It's fine.  Go ahead.

Thanks for seeing this through to a reasonable conclusion.
diff mbox series

Patch

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