3
\$\begingroup\$

I'm new to bare-metal programming. I'm currently developing some drivers for my first project.

This is a simple GPIO driver. I tested it and it's working fine.

What do you guys think of it, and how could I improve this?

GPIO.h:

#pragma once
#include "stm32f4xx.h"

#define BIT(x) (1UL << (x))
#define PIN(bank, num) ((((bank) - 'A') << 8) | (num))
#define PINNO(pin) (pin & 255)
#define PINBANK(pin) (pin >> 8)

#define GPIO_BASE GPIOA_BASE

typedef enum { DISABLE_CLK, ENABLE_CLK} GPIO_clock;
typedef enum { INPUT, OUTPUT, ALT_FUN, ANALOG } GPIO_mode;
typedef enum { PUSHPULL, OPENDRAIN } GPIO_outputType;
typedef enum { LOWSPEED, MEDIUMSPEED, HIGHSPEED, VERYHIGHSPEED } GPIO_speed;
typedef enum { PULLUP, PULLDOWN } GPIO_Pull;
typedef enum { LOW, HIGH } GPIO_pinState;

typedef uint16_t pin_t;

void GPIO_setClock(pin_t pin, GPIO_clock EnOrDis);

void GPIO_setMode(pin_t pin, GPIO_mode mode);
void GPIO_setOutputType(pin_t pin, GPIO_outputType type);
void GPIO_setSpeed(pin_t pin, GPIO_speed speed);
void GPIO_setPull(pin_t pin, GPIO_Pull UpOrDown);
GPIO_pinState GPIO_read(pin_t pin);
void GPIO_write(pin_t pin, GPIO_pinState state);
void GPIO_toggle(pin_t pin);

gpio.c:

#include "gpio.h"

void GPIO_setClock(pin_t GPIO_pin, GPIO_clock EnOrDi) {
  if (EnOrDi) {
    RCC->AHB1ENR |= BIT(PINBANK(GPIO_pin));
  } else {
    RCC->AHB1ENR &= ~BIT(PINBANK(GPIO_pin));
  }
}

void GPIO_setMode(pin_t pin, GPIO_mode mode) {
  GPIO_TypeDef *GPIOx = (GPIO_TypeDef *)(GPIOA_BASE + (0x0400U * PINBANK(pin)));
  GPIOx->MODER &= ~(3U << PINNO(pin) * 2);
  GPIOx->MODER |= (mode & 3U) << (PINNO(pin) * 2);
}

void GPIO_setOutputType(pin_t pin, GPIO_outputType type) {
  GPIO_TypeDef *GPIOx = (GPIO_TypeDef *)(GPIOA_BASE + (0x0400U * PINBANK(pin)));
  GPIOx->OTYPER &= ~(1U << PINNO(pin));
  GPIOx->OTYPER |= type << PINNO(pin);
}

void GPIO_setSPeed(pin_t pin, GPIO_speed speed) {
  GPIO_TypeDef *GPIOx = (GPIO_TypeDef *)(GPIOA_BASE + (0x0400U * PINBANK(pin)));
  GPIOx->OSPEEDR &= ~(3U << PINNO(pin) * 2);
  GPIOx->OSPEEDR |= (speed & 3U) << (PINNO(pin) * 2);
}

void GPIO_setPull(pin_t pin, GPIO_Pull UpOrDown) {
  GPIO_TypeDef *GPIOx = (GPIO_TypeDef *)(GPIOA_BASE + (0x0400U * PINBANK(pin)));
  GPIOx->PUPDR &= ~(3U << PINNO(pin) * 2);
  GPIOx->PUPDR |= (UpOrDown & 3U) << (PINNO(pin) * 2);
}

uint8_t GPIO_read(pin_t pin) {
  GPIO_TypeDef *GPIOx = (GPIO_TypeDef *)(GPIOA_BASE + (0x0400U * PINBANK(pin)));
  return (GPIOx->IDR >> PINNO(pin)) & 1;
}

void GPIO_write(pin_t pin, GPIO_pinState state) {
  GPIO_TypeDef *GPIOx = (GPIO_TypeDef *)(GPIOA_BASE + (0x0400U * PINBANK(pin)));
  GPIOx->ODR &= ~(1U << PINNO(pin));
  GPIOx->ODR |= state << PINNO(pin);
}

void GPIO_toggle(pin_t pin) {
  GPIO_TypeDef *GPIOx = (GPIO_TypeDef *)(GPIOA_BASE + (0x0400U * PINBANK(pin)));
  uint8_t tmp = (GPIOx->ODR >> PINNO(pin)) & 1;
  GPIOx->ODR &= ~(1U << PINNO(pin));
  GPIOx->ODR |= !tmp << PINNO(pin);
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ It might be a good idea to say exactly which GPIO chip you're driving. Perhaps also to show the device-tree fragment to show how it's used? \$\endgroup\$ Commented Aug 2, 2023 at 11:53
  • 2
    \$\begingroup\$ GPIO_setMode is a strange case, I would name functions gpio_set_mode for example \$\endgroup\$
    – coredump
    Commented Aug 2, 2023 at 13:03
  • 1
    \$\begingroup\$ this part could be an inline function (GPIO_TypeDef *)(GPIOA_BASE + (0x0400U * PINBANK(pin))) or macro \$\endgroup\$
    – coredump
    Commented Aug 2, 2023 at 13:05
  • \$\begingroup\$ @Double-Amphibian, With BIT(x) (1UL << (x)), why 1UL? Why not 1ULL or 1U? What is special about 1UL? \$\endgroup\$ Commented Aug 3, 2023 at 0:42

1 Answer 1

5
\$\begingroup\$
  • I am very uncomfortable with GPIO_write. It momentarily drives the pin low

      GPIOx->ODR &= ~(1U << PINNO(pin));
    

    before setting it to a desired state. Granted, it is ust one instruction cycle long, but it may very well confuse the peer.

    Consider instead

      uint8_t tmp = GPIOx->ODR;
      tmp &= ~(1U << PINNO(pin));
      tmp |= state << PINNO(pin);
      GPIOx->ODR = tmp;
    

    Ditto GPIO_toggle.

  • I'd seriously consider a protection against reading a pin configured for output (and vice versa).

  • As stated in comments,

      GPIO_TypeDef *GPIOx =(GPIO_TypeDef *)(GPIOA_BASE + (0x0400U * PINBANK(pin)));
    

    shall be factored out as an inline function or a macro.

  • GPIO_setSPeed looks like a typo. You don't want P capitalized.

  • GPIO_setClock looks like a misnomer. A cursory reader like me would expect such function to set a clock rate, which is meaningless for a general purpose IO. Consider GPIO_enable and GPIO_disable.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the advice! You wrote uint8_t tmp = GPIOx->ODR, shouldn't it be uint32_t? \$\endgroup\$ Commented Aug 3, 2023 at 17:22
  • \$\begingroup\$ @Double-Amphibian Perhaps it should. I didn't check the datasheet. Feel free to edit. \$\endgroup\$
    – vnp
    Commented Aug 3, 2023 at 23:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.