[PATCH] bsps/arm/stm32f4 Optimize get pin and change from HAL to LL

[Gedare Bloom]

Hi Duc,

As I mentioned in the other patch, please save and consolidate all
patch revisions and send an entire new patch set of -v2 patches with
all your changes. You can also squash changes into your patch set, for
example to address reviewer comments like these. We would like to have
a clean set of patches to apply to RTEMS, which may not reflect how
you arrived at the end result of your code. I will try to discuss more
in today's discord meeting.

Gedare

On Wed, Jul 6, 2022 at 5:09 AM Duc Doan <dtbpkmte at gmail.com> wrote:
>
> Hello Cedric,
>
> Thank you for your feedback. I agree with you that there are places that could be optimized out. Here is a new patch for that.
>
> Best,
>
> Duc Doan
>
> ---
>  bsps/arm/stm32f4/gpio/gpio.c | 87 ++++++++++++------------------------
>  1 file changed, 29 insertions(+), 58 deletions(-)
>
> diff --git a/bsps/arm/stm32f4/gpio/gpio.c b/bsps/arm/stm32f4/gpio/gpio.c
> index e971f91140..b632236d8d 100644
> --- a/bsps/arm/stm32f4/gpio/gpio.c
> +++ b/bsps/arm/stm32f4/gpio/gpio.c
> @@ -22,9 +22,18 @@
>  #include <stdlib.h>
>
>  /*********** Helpers *****************/
> -static stm32f4_gpio *get_gpio_from_base(
> -    rtems_gpio *base
> -);
> +/**
> +  * @brief Macro to get stm32f4_gpio object from a base rtems_gpio
> +  *        object.
> +  *
> +  * This is a wrapper of RTEMS_CONTAINER_OF macro
> +  *
> +  * @param base The pointer to a rtems_gpio object
> +  * @retval The pointer to the stm32f4_gpio object owning
> +  *         the specified rtems_gpio object
> +  */
> +#define get_gpio_from_base(base) \
> +    RTEMS_CONTAINER_OF(base, stm32f4_gpio, base)
>
>  /*********** GPIO API ***************/
>  static rtems_status_code stm32f4_gpio_get(
> @@ -115,44 +124,6 @@ static GPIO_TypeDef *GPIOx[] = {
>  #endif /* STM32F429X */
>  };
>
> -static uint16_t GPIO_PIN_x[] = {
> -    GPIO_PIN_0,
> -    GPIO_PIN_1,
> -    GPIO_PIN_2,
> -    GPIO_PIN_3,
> -    GPIO_PIN_4,
> -    GPIO_PIN_5,
> -    GPIO_PIN_6,
> -    GPIO_PIN_7,
> -    GPIO_PIN_8,
> -    GPIO_PIN_9,
> -    GPIO_PIN_10,
> -    GPIO_PIN_11,
> -    GPIO_PIN_12,
> -    GPIO_PIN_13,
> -    GPIO_PIN_14,
> -    GPIO_PIN_15
> -};
> -
> -static uint32_t LL_EXTI_LINE_x[] = {
> -    LL_EXTI_LINE_0,
> -    LL_EXTI_LINE_1,
> -    LL_EXTI_LINE_2,
> -    LL_EXTI_LINE_3,
> -    LL_EXTI_LINE_4,
> -    LL_EXTI_LINE_5,
> -    LL_EXTI_LINE_6,
> -    LL_EXTI_LINE_7,
> -    LL_EXTI_LINE_8,
> -    LL_EXTI_LINE_9,
> -    LL_EXTI_LINE_10,
> -    LL_EXTI_LINE_11,
> -    LL_EXTI_LINE_12,
> -    LL_EXTI_LINE_13,
> -    LL_EXTI_LINE_14,
> -    LL_EXTI_LINE_15
> -};
> -
>  static unsigned int EXTIx_IRQn[] = {
>      EXTI0_IRQn,
>      EXTI1_IRQn,
> @@ -200,13 +171,13 @@ static unsigned int EXTIx_IRQn[] = {
>    * @brief Converts pin number from 0-15 to HAL pin mask.
>    * @param pin is the pin number from 0-15
>    */
> -#define STM32F4_GET_HAL_GPIO_PIN(pin) (GPIO_PIN_x[( pin )])
> +#define STM32F4_GET_HAL_GPIO_PIN(pin) ((uint16_t) (1 << ( pin )))
>
>  /**
>    * @brief Get EXTI Line from pin number 0-15
>    * @param pin is the pin number from 0-15
>    */
> -#define STM32F4_GET_LL_EXTI_LINE(pin) (LL_EXTI_LINE_x[( pin )])
> +#define STM32F4_GET_LL_EXTI_LINE(pin) (0x1UL << ( pin ))
>
>  /**
>    * @brief Get EXTI IRQ number from pin 0-15
> @@ -229,14 +200,6 @@ static stm32f4_interrupt isr_table[16];
>
>  void exti_handler(void *arg);
>
> -/************* Helpers implementation ********************/
> -
> -static stm32f4_gpio *get_gpio_from_base(
> -    rtems_gpio *base
> -)
> -{
> -    return RTEMS_CONTAINER_OF(base, stm32f4_gpio, base);
> -}
>
>  /********** STM32F4 GPIO API functions ************/
>
> @@ -523,7 +486,8 @@ rtems_status_code stm32f4_gpio_remove_interrupt(
>      rtems_status_code sc = rtems_interrupt_handler_remove(
>              STM32F4_GET_EXTI_IRQn(gpio->pin),
>              exti_handler,
> -            &isr_table[gpio->pin].arg);
> +            &isr_table[gpio->pin].arg
> +    );
>      if (sc == RTEMS_SUCCESSFUL) {
>          isr_table[gpio->pin] = (stm32f4_interrupt){0};
>      }
> @@ -554,9 +518,12 @@ rtems_status_code stm32f4_gpio_write(
>  )
>  {
>      stm32f4_gpio *gpio = get_gpio_from_base(base);
> -    uint32_t pin_mask = STM32F4_GET_HAL_GPIO_PIN(gpio->pin);
>
> -    HAL_GPIO_WritePin(gpio->port, pin_mask, value);
> +    if (value)
> +        LL_GPIO_SetOutputPin(gpio->port, STM32F4_GET_HAL_GPIO_PIN(gpio->pin));
> +    else
> +        LL_GPIO_ResetOutputPin(gpio->port, STM32F4_GET_HAL_GPIO_PIN(gpio->pin));
> +
>      return RTEMS_SUCCESSFUL;
>  }
>
> @@ -566,9 +533,11 @@ rtems_status_code stm32f4_gpio_read(
>  )
>  {
>      stm32f4_gpio *gpio = get_gpio_from_base(base);
> -    uint32_t pin_mask = STM32F4_GET_HAL_GPIO_PIN(gpio->pin);
>
> -    *value = HAL_GPIO_ReadPin(gpio->port, pin_mask);
> +    *value = LL_GPIO_IsInputPinSet(
> +            gpio->port,
> +            STM32F4_GET_HAL_GPIO_PIN(gpio->pin)
> +    );
>      return RTEMS_SUCCESSFUL;
>  }
>
> @@ -577,9 +546,11 @@ rtems_status_code stm32f4_gpio_toggle(
>  )
>  {
>      stm32f4_gpio *gpio = get_gpio_from_base(base);
> -    uint32_t pin_mask = STM32F4_GET_HAL_GPIO_PIN(gpio->pin);
>
> -    HAL_GPIO_TogglePin(gpio->port, pin_mask);
> +    LL_GPIO_TogglePin(
> +            gpio->port,
> +            STM32F4_GET_HAL_GPIO_PIN(gpio->pin)
> +    );
>      return RTEMS_SUCCESSFUL;
>  }
>
> --
> 2.36.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel