linux-kernel - Re: [PATCH v3] ARM: dts: stm32: change pinctrl bindings definition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b8894885-a96e-f4e9-5513-a16d04ff3600@st.com>
Date:   Mon, 28 Aug 2017 17:26:01 +0200
From:   Alexandre Torgue <alexandre.torgue@...com>
To:     Rob Herring <robh@...nel.org>
CC:     Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Russell King <linux@...linux.org.uk>,
        <linux-arm-kernel@...ts.infradead.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <vikas.manocha@...com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>
Subject: Re: [PATCH v3] ARM: dts: stm32: change pinctrl bindings definition

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@...com>
>> Reviewed-by: Vikas MANOCHA <vikas.manocha@...com>
>> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@...aro.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;
>>   		};
>>   	};