[PATCH v5 2/4] bsps: New GPIO API & peripherals API framework
oss at c-mauderer.de
oss at c-mauderer.de
Mon Aug 1 18:29:12 UTC 2022
Hello Duc,
Am 01.08.22 um 05:50 schrieb Duc Doan:
> Hello Christian,
>
> On Sat, 2022-07-30 at 17:50 +0200, oss at c-mauderer.de wrote:
>> 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"?
>>
>
> I will rename the C file to "gpio2.c" for now.
>
>>> 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.
>>
>
> Thanks for the reminder, I will add that.
>
>>> +/*
>>> + * 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?
>>
>
> I forgot to remove this CONFIGURE_GPIO_MAXIMUM_CONTROLLERS thing. At
> first I wanted to do something like confdef but couldn't, so I changed
> to use BSP_GPIO_NUM_CONTROLLERS, which is a build option. I will delete
> that code. However, if you have a document about how confdef works, it
> would be helpful for me and I may be able to make that
> CONFIGURE_GPIO_MAXIMUM_CONTROLLERS work.
I haven't checked since the new build system either. The official way is
still to just add them to confdefs.h similar like all other options. Not
sure whether that is documented somewhere. The better mut much more
difficult way is to add them via the specification items in
rtems-central. But I think a BSP option BSP_GPIO_NUM_CONTROLLERS is OK
for now. Please select a sensible default value. I think the bigger
controllers that I know have about 300 pins and GPIOs with 32 pins per
GPIO. That would be about 10 controllers. So a default could be
something like 16 and it should work in most cases. Small controllers
can save a few bytes by reducing the default.
>
>>> +
>>> +#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).
>>
>
> Sure, I will add that.
>
>>> +} 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.
>>
>
> I saw that in a file (I don't remember which one) so I thought that was
> the convention. But yes, I also found it pretty redundant. I will
> remove that.
In general a shorter description is easier to read. If I remember
correctly, one of my first commits to RTEMS was reworking some
documentation for a subsystem. I removed quite some of these long
descriptions in to make them better readable.
>
>>> + * 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?
>>
>
> This is a global 0.
>
>> 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.
>>
>
> Yes, I will write a general description at the beginning.
>
>>> + */
>>> + 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.
>>
>
> I will change that.
>
>>> + *
>>> + * @{
>>> + */
>>> +
>>> +/**
>>> + * @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.
>>
>
> Would keeping this function makes it easier for BSP? Like, each BSP
> does not need to separately use SYSINIT and care about the order; they
> just need to define a function.
>
SYSINIT is exactly for that: That a BSP and application can specify the
order itself. A system that supports the drivers should be initialized
before the drivers somewhere in RTEMS_SYSINIT_BSP_PRE_DRIVERS or maybe
even earlier.
Currently you force an order. What if some driver for a serial bus or
similar needs the GPIO system. It would have to be initialized after
your SYSINIT entry and therefore know where you put your initialization.
So for these cases you make it more difficult for the driver developer
who want's to use the GPIO system.
>>> +
>>> +/** @} */
>>> +
>>> +/**
>>> + * @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"?
>>
>
> Oh, I understood it incorrectly. I will change that.
>
>>> + */
>>> +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 was thinking that this periph_api is the base for other APIs, so I
> added "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.
>>
>
> That is also an option. The reason I separated them is that I thought
> these additional functions should be somehow separated from the basic
> GPIO to avoid making GPIO API too complicated. Also, this API is mostly
> for adding new peripheral APIs and not targetting user application.
> Users only need to use one function, set_api().
Please note that in our case a user can and often will add his own
drivers to an application too. Not all drivers are in the BSP. Some
applications need specialy tuned drivers and therefor will bring their
own ones.
>
>> 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.
>>
>
> Logically, a pin doesn't have to know about the connected peripheral.
> However, this newly added API is just a way to make things easier for
> users.
>
> Without this API, for each peripheral, users need to create new objects
> that hold information about both the GPIO pins and the handlers of that
> peripheral. They would have to maintain those objects all the time
> during the use of that peripheral. If users want to change the
> functionality, they have to create new objects of that peripheral
> type.
If the functionallity does not change, the peripheral can just know it's
pin and the user just has to know and handle the peripheral. Only during
the initialization he has to init the pin first and the peripheral
afterwards. But that's the case with your API too, isn't it?
If the functionality of a pin changes (which is a really rare use case)
the user will need two APIs. So if I understand your code correctly, he
would have to:
- Init GPIO pin.
- Add API for example for ADC.
- Use that pin.
- Remove API for ADC.
- Add API for example for DAC.
- Use the pin.
- Remove API for DAC.
...
So he has to switch in or out the API. For this switching he eitherhas
to provide memory for the ADC / DAC API or the memory is dynamically
allocated on every switch. I think I would rather keep multiple pointers
in my application instead of allocating something every time.
>
> This API avoids that by reusing the already-existing GPIO pin objects.
> Users only need a single GPIO object for a pin for all operations, be
> it basic GPIO or additional peripherals. This creates simplicity for
> user application at the cost of added memory (one additional pointer
> member, which is not much in my opinion).
>
For a single use pin that doesn't have to switch function, the user
needs only one pointer. The one to the object of the driver with the
function he uses. The pin can be a part of that object.
> By the way, this newly added API is mostly targeting peripherals that
> require a single pin like ADC or DAC.
That doesn't make it better. Now you added complexity for very special
cases ;-)
>
>>> @@ -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?
>>
>
> Hmm, I'm not sure because this is aimed for creating APIs. It's
> probably useful, so I'll add it.
>
>>> +} 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_gpi
>>> o *);
>>> +
>>> +/**
>>> + * @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.
>>
>
> It is called only in initialization. I'll remove the comment; I was
> putting there as a reminder but forgot about it...
>
>>> + 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?
>>
>
> Yes, unregistering is currently not supported.
>
OK. In that case I'm OK with this code.
>>> +// 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
>
> Thanks for the comments,
>
> Duc
>
Best regards
Christian
More information about the devel
mailing list