Arm GPIO driver

Question

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);
}

Answer 1

• 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.