[PATCH v3] ARM: dts: stm32: change pinctrl bindings definition

[PATCH v3] ARM: dts: stm32: change pinctrl bindings definition

[Alexandre Torgue]

Initially each pin was declared in "include/dt-bindings/stm32<SOC>-pinfunc.h"
and each definition contained SOC names (ex: STM32F429_PA9_FUNC_USART1_TX).
Since this approach was approved, the number of supported MCU has
increased (STM32F429/STM32F469/STM32f746/STM32H743). To avoid to add a new
file in "include/dt-bindings" each time a new STM32 SOC arrives I propose
a new approach which consist to use a macro to define pin muxing in device
tree. All STM32 DT will use the common macro to define pinmux. Furthermore, it
will make maintenance and integration of new SOC easier.

I made only one patch to avoid dependencies. Let me know if you prefer that I
split it.

To check that patch generate same dtb than befrore I used dtc binary to (re)generate dts files
(before and after apply the series).

Example on stm32f469-disco:

./scripts/dtc/dtc -I dtb -O dts -o stm32f469-disco-after.dts
stm32f469-disco-after.dtb

./scripts/dtc/dtc -I dtb -O dts -o stm32f469-disco-before.dts
stm32f469-disco-before.dtb

Then diff -u stm32f469-disco-before.dts stm32f469-disco-after.dts

Changes since v2:
 -Drop RFC tag
 -Add reviewers
 -Enhance modifications to all STM32 (F4/F7/H7)

Changes since v1:
 -According to Vikas Manocha review, use generic macro to define port.


Regards
Alex


Alexandre Torgue (1):
  ARM: dts: stm32: change pinctrl bindings definition

 .../bindings/pinctrl/st,stm32-pinctrl.txt          |   37 +-
 arch/arm/boot/dts/stm32f429.dtsi                   |  176 +--
 arch/arm/boot/dts/stm32f746.dtsi                   |   10 +-
 arch/arm/boot/dts/stm32h743-pinctrl.dtsi           |   10 +-
 include/dt-bindings/pinctrl/stm32-pinfunc.h        |   31 +
 include/dt-bindings/pinctrl/stm32f429-pinfunc.h    | 1239 ---------------
 include/dt-bindings/pinctrl/stm32f746-pinfunc.h    | 1324 ----------------
 include/dt-bindings/pinctrl/stm32h7-pinfunc.h      | 1612 --------------------
 8 files changed, 150 insertions(+), 4289 deletions(-)
 create mode 100644 include/dt-bindings/pinctrl/stm32-pinfunc.h
 delete mode 100644 include/dt-bindings/pinctrl/stm32f429-pinfunc.h
 delete mode 100644 include/dt-bindings/pinctrl/stm32f746-pinfunc.h
 delete mode 100644 include/dt-bindings/pinctrl/stm32h7-pinfunc.h

-- 
2.7.4

[PATCH v3] ARM: dts: stm32: change pinctrl bindings definition

[Rob Herring]

On Thu, Jul 27, 2017 at 03:49:53PM +0200, Alexandre Torgue wrote:
> Initially each pin was declared in "include/dt-bindings/stm32<SOC>-pinfunc.h"
> and each definition contained SOC names (ex: STM32F429_PA9_FUNC_USART1_TX).
> Since this approach was approved, the number of supported MCU has
> increased (STM32F429/STM32F469/STM32f746/STM32H743). To avoid to add a new
> file in "include/dt-bindings" each time a new STM32 SOC arrives I propose
> a new approach which consist to use a macro to define pin muxing in device
> tree. All STM32 will use the common macro to define pinmux. Furthermore, it
> will make STM32 maintenance and integration of new SOC easier .
> 
> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>
> Reviewed-by: Vikas MANOCHA <vikas.manocha@st.com>
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt
> index d907a74..567aa72 100644
> --- a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt
> @@ -126,22 +126,27 @@ configuration, pullups, drive, output high/low and output speed.
>      };
>  
>  Required properties:
> -- pinmux: integer array, represents gpio pin number and mux setting.
> -  Supported pin number and mux varies for different SoCs, and are defined in
> -  dt-bindings/pinctrl/<soc>-pinfunc.h directly.
> -  These defines are calculated as:
> -    ((port * 16 + line) << 8) | function
> +- pinmux: integer array, represents gpio pin number and mux setting. Use
> +  following macro: STM32_PINMUX(PIN_NO(port_name, line), mode) to declare it.

I would keep the above formula. It can't change because that is the ABI. 
The macro is just convenience.

Is there any reason to have 2 macros? I'd just do STM32_PINMUX(port, 
line, mode) and make port names defines (PA, PB, PC, etc.).

> +
>    With:
> -    - port: The gpio port index (PA = 0, PB = 1, ..., PK = 11)
> -    - line: The line offset within the port (PA0 = 0, PA1 = 1, ..., PA15 = 15)
> -    - function: The function number, can be:
> -      * 0 : GPIO
> -      * 1 : Alternate Function 0
> -      * 2 : Alternate Function 1
> -      * 3 : Alternate Function 2
> +    - port_name: The gpio port name ('A', 'B', ..., 'K')
> +    - line: The line offset within the port (0, 1, ..., 15)
> +    - mode: The mode can be:
> +      * GPIO
> +      * AF0 : Alternate Function 0
> +      * AF1 : Alternate Function 1
> +      * AF2 : Alternate Function 2
>        * ...
> -      * 16 : Alternate Function 15
> -      * 17 : Analog
> +      * AF15 : Alternate Function 15
> +      * ANALOG

Here too, keeping the numbers is important.

Perhaps the macro description should either be its own additional 
section or document it inline with the macro definition.

Overall, it does seem like a nice shrinking of the header files.

> +
> +  Example:
> +    To declare pin PA7 in mode "alternate function 7" you have to
> +    declare:
> +      pinmux = <STM32_PINMUX(PIN_NO('A', 9), AF7)>;
> +
> +  This macro is defined in dt-bindings/pinctrl/stm32-pinfunc.h
>  
>  Optional properties:
>  - GENERIC_PINCONFIG: is the generic pinconfig options to use.
> @@ -165,13 +170,13 @@ pin-controller {
>  ...
>  	usart1_pins_a: usart1 at 0 {
>  		pins1 {
> -			pinmux = <STM32F429_PA9_FUNC_USART1_TX>;
> +			pinmux = <STM32_PINMUX(PIN_NO('A', 9), AF7)>; /* USART1_TX */
>  			bias-disable;
>  			drive-push-pull;
>  			slew-rate = <0>;
>  		};
>  		pins2 {
> -			pinmux = <STM32F429_PA10_FUNC_USART1_RX>;
> +			pinmux = <STM32_PINMUX(PIN_NO('A', 10), AF7)>; /* USART1_RX */
>  			bias-disable;
>  		};
>  	};

[PATCH v3] ARM: dts: stm32: change pinctrl bindings definition

[Alexandre Torgue]

Hi Rob,

On 08/03/2017 10:21 PM, Rob Herring wrote:
> On Thu, Jul 27, 2017 at 03:49:53PM +0200, Alexandre Torgue wrote:
>> Initially each pin was declared in "include/dt-bindings/stm32<SOC>-pinfunc.h"
>> and each definition contained SOC names (ex: STM32F429_PA9_FUNC_USART1_TX).
>> Since this approach was approved, the number of supported MCU has
>> increased (STM32F429/STM32F469/STM32f746/STM32H743). To avoid to add a new
>> file in "include/dt-bindings" each time a new STM32 SOC arrives I propose
>> a new approach which consist to use a macro to define pin muxing in device
>> tree. All STM32 will use the common macro to define pinmux. Furthermore, it
>> will make STM32 maintenance and integration of new SOC easier .
>>
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>
>> Reviewed-by: Vikas MANOCHA <vikas.manocha@st.com>
>> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt
>> index d907a74..567aa72 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.txt
>> @@ -126,22 +126,27 @@ configuration, pullups, drive, output high/low and output speed.
>>       };
>>   
>>   Required properties:
>> -- pinmux: integer array, represents gpio pin number and mux setting.
>> -  Supported pin number and mux varies for different SoCs, and are defined in
>> -  dt-bindings/pinctrl/<soc>-pinfunc.h directly.
>> -  These defines are calculated as:
>> -    ((port * 16 + line) << 8) | function
>> +- pinmux: integer array, represents gpio pin number and mux setting. Use
>> +  following macro: STM32_PINMUX(PIN_NO(port_name, line), mode) to declare it.
> 
> I would keep the above formula. It can't change because that is the ABI.
> The macro is just convenience.
> 
> Is there any reason to have 2 macros? I'd just do STM32_PINMUX(port,
> line, mode) and make port names defines (PA, PB, PC, etc.).

No. It has been done in this way to simplify macro definition, but it 
could be done in another way. I can try to create a macro like you 
propose: STM32_PINMUX(port, line, mode) if you think it is more readable.

> 
>> +
>>     With:
>> -    - port: The gpio port index (PA = 0, PB = 1, ..., PK = 11)
>> -    - line: The line offset within the port (PA0 = 0, PA1 = 1, ..., PA15 = 15)
>> -    - function: The function number, can be:
>> -      * 0 : GPIO
>> -      * 1 : Alternate Function 0
>> -      * 2 : Alternate Function 1
>> -      * 3 : Alternate Function 2
>> +    - port_name: The gpio port name ('A', 'B', ..., 'K')
>> +    - line: The line offset within the port (0, 1, ..., 15)
>> +    - mode: The mode can be:
>> +      * GPIO
>> +      * AF0 : Alternate Function 0
>> +      * AF1 : Alternate Function 1
>> +      * AF2 : Alternate Function 2
>>         * ...
>> -      * 16 : Alternate Function 15
>> -      * 17 : Analog
>> +      * AF15 : Alternate Function 15
>> +      * ANALOG
> 
> Here too, keeping the numbers is important.
> 
> Perhaps the macro description should either be its own additional
> section or document it inline with the macro definition.

I agree. I can keep previous definition like it was done, and add a 
section only for the new macro.

Thanks for your review.

Alex


> 
> Overall, it does seem like a nice shrinking of the header files.
> 
>> +
>> +  Example:
>> +    To declare pin PA7 in mode "alternate function 7" you have to
>> +    declare:
>> +      pinmux = <STM32_PINMUX(PIN_NO('A', 9), AF7)>;
>> +
>> +  This macro is defined in dt-bindings/pinctrl/stm32-pinfunc.h
>>   
>>   Optional properties:
>>   - GENERIC_PINCONFIG: is the generic pinconfig options to use.
>> @@ -165,13 +170,13 @@ pin-controller {
>>   ...
>>   	usart1_pins_a: usart1 at 0 {
>>   		pins1 {
>> -			pinmux = <STM32F429_PA9_FUNC_USART1_TX>;
>> +			pinmux = <STM32_PINMUX(PIN_NO('A', 9), AF7)>; /* USART1_TX */
>>   			bias-disable;
>>   			drive-push-pull;
>>   			slew-rate = <0>;
>>   		};
>>   		pins2 {
>> -			pinmux = <STM32F429_PA10_FUNC_USART1_RX>;
>> +			pinmux = <STM32_PINMUX(PIN_NO('A', 10), AF7)>; /* USART1_RX */
>>   			bias-disable;
>>   		};
>>   	};