Re: [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@xxxxxx>
> > Reviewed-by: Vikas MANOCHA <vikas.manocha@xxxxxx>
> > Reviewed-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
> >
> > 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@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;
> >   		};
> >   	};