Messages in this thread | | | Subject | Re: [PATCH v3] ARM: dts: stm32: change pinctrl bindings definition | From | Alexandre Torgue <> | Date | Mon, 28 Aug 2017 17:26:01 +0200 |
| |
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@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; >> }; >> };
| |