[PATCH v5 2/4] bsps: New GPIO API & peripherals API framework
oss at c-mauderer.de
oss at c-mauderer.de
Sat Jul 30 15:50:42 UTC 2022
Hello Duc,
Am 24.07.22 um 14:01 schrieb Duc Doan:
> ---
> bsps/include/bsp/gpio2.h | 528 ++++++++++++++++++++++++
> bsps/include/bsp/periph_api.h | 142 +++++++
> bsps/shared/dev/gpio/gpio.c | 212 ++++++++++
> bsps/shared/dev/periph_api/periph_api.c | 101 +++++
> spec/build/bsps/obj.yml | 4 +-
> 5 files changed, 986 insertions(+), 1 deletion(-)
> create mode 100644 bsps/include/bsp/gpio2.h
> create mode 100644 bsps/include/bsp/periph_api.h
> create mode 100644 bsps/shared/dev/gpio/gpio.c
> create mode 100644 bsps/shared/dev/periph_api/periph_api.c
>
> diff --git a/bsps/include/bsp/gpio2.h b/bsps/include/bsp/gpio2.h
I'm not really happy with the "2" in the name. But at the moment I don't
have a much better idea either. But if you call it "gpio2", you should
call the C files "gpio2" too. Maybe "dev/gpio/gpio.h" similar to the
"dev/i2c/i2c.h"?
> new file mode 100644
> index 0000000000..9cb1c720ab
> --- /dev/null
> +++ b/bsps/include/bsp/gpio2.h
> @@ -0,0 +1,528 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
Your file is missing some general doxygen group information. Take a look
at for example bsps/include/ofw/ofw.h.
> +/*
> + * Copyright (C) 2022 Duc Doan (dtbpkmte at gmail.com)
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef LIBBSP_BSP_GPIO2_H
> +#define LIBBSP_BSP_GPIO2_H
> +
> +#include <bsp.h>
> +#include <rtems.h>
> +
> +/**
> + * Configure the maximum number of GPIO controllers used in
> + * a application.
> + *
> + * The macro CONFIGURE_GPIO_MAXIMUM_CONTROLLERS can be
> + * defined in application code. If it is not defined,
> + * it will default to BSP_GPIO_NUM_CONTROLLERS. If BSP's
> + * number of controllers is not defined, it will default
> + * to 1.
> + */
> +#ifndef CONFIGURE_GPIO_MAXIMUM_CONTROLLERS
How do you manage that the CONFIGURE_GPIO_MAXIMUM_CONTROLLERS from the
application is visible here?
> +
> +#ifndef BSP_GPIO_NUM_CONTROLLERS
> +#define CONFIGURE_GPIO_MAXIMUM_CONTROLLERS 1
> +#else
> +#define CONFIGURE_GPIO_MAXIMUM_CONTROLLERS BSP_GPIO_NUM_CONTROLLERS
> +#endif /* BSP_GPIO_NUM_CONTROLLERS */
> +
> +#endif /* CONFIGURE_GPIO_MAXIMUM_CONTROLLERS */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +/**
> + * @brief Macro to initialize rtems_gpio.
> + *
> + * @param gpioh pointer to GPIO handlers
> + */
> +#define RTEMS_GPIO_BUILD_BASE(_gpio_handlers) \
> + (rtems_gpio) { .virtual_pin = 0, \
> + .gpio_handlers = ( _gpio_handlers ), \
> + .api = NULL \
> + };
> +
> +/**
> + * @name GPIO data structures
> + *
> + * @{
> + */
> +
> +/**
> + * @brief GPIO bit set and reset enumeration.
> + */
> +typedef enum {
> + RTEMS_GPIO_PIN_RESET = 0,
> + RTEMS_GPIO_PIN_SET = 1
> +} rtems_gpio_pin_state;
> +
> +/**
> + * @brief GPIO pin modes.
> + */
> +typedef enum {
> + RTEMS_GPIO_PINMODE_OUTPUT = 0,
> + RTEMS_GPIO_PINMODE_OUTPUT_PP = 0,
> + RTEMS_GPIO_PINMODE_OUTPUT_OD = 1,
> + RTEMS_GPIO_PINMODE_INPUT = 2,
> + RTEMS_GPIO_PINMODE_ANALOG = 3,
> + RTEMS_GPIO_PINMODE_BSP_SPECIFIC = 4
Maybe the BSP_SPECIFIC should start at a high, fixed offest so that
adding new general values is simpler. Something like
RTEMS_GPIO_PINMODE_BSP_SPECIFIC = 100,
A BSP can than use 100, 101, 102, ... for it's specific functions. If
someone later adds a generic RTEMS_GPIO_PINMODE_FOO = 4, it won't result
in any conflicts.
> +} rtems_gpio_pin_mode;
> +
> +/**
> + * @brief GPIO pull resistor configuration. Defines pull-up or
> + * pull-down activation.
> + */
> +typedef enum {
> + RTEMS_GPIO_NOPULL,
> + RTEMS_GPIO_PULLUP,
> + RTEMS_GPIO_PULLDOWN
Maybe add a "BSP_SPECIFIC" here too. I know of controllers that can
activate different values for pull up or pull down (like 100k or 10k).
> +} rtems_gpio_pull;
> +
> +/**
> + * @brief Interrupt modes enumeration.
> + */
> +typedef enum {
> + RTEMS_GPIO_INT_TRIG_NONE = 0,
> + RTEMS_GPIO_INT_TRIG_FALLING,
> + RTEMS_GPIO_INT_TRIG_RISING,
> + RTEMS_GPIO_INT_TRIG_BOTH_EDGES,
> + RTEMS_GPIO_INT_TRIG_LOW,
> + RTEMS_GPIO_INT_TRIG_HIGH
> +} rtems_gpio_interrupt_trig;
> +
> +typedef struct rtems_gpio_handlers rtems_gpio_handlers;
> +typedef struct rtems_gpio rtems_gpio;
> +/**
> + * @brief Typedef of the function pointer of an ISR.
> + */
> +typedef void (*rtems_gpio_isr)(void *);
> +
> +#include <bsp/periph_api.h>
> +
> +/**
> + * @brief Structure containing pointers to handlers of a
> + * BSP/driver. Each BSP/driver must define its own
> + * handlers and create an object of this structure
> + * with pointers to those handlers.
> + */
> +struct rtems_gpio_handlers {
> + /**
> + * @brief This member is the pointer to a handler for setting
> + * pin mode.
> + *
I would remove the repeating "This member is ...". Just describe it as
"Handler for setting the pin mode." It's clear that it is a member
because it is in a structure and it's clear that it is a pointer due to
it's type. So this information is just redundant and makes it longer to
read.
The same is true for all following handlers.
> + * Pin modes are from rtems_gpio_pin_mode enumeration.
I think doxygen wants a @ref if you reference some other object.
> + */
> + rtems_status_code (*set_pin_mode)(rtems_gpio *, rtems_gpio_pin_mode);
> +
> + /**
> + * @brief This member is the pointer to a handler for setting
> + * pull resistor mode.
> + *
> + * Pull resistor modes are from rtems_gpio_pull enumeration.
> + */
> + rtems_status_code (*set_pull)(rtems_gpio *, rtems_gpio_pull);
> +
> + /**
> + * @brief This member is the pointer to a handler for configuring
> + * interrupt of a pin.
> + *
> + * This handler should register ISR and its argument, interrupt
> + * trigger mode, and pull resister mode for the pin.
> + *
> + * @note Enabling interrupt should be done in enable_interrupt()
> + * handler.
> + */
> + rtems_status_code (*configure_interrupt)(rtems_gpio *, rtems_gpio_isr, void *, rtems_gpio_interrupt_trig, rtems_gpio_pull);
> +
> + /**
> + * @brief This member is the pointer to a handler for removing
> + * interrupt settings of a pin.
> + *
> + * Interrupt settings can be ISR address, pin configuration, etc.
> + */
> + rtems_status_code (*remove_interrupt)(rtems_gpio *);
> +
> + /**
> + * @brief This member is the pointer to a handler for enabling
> + * interrupt functionality of a pin.
> + */
> + rtems_status_code (*enable_interrupt)(rtems_gpio *);
> +
> + /**
> + * @brief This member is the pointer to a handler for disabling
> + * interrupt of a pin.
> + */
> + rtems_status_code (*disable_interrupt)(rtems_gpio *);
> +
> + /**
> + * @brief This member is the pointer to a handler for reading
> + * the digital value of a pin.
> + *
> + * The returned value should be in rtems_gpio_pin_state enum.
> + */
> + rtems_status_code (*read)(rtems_gpio *, rtems_gpio_pin_state *);
> +
> + /**
> + * @brief This member is the pointer to a handler for writing
> + * a digital value to a pin.
> + */
> + rtems_status_code (*write)(rtems_gpio *, rtems_gpio_pin_state);
> +
> + /**
> + * @brief This member is the pointer to a handler for toggling
> + * a pin.
> + *
> + * It should change pin state from SET to RESET or vice versa.
> + */
> + rtems_status_code (*toggle)(rtems_gpio *);
> +
> +};
> +
> +/**
> + * @brief Structure representing a GPIO object.
> + *
> + * BSP/drivers need to define their own GPIO structures with
> + * rtems_gpio being the first member.
> + */
> +struct rtems_gpio {
> + /**
> + * @brief This member is a virtual pin number, counting from
> + * 0 (zero).
Is that a global 0 or a per controller 0? Can you add that to the
doxygen description?
If you have to describe it multiple times: Maybe add a general GPIO-API
description. That's what the group right at the beginning of the file is
for.
> + */
> + uint32_t virtual_pin;
> + /**
> + * @brief This member is a pointer to a structure containing
> + * pointers to handlers of a GPIO controller.
> + */
> + const rtems_gpio_handlers *gpio_handlers;
> + /**
> + * @brief This member is a pointer to a peripheral API.
> + */
> + rtems_periph_api *api;
> +};
> +
> +/** @} */
> +
> +/**
> + * @name GPIO System operations
> + *
> + * Functions in this group should not be called by user
> + * applications. They are used by BSP/drivers only.
An application can add a driver. So the functions can be called by an
application too.
> + *
> + * @{
> + */
> +
> +/**
> + * @brief Perform initialization for GPIO system and
> + * Peripheral API system.
> + * This function is called automatically using
> + * SYSINIT.
> + */
> +extern void rtems_gpio_start(
> + void
> +);
You have the sysinit right below the function in the C file. I think you
can just remove the prototype and make the function static.
> +
> +/**
> + * @brief Registers a GPIO controller with GPIO manager.
> + *
> + * This function registers the pointer to BSP/driver-specific
> + * get_gpio() and destroy_gpio() functions. Those two functions
> + * are for creating and destroying GPIO objects. It also takes
> + * the number of pins of each BSP/driver for mapping into a
> + * flat pin numbering system (virtual pin number).
> + * This function also help register peripherals API get()
> + * and remove() functions.
> + *
> + * @param get_gpio The pointer to BSP/driver-specific get_gpio()
> + * @param destroy_gpio The pointer to BSP/driver-specific
> + * destroy_gpio()
> + * @param get_api The pointer to BSP/driver-specific get_api()
> + * @param remove_api The pointer to BSP/driver-specific remove_api()
> + * @param pin_count The number of GPIO pins in the controller
> + *
> + * @retval RTEMS_SUCCESSFUL Controller registered successfully
> + * @retval RTEMS_TOO_MANY if the maximum number of controllers are
> + * already registered
> + */
> +extern rtems_status_code rtems_gpio_register(
> + rtems_status_code (*get_gpio)(uint32_t, rtems_gpio **),
> + rtems_status_code (*destroy_gpio)(rtems_gpio *),
> + rtems_periph_api *(*get_api)(rtems_gpio *, rtems_periph_api_type),
> + rtems_status_code (*remove_api)(rtems_gpio *),
> + uint32_t pin_count
> +);
> +
> +/** @} */
> +
> +/**
> + * @name GPIO BSP functions
> + *
> + * BSP and drivers need to implement these.
I would replace "BSP" by "drivers" and not mention both. It's not
relevant whether the driver is in the BSP or in the application.
> + *
> + * @{
> + */
> +
> +/**
> + * @brief Wrapper to perform all BSP controllers registering
> + * with the GPIO manager.
> + *
> + * This function must be implemented by BSP. It should call
> + * rtems_gpio_register() to register each integrated GPIO
> + * controller.
> + */
> +extern rtems_status_code bsp_gpio_register_controllers(
> + void
> +);
I would remove that function. Let the BSP use SYSINIT for that. Make
sure that the rtems_periph_api_start is started before the drivers in
RTEMS_SYSINIT_BSP_PRE_DRIVERS.
> +
> +/** @} */
> +
> +/**
> + * @name GPIO Application operations
> + *
> + * @{
> + */
> +
> +/**
> + * @brief Get the GPIO object containing information
> + * about the specified pin.
> + *
> + * This function maps the virtual pin to intermediate pin,
> + * and pass to the BSP/driver-specific function to get a
> + * GPIO object.
> + *
> + * @note Warning: this function may use malloc(). When you
> + * are done with the GPIO object, call rtems_gpio_destroy()
> + * to avoid memory leak.
> + *
> + * @param virt_pin The virtual pin number.
> + * @param[out] out The pointer to the pointer to the output
> + * GPIO object.
> + *
> + * @retval RTEMS_SUCCESSFUL
> + * @retval RTEMS_UNSTISFIED if the virtual pin number
> + * is invalid (i.e. out of range)
> + */
> +extern rtems_status_code rtems_gpio_get(
> + uint32_t virt_pin,
> + rtems_gpio **out
> +);
> +
> +/**
> + * @brief Destroy a GPIO object
> + *
> + * This function should be called on an GPIO object which is
> + * no longer used to avoid memory leak. Internally it can
> + * use free().
> + *
> + * @param[in] base The pointer to the GPIO object.
> + *
> + * @retval RTEMS_SUCCESSFUL
> + * @retval RTEMS_UNSATISFIED
> + */
> +extern rtems_status_code rtems_gpio_destroy(
> + rtems_gpio *base
> +);
> +
> +/**
> + * @brief Sets the pin mode of a pin.
> + *
> + * This function calls the registered BSP/driver-specific handler.
> + * It can be used to set the pin mode for a pin.
> + *
> + * @param[in] base The GPIO object to be configured.
> + * @param mode The pin mode from the enumeration rtems_gpio_pin_mode
> + *
> + * @retval RTEMS_SUCCESSFUL GPIO configured successfully.
> + * @retval RTEMS_UNSATISFIED Could not set the pin mode.
> + * @retval @see BSP/driver-specific function for other return codes.
> + */
> +extern rtems_status_code rtems_gpio_set_pin_mode(
> + rtems_gpio *base,
> + rtems_gpio_pin_mode mode
> +);
> +
> +/**
> + * @brief Sets the pin's pull resistor configuration.
> + *
> + * This function calls the registered BSP/driver-specific handler.
> + * It can be used to set the pull resistor mode for a pin.
> + *
> + * @param[in] base The GPIO object to be configured.
> + * @param mode The pull mode from the enumeration rtems_gpio_pull
> + *
> + * @retval RTEMS_SUCCESSFUL GPIO configured successfully.
> + * @retval RTEMS_UNSATISFIED Could not set the pull resistor mode.
> + * @retval @see BSP/driver-specific function for other return codes.
The doxygen "@see" wants a reference where it can point to. I don't
think that you have one for "BSP/driver-specific"?
> + */
> +extern rtems_status_code rtems_gpio_set_pull(
> + rtems_gpio *base,
> + rtems_gpio_pull pull
> +);
> +
> +/**
> + * @brief Configure interrupt on a GPIO pin.
> + *
> + * This function calls the registered BSP/driver-specific handler.
> + * It can be used to set up a pin for interrupt mode.
> + *
> + * @note This only configures the interrupt but not enable it. Use
> + * rtems_gpio_enable_interrupt() to actually enable interrupt.
> + *
> + * @param[in] base The GPIO object to be configured.
> + * @param isr The pointer to the ISR to be attached to this pin.
> + * @param[in] arg The pointer to the argument of the ISR.
> + * @param trig The trigger mode
> + * @param pull The pull resistor mode
> + *
> + * @retval RTEMS_SUCCESSFUL Interrupt configured successfully.
> + * @retval RTEMS_UNSATISFIED Could not configure interrupt.
> + * @retval @see BSP/driver-specific function for other return codes.
> + */
> +extern rtems_status_code rtems_gpio_configure_interrupt(
> + rtems_gpio *base,
> + rtems_gpio_isr isr,
> + void *arg,
> + rtems_gpio_interrupt_trig trig,
> + rtems_gpio_pull pull
> +);
> +
> +/**
> + * @brief Remove interrupt settings for a pin.
> + *
> + * This function calls the registered BSP/driver-specific handler.
> + * It can be used to remove interrupt configuration of a pin like
> + * ISR, ISR argument, pin mode, etc.
> + *
> + * @note This only removes the interrupt but not disable it. Use
> + * rtems_gpio_disable_interrupt() to actually disable
> + * interrupt.
> + *
> + * @param[in] base The GPIO object to be configured.
> + *
> + * @retval RTEMS_SUCCESSFUL Interrupt removed successfully.
> + * @retval RTEMS_UNSATISFIED Could not remove interrupt.
> + * @retval @see BSP/driver-specific function for other return codes.
> + */
> +extern rtems_status_code rtems_gpio_remove_interrupt(
> + rtems_gpio *base
> +);
> +
> +/**
> + * @brief Enable interrupt on a GPIO pin.
> + *
> + * This function calls the registered BSP/driver-specific handler.
> + *
> + * @note This function only enables the interrupt (e.g. the vector)
> + * but not configure the pin. Use
> + * rtems_gpio_configure_interrupt() for pin configuration.
> + *
> + * @param[in] base The GPIO object to be configured.
> + *
> + * @retval RTEMS_SUCCESSFUL Interrupt enabled successfully.
> + * @retval RTEMS_UNSATISFIED Could not enable interrupt.
> + * @retval @see BSP/driver-specific function for other return codes.
> + */
> +extern rtems_status_code rtems_gpio_enable_interrupt(
> + rtems_gpio *base
> +);
> +
> +/**
> + * @brief Disable interrupt on a GPIO pin.
> + *
> + * This function calls the registered BSP/driver-specific handler.
> + *
> + * @note This function only disables the interrupt (e.g. the vector)
> + * but not remove the pin's configurations. Use
> + * rtems_gpio_remove_interrupt() for the latter purpose.
> + *
> + * @param[in] base The GPIO object to be configured.
> + *
> + * @retval RTEMS_SUCCESSFUL Interrupt disabled successfully.
> + * @retval RTEMS_UNSATISFIED Could not disable interrupt.
> + * @retval @see BSP/driver-specific function for other return codes.
> + */
> +extern rtems_status_code rtems_gpio_disable_interrupt(
> + rtems_gpio *base
> +);
> +
> +/**
> + * @brief Writes a digital value to a pin.
> + *
> + * This function calls the registered BSP/driver-specific handler.
> + *
> + * @param[in] base The GPIO object that has information about the
> + * pin.
> + * @param value The state to be written to the pin.
> + *
> + * @retval RTEMS_SUCCESSFUL Pin successfully written.
> + * @retval @see BSP/driver-specific function for other return codes.
> + */
> +extern rtems_status_code rtems_gpio_write(
> + rtems_gpio *base,
> + rtems_gpio_pin_state value
> +);
> +
> +/**
> + * @brief Reads the digital value of a pin.
> + *
> + * This function calls the registered BSP/driver-specific handler.
> + *
> + * @param[in] base The GPIO object that has information about the
> + * pin.
> + *
> + * @param[out] value The state of the pin.
> + *
> + * @retval RTEMS_SUCCESSFUL Pin succesfully read.
> + * @retval @see BSP/driver-specific function for other return codes.
> + */
> +extern rtems_status_code rtems_gpio_read(
> + rtems_gpio *base,
> + rtems_gpio_pin_state *value
> +);
> +
> +/**
> + * @brief Toggles the state of a GPIO pin.
> + *
> + * This function calls the registered BSP/driver-specific handler.
> + *
> + * @param[in] base The GPIO object that has information about the
> + * pin.
> + *
> + * @retval RTEMS_SUCCESSFUL Pin successfully toggled.
> + * @retval @see BSP/driver-specific function for other return codes.
> + */
> +extern rtems_status_code rtems_gpio_toggle(
> + rtems_gpio *base
> +);
> +
> +/** @} */
> +
> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* LIBBSP_BSP_GPIO2_H */
> diff --git a/bsps/include/bsp/periph_api.h b/bsps/include/bsp/periph_api.h
> new file mode 100644
> index 0000000000..fb02b701dc
> --- /dev/null
> +++ b/bsps/include/bsp/periph_api.h
Isn't it an API just because you add it here. Is it really necessary to
add the "API" in the name?
I'm not entirely sure yet whether this API is really something separate
from your GPIO API. The "gpio_start" calls the "periph_api_start" and
the periph_api functions use gpio internal structures. I think I would
just add these few functions to the GPIO API.
I'm still not convinced that this is necessary at all. A peripheral has
to know it's pins. But the pin doesn't have to know anything about the
connected peripheral. So why do we need that? At the moment it seems to
add mainly some complexity and uses some memory.
> @@ -0,0 +1,142 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/*
> + * Copyright (C) 2022 Duc Doan (dtbpkmte at gmail.com)
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef LIBBSP_BSP_PERIPH_API_H
> +#define LIBBSP_BSP_PERIPH_API_H
> +
> +#include <bsp.h>
> +#include <rtems.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +/**
> + * @brief Enumeration of supported peripheral APIs.
> + */
> +typedef enum {
> + RTEMS_PERIPH_API_TYPE_ADC
Would a BSP / Application specific range be usefull?
> +} rtems_periph_api_type;
> +
> +/**
> + * @brief The base structure of peripheral APIs.
> + * All peripheral APIs must have this struct as
> + * their first member.
> + *
> + * @see rtems_adc_api for example implementation.
> + */
> +typedef struct rtems_periph_api rtems_periph_api;
> +
> +#include <bsp/gpio2.h>
> +
> +/**
> + * rtems_periph_api structure definition.
> + */
> +struct rtems_periph_api {
> + /**
> + * @brief This member tells the type of this
> + * API.
> + */
> + rtems_periph_api_type api_type;
> + /**
> + * @brief This member is a pointer to a
> + * BSP/driver-specific function that
> + * performs initialization for the
> + * API.
> + */
> + void (*init)(rtems_gpio *pin);
> +};
> +
> +/**
> + * @brief Register a handler to get API pointer.
> + *
> + * @param[in] get_api pointer to a handler to
> + * get an API with specified type.
> + * @param index the index of the GPIO controller.
> + */
> +void rtems_periph_api_register_api(
> + rtems_periph_api *(*get_api)(rtems_gpio *, rtems_periph_api_type),
> + rtems_status_code (*remove_api)(rtems_gpio *),
> + uint32_t index
> +);
> +
> +/**
> + * @brief Performs initialization for peripheral
> + * API system.
> + * This function registers some variables that
> + * the peripheral API utilize from GPIO API.
> + *
> + * @param get_ctrl_index Pointer to a helper
> + * that returns a GPIO controller index
> + * of a pin.
> + * @param num_ctrl Pointer to a variable that
> + * tells the number of registered GPIO
> + * controllers.
> + */
> +void rtems_periph_api_start(
> + uint32_t (*get_ctrl_index)(uint32_t),
> + uint32_t *num_ctrl
> +);
> +
> +/**
> + * @brief Assign an API to a GPIO object and
> + * initialize it.
> + *
> + * @note This function may use malloc().
> + * @note This function calls the handler init().
> + *
> + * @param pin The rtems_gpio object representing
> + * a pin.
> + * @param type The peripheral API type.
> + *
> + * @retval RTEMS_SUCCESSFUL if an API is set
> + * correctly.
> + * @retval RTEMS_UNSATISFIED if the API type is
> + * invalid for this pin.
> + * @retval RTEMS_NO_MEMORY if memory cannot be
> + * allocated for API object.
> + */
> +rtems_status_code rtems_periph_api_set_api(
> + rtems_gpio *pin,
> + rtems_periph_api_type type
> +);
> +
> +/**
> + * @brief Remove the API assigned to a pin by
> + * setting the pointer to NULL.
> + *
> + * @retval RTEMS_SUCCESSFUL
> + */
> +rtems_status_code rtems_periph_api_remove_api(
> + rtems_gpio *pin
> +);
> +
> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* LIBBSP_BSP_PERIPH_API_H */
> diff --git a/bsps/shared/dev/gpio/gpio.c b/bsps/shared/dev/gpio/gpio.c
> new file mode 100644
> index 0000000000..295b9c3738
> --- /dev/null
> +++ b/bsps/shared/dev/gpio/gpio.c
> @@ -0,0 +1,212 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/*
> + * Copyright (C) 2022 Duc Doan (dtbpkmte at gmail.com)
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <bsp/gpio2.h>
> +#include <rtems/sysinit.h>
> +
> +/**
> + * @brief An array to store all registered GPIO controllers.
> + */
> +static rtems_status_code (*get_gpio_table[CONFIGURE_GPIO_MAXIMUM_CONTROLLERS])(uint32_t, rtems_gpio **);
> +
> +/**
> + * @brief An array to store all registered GPIO controllers.
> + */
> +static rtems_status_code (*destroy_gpio_table[CONFIGURE_GPIO_MAXIMUM_CONTROLLERS])(rtems_gpio *);
> +
> +/**
> + * @brief An array to store the boundaries of pin index of
> + * GPIO controllers.
> + *
> + * Example with 2 16-pin controllers and 1 32-pin controller,
> + * the pin_map will be:
> + * { 0, 16, 32, 64}
> + * Value 0 is always at index 0 for convenience of calculation.
> + * The length of this array is always 1+(number of controllers).
> + */
> +static uint32_t pin_map[CONFIGURE_GPIO_MAXIMUM_CONTROLLERS+1] = {0};
> +
> +/**
> + * @brief The number of controllers registered.
> + */
> +static uint32_t num_ctrl = 0;
> +
> +static uint32_t get_ctrl_index(
> + uint32_t virtual_pin
> +);
> +
> +static uint32_t get_ctrl_index(
> + uint32_t virtual_pin
> +)
> +{
> + uint32_t i;
> + // TODO: binary search
If you add comments that should stay in the code: Please use C style
comments with /* */
Regarding the binary search: Is it called during normal useage or only
on pin initialization. If the later one is the case, I don't think that
it has to be optimized.
> + for (i = 1; i <= num_ctrl; ++i) {
> + if (virtual_pin < pin_map[i]) {
> + break;
> + }
> + }
> + return i-1;
> +}
> +
> +rtems_status_code rtems_gpio_register(
> + rtems_status_code (*get_gpio)(uint32_t, rtems_gpio **),
> + rtems_status_code (*destroy_gpio)(rtems_gpio *),
> + rtems_periph_api *(*get_api)(rtems_gpio *, rtems_periph_api_type),
> + rtems_status_code (*remove_api)(rtems_gpio *),
> + uint32_t pin_count
> +)
> +{
> +// rtems_interrupt_level level;
If it is not necessary, please remove commented code completely.
> +
> + if (num_ctrl == CONFIGURE_GPIO_MAXIMUM_CONTROLLERS)
> + return RTEMS_TOO_MANY;
> +
> +// rtems_interrupt_disable(level);
> + get_gpio_table[num_ctrl] = get_gpio;
> + destroy_gpio_table[num_ctrl] = destroy_gpio;
> +
> + rtems_periph_api_register_api(get_api, remove_api, num_ctrl);
> +
> + pin_map[num_ctrl+1] = pin_map[num_ctrl] + pin_count;
> + ++num_ctrl;
You support only registering controllers. Unregistering is not
supported. Is that correct?
> +// rtems_interrupt_enable(level);
> +
> + return RTEMS_SUCCESSFUL;
> +}
> +
> +rtems_status_code rtems_gpio_get(
> + uint32_t virt_pin,
> + rtems_gpio **out
> +)
> +{
> + uint32_t i = get_ctrl_index(virt_pin);
> + if (i >= num_ctrl)
> + return RTEMS_UNSATISFIED;
> +
> + uint32_t pin = virt_pin - pin_map[i];
> + rtems_status_code sc = (*get_gpio_table[i])(pin, out);
> + if (sc == RTEMS_SUCCESSFUL) {
> + (*out)->virtual_pin = virt_pin;
> + }
> + return sc;
> +}
> +
> +rtems_status_code rtems_gpio_destroy(
> + rtems_gpio *base
> +)
> +{
> + uint32_t i = get_ctrl_index(base->virtual_pin);
> + if (i >= num_ctrl)
> + return RTEMS_UNSATISFIED;
> + return (*destroy_gpio_table[i])(base);
> +}
> +
> +void rtems_gpio_start(
> + void
> +)
> +{
> + rtems_periph_api_start(get_ctrl_index, &num_ctrl);
> + bsp_gpio_register_controllers();
See above: I would remove the bsp_gpio_register_controllers() ...
> +}
> +RTEMS_SYSINIT_ITEM(
> + rtems_gpio_start,
> + RTEMS_SYSINIT_DEVICE_DRIVERS,
> + RTEMS_SYSINIT_ORDER_LAST_BUT_1
... and move that to the RTEMS_SYSINIT_BSP_PRE_DRIVERS.
Best regards
Christian
> +);
> +
> +rtems_status_code rtems_gpio_set_pin_mode(
> + rtems_gpio *base,
> + rtems_gpio_pin_mode mode
> +)
> +{
> + return base->gpio_handlers->set_pin_mode(base, mode);
> +}
> +
> +rtems_status_code rtems_gpio_set_pull(
> + rtems_gpio *base,
> + rtems_gpio_pull pull
> +)
> +{
> + return base->gpio_handlers->set_pull(base, pull);
> +}
> +
> +rtems_status_code rtems_gpio_configure_interrupt(
> + rtems_gpio *base,
> + rtems_gpio_isr isr,
> + void *arg,
> + rtems_gpio_interrupt_trig trig,
> + rtems_gpio_pull pull
> +)
> +{
> + return base->gpio_handlers->configure_interrupt(base, isr, arg, trig, pull);
> +}
> +
> +rtems_status_code rtems_gpio_remove_interrupt(
> + rtems_gpio *base
> +)
> +{
> + return base->gpio_handlers->remove_interrupt(base);
> +}
> +
> +rtems_status_code rtems_gpio_enable_interrupt(
> + rtems_gpio *base
> +)
> +{
> + return base->gpio_handlers->enable_interrupt(base);
> +}
> +
> +rtems_status_code rtems_gpio_disable_interrupt(
> + rtems_gpio *base
> +)
> +{
> + return base->gpio_handlers->disable_interrupt(base);
> +}
> +
> +rtems_status_code rtems_gpio_write(
> + rtems_gpio *base,
> + rtems_gpio_pin_state value
> +)
> +{
> + return base->gpio_handlers->write(base, value);
> +}
> +
> +rtems_status_code rtems_gpio_read(
> + rtems_gpio *base,
> + rtems_gpio_pin_state *value
> +)
> +{
> + return base->gpio_handlers->read(base, value);
> +}
> +
> +rtems_status_code rtems_gpio_toggle(
> + rtems_gpio *base
> +)
> +{
> + return base->gpio_handlers->toggle(base);
> +}
> +
> diff --git a/bsps/shared/dev/periph_api/periph_api.c b/bsps/shared/dev/periph_api/periph_api.c
> new file mode 100644
> index 0000000000..4bc6b02939
> --- /dev/null
> +++ b/bsps/shared/dev/periph_api/periph_api.c
> @@ -0,0 +1,101 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/*
> + * Copyright (C) 2022 Duc Doan (dtbpkmte at gmail.com)
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <bsp/periph_api.h>
> +
> +/**
> + * @brief An array to store handlers to get peripheral
> + * API of each registered GPIO controller.
> + */
> +static rtems_periph_api *(*get_api_table[CONFIGURE_GPIO_MAXIMUM_CONTROLLERS])(rtems_gpio *, rtems_periph_api_type);
> +
> +/**
> + * @brief An array to store handlers to remove peripheral
> + * API of each registered GPIO controller.
> + */
> +static rtems_status_code (*remove_api_table[CONFIGURE_GPIO_MAXIMUM_CONTROLLERS])(rtems_gpio *);
> +
> +/**
> + * Mirrors to GPIO private variables/
> + */
> +static uint32_t (*get_ctrl_index_ptr)(uint32_t);
> +static uint32_t *num_ctrl_ptr;
> +
> +
> +void rtems_periph_api_start(
> + uint32_t (*get_ctrl_index)(uint32_t),
> + uint32_t *num_ctrl
> +)
> +{
> + get_ctrl_index_ptr = get_ctrl_index;
> + num_ctrl_ptr = num_ctrl;
> +}
> +
> +void rtems_periph_api_register_api(
> + rtems_periph_api *(*get_api)(rtems_gpio *, rtems_periph_api_type),
> + rtems_status_code (*remove_api)(rtems_gpio *),
> + uint32_t index
> +)
> +{
> + get_api_table[index] = get_api;
> + remove_api_table[index] = remove_api;
> +}
> +
> +rtems_status_code rtems_periph_api_set_api(
> + rtems_gpio *pin,
> + rtems_periph_api_type type
> +)
> +{
> + // Prevent memory-leak by removing the old API
> + // first.
> + if (pin->api != NULL)
> + rtems_periph_api_remove_api(pin);
> +
> + uint32_t i = (*get_ctrl_index_ptr)(pin->virtual_pin);
> + if (i >= *num_ctrl_ptr)
> + return RTEMS_UNSATISFIED;
> +
> + pin->api = (*get_api_table[i])(pin, type);
> + if (pin->api == NULL)
> + return RTEMS_UNSATISFIED;
> +
> + // Initialize the API object
> + pin->api->init(pin);
> +
> + return RTEMS_SUCCESSFUL;
> +}
> +
> +rtems_status_code rtems_periph_api_remove_api(
> + rtems_gpio *pin
> +)
> +{
> + uint32_t i = (*get_ctrl_index_ptr)(pin->virtual_pin);
> + if (i >= *num_ctrl_ptr)
> + return RTEMS_UNSATISFIED;
> +
> + return (*remove_api_table[i])(pin);
> +}
> diff --git a/spec/build/bsps/obj.yml b/spec/build/bsps/obj.yml
> index ce05c11c0e..80736c197d 100644
> --- a/spec/build/bsps/obj.yml
> +++ b/spec/build/bsps/obj.yml
> @@ -16,7 +16,7 @@ install:
> - bsps/include/bsp/default-initial-extension.h
> - bsps/include/bsp/fatal.h
> - bsps/include/bsp/fdt.h
> - - bsps/include/bsp/gpio.h
> + - bsps/include/bsp/periph_api.h
> - bsps/include/bsp/gpio2.h
> - bsps/include/bsp/irq-default.h
> - bsps/include/bsp/irq-generic.h
> @@ -71,6 +71,8 @@ links:
> - role: build-dependency
> uid: objnosmp
> source:
> +- bsps/shared/dev/gpio/gpio.c
> +- bsps/shared/dev/periph_api/periph_api.c
> - bsps/shared/dev/display/disp_hcms29xx.c
> - bsps/shared/dev/display/font_hcms29xx.c
> - bsps/shared/dev/i2c/i2c-2b-eeprom.c
More information about the devel
mailing list