Hello Duc,

Am 01.08.22 um 05:50 schrieb Duc Doan:
Hello Christian,

On Sat, 2022-07-30 at 17:50 +0200, o...@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
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Reply via email to