On 23-06-2015 17:16, Gedare Bloom wrote:
+ uint32_t bank;
+ int handled_count;
+ int rv;
+
+ gpio = (gpio_pin*) arg;
+
+ bank = gpio->bank_number;
Validate args for errors.
These tasks are only created by the rtems_gpio_enable_interrupt() function,
and all parameters are validated before creating/starting the task. Should
they be validated again here?
No, as long as validated in the entry-ways to the module it should be fine.
+
+ AQUIRE_LOCK(bank_lock[bank]);
What is the lock protecting?
Since the rtems_gpio_disable_interrupt() kills this task when the interrupt
is disabled, having the lock here (which rtems_gpio_disable_interrupt() must
also acquire before doing anything) ensures that any interrupt disable call
occurring during an interrupt process will have to wait while the on-going
interrupt is processed. Otherwise the handler could leave the system on an
inconsistent state. I shall make this clear with a comment.
Do we really want the task to be killed? Anyway, the naming of this
"gpio_disable_interrupt" and "gpio_enable_interrupt" is a bit
confusing to me, since it really is about creating/destroying the
interrupt handling thread? I don't quite understand the design there.
When an interrupt is enabled on a pin, a task is created to call its
handler/handlers (this task is put to sleep and woken by the bank's ISR
each time an interrupt on that pin is sensed). If that interrupt is
disabled on that pin, the task is killed because the pin will not be
providing more interrupts. An instance of this task is created for every
pin with an interrupt enabled.
+ if ( handled_count == 0 ) {
+ bsp_interrupt_handler_default(rtems_bsp_gpio_get_vector(bank));
+ }
+
+ RELEASE_LOCK(bank_lock[bank]);
+ }
+}
+
+static void generic_isr(void* arg)
+{
+ rtems_vector_number vector;
+ uint32_t event_status;
+ uint32_t bank_pin_count;
+ gpio_pin *gpio;
+ uint32_t bank_number;
+ int i;
+
+ gpio = (gpio_pin *) arg;
+
+ /* Get the bank/vector number of a pin (e.g.: 0) from this bank. */
+ bank_number = gpio[0].bank_number;
+
Again, validate args. Why is it gpio[0] here? And not above?
As with generic_handler_task, these ISRs are only created by
rtems_gpio_enable_interrupt() and rtems_gpio_disable_interrupt(), which
validate the arguments sent here. Should they be re-validated here?
This is a little different, in that the code path gets reached
indirectly via interrupt handling. I recommand always verifying these
inputs, even if it is with debug asserts only.
Noted.
As for the gpio[0], the gpio pointer points to a whole gpio bank, so gpio[0]
is the first pin of this bank. Since all pins in this bank have the same
bank number, I can use any pin from it to obtain the bank number that this
ISR is listening. Maybe gpio should be renamed to gpio_bank.
Ah. Something bugs me about redundantly storing the same bank number
32 times... Is there a better data structure for you to use to keep
track of all the pins and their metadata / configurations?
There may be a few alternatives:
1. With the current bank*bank_pins matrix structure, since I always need
to have the bank and pin number to access a GPIO pin I can ommit these
values from the pin struct. This, however, requires about two divisions
to calculate the bank/pin number combination, but saves 64 bits per pin
in memory.
2. Use the uni-dimensional array with direct access with pin_number (see
the end of this e-mail), and keep the bank/pin data for each pin struct,
but now I do not have to calculate the bank/pin from pin_number.
3. The application could use the bank/pin number directly to refer to a
pin, instead of the global pin number. However, if the bsp wants to
define a series of constants for each pin, it would become more
difficult since it would have to store two numbers (#define GPIO_40 40
would not be possible), so each platform can either not define any pin
reference (each application hardcodes the pin reference on their
configuration), or it would have to resort to a table of structs,
probably facing this same problem.
Maybe option 3 is better with the hardcoded reference to the bank/pin
combination, otherwise I feel like the only option becomes a choice
between 1 or 2, or moving the problem around. The usefulness of having
platform defined references to the pins may also be arguable.
+
+ /* Obtains a 32-bit bitmask, with the pins currently reporting
interrupts
+ * signaled with 1. */
+ event_status = rtems_bsp_gpio_interrupt_line(vector);
+
Is it ever the case there are more than 32 pins in a bank? (If not,
this should be assert()ed somewhere in the code.)
There may be bigger (up to 64?). But yes it would help to define a max pin
number for a single bank, and have that asserted. Would it be best to set a
max pin number per bank of 64 pins, and change this to a 64-bit bitmask to
account for any number in-between? The reasoning behind the pin bitmask is
that most platforms should be able to directly (or after some bitwise
operations) write it to the registers.
Stick to 32.
+ /* Iterates through the bitmask and calls the corresponding handler
+ * for active interrupts. */
+ for ( i = 0; i < bank_pin_count; ++i ) {
+ /* If active, wake the corresponding pin's ISR task. */
+ if ( event_status & (1 << i) ) {
+ rtems_event_send(gpio[i].task_id, RTEMS_EVENT_1);
It may improve readability to #define something as RTEMS_EVENT_1. like
#define GPIO_INTERRUPT_ACTIVE RTEMS_EVENT_1.
maybe GPIO_INTERRUPT_EVENT?
Yes.
+ }
+
+ bitmask = 0;
+
+ for ( i = 0; i < count; ++i ) {
+ pin_number = va_arg(args, uint32_t);
+
+ if ( pin_number < 0 || pin_number >= gpio_count ) {
+ *sc = RTEMS_INVALID_ID;
+
+ return 0;
+ }
+
+ bank = BANK_NUMBER(pin_number);
If you already know the bank_number, you can replace this with
if(BANK_NUMBER(pin_number) != bank_number) return RTEMS_UNSATISFIED;
I never know the bank number until the first iteration ( the i == 0 case
bellow).
You can pre-compute this before the loop.
+
+ return RTEMS_UNSATISFIED;
+ }
+
+ sc = rtems_gpio_resistor_mode(conf->pin_number, conf->pull_mode);
+
+ if ( sc != RTEMS_SUCCESSFUL ) {
+ RTEMS_SYSLOG_ERROR("rtems_gpio_resistor_mode failed with status code
%d\n", sc);
+
+ return RTEMS_UNSATISFIED;
+ }
+
+ interrupt_conf = (rtems_gpio_interrupt_conf*) conf->interrupt;
+
+ if ( interrupt_conf != NULL ) {
+ bank = BANK_NUMBER(conf->pin_number);
+ pin = PIN_NUMBER(conf->pin_number);
Why doesn't the conf store these, anyway?
The API calculates the pin and bank numbers from the "global" pin number as
a protection mechanism against a change in the GPIO layout. On a second
thougth, I do not actually see a reason for it to change, unless there is an
application where I would like to work with banks instead of individual
pins, and would change the GPIO layout accordingly. But even in that case I
could just create a separate conf file with those specific requirements (and
maybe it can allow to define multiple pins with the same conf, to form a
group that is to be treated as a single entity, if that can be a thing?).
Using configuration structs to abstract the pins/banks/groups-of-pins
makes sense to me. Having an efficient way to map from the pin number
to its configuration could solve the dilemma you face about wanting to
keep things simple. A linear index (array) works fine for 1:1
pin-to-conf. A tree (rbtree) works for sparse mappings. Or rely on the
application to keep track of the confs itself.
One issue that arises with storing the bank and pin numbers directly is that
it becomes more hardcoded. With a global pin number a bsp can define a pin
with
#define GPIO_40 40
and use that to refer to that pin. By having to explicitly use the bank and
pin numbers, I would have two numbers that would be hardcoded (and the
separate conf file would apply here).
Or the application passes the configuration instead of the numbers.
Then the configuration can store things like pin/bank numbers.
So yes, probably the bank and pin numbers can be directly stored. The other
API calls can also receive the two numbers, or at least the
rtems_gpio_request_pin could. If this function (and rtems_gpio_request_conf)
would return a gpio_pin_id var/struct with the bank and pin number
combination, the following calls would only use this gpio_pin_id, and could
even avoid the bank/pin check if this type is opaque.
+
+ AQUIRE_LOCK(bank_lock[bank]);
And also store its own lock field?
The lock field is per bank. Each conf struct only refer to an individual
pin.
OK, or possibly a group of pins, fine. It could store a pointer to the
lock maybe, but this global array of locks is I suppose acceptable.
+int rtems_gpio_get_value(uint32_t pin_number)
+{
+ uint32_t bank;
+ uint32_t pin;
+ int rv;
+
+ if ( pin_number < 0 || pin_number >= gpio_count ) {
+ return -1;
+ }
+
+ bank = BANK_NUMBER(pin_number);
+ pin = PIN_NUMBER(pin_number);
+
+ AQUIRE_LOCK(bank_lock[bank]);
+
+ if ( gpio_pin_state[bank][pin].pin_function != DIGITAL_INPUT) {
+ RELEASE_LOCK(bank_lock[bank]);
+
+ RTEMS_SYSLOG_ERROR("Can only read digital input pins\n");
+
+ return -1;
+ }
+
+ rv = rtems_bsp_gpio_get_value(bank, pin);
+
+ if ( gpio_pin_state[bank][pin].logic_invert && rv > 0 ) {
why rv > 0?
The rtems_bsp_gpio_get_value can return -1 if for some reason it failed to
get a value. With the rv > 0 condition that would be sent to an application
through the last return.
So in case of an error, the lock remains held? What does rv==0 signify?
If rtems_bsp_gpio_get_value() has an error it should return -1 and then
the lock is released below. In success that function should return 0 (rv
== 0) if the pin level is currently logical low, or a number higher than
0 if the pin level is a logical high.
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return !rv;
+ }
+
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return ( rv > 0 ) ? 1 : rv;
+}
+
+rtems_status_code rtems_gpio_request_pin(uint32_t pin_number,
rtems_gpio_function function, bool output_enabled, bool logic_invert, void*
bsp_specific)
+{
+ rtems_gpio_specific_data* bsp_data;
+ rtems_status_code sc = RTEMS_SUCCESSFUL;
+ uint32_t bank;
+ uint32_t pin;
+
+ if ( pin_number < 0 || pin_number >= gpio_count ) {
+ return RTEMS_INVALID_ID;
+ }
+
+ bank = BANK_NUMBER(pin_number);
+ pin = PIN_NUMBER(pin_number);
+
+ AQUIRE_LOCK(bank_lock[bank]);
+
+ /* If the pin is already being used returns with an error. */
+ if ( gpio_pin_state[bank][pin].pin_function != NOT_USED ) {
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return RTEMS_RESOURCE_IN_USE;
+ }
+
+ switch ( function ) {
+ case DIGITAL_INPUT:
+ sc = rtems_bsp_gpio_select_input(bank, pin, bsp_specific);
+ break;
+ case DIGITAL_OUTPUT:
+ sc = rtems_bsp_gpio_select_output(bank, pin, bsp_specific);
+ break;
+ case BSP_SPECIFIC:
+ bsp_data = (rtems_gpio_specific_data*) bsp_specific;
+
+ if ( bsp_data == NULL ) {
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return RTEMS_UNSATISFIED;
+ }
+
+ sc = rtems_bsp_select_specific_io(bank, pin,
bsp_data->io_function, bsp_data->pin_data);
+ break;
+ case NOT_USED:
+ default:
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return RTEMS_NOT_DEFINED;
+ }
+
+ if ( sc != RTEMS_SUCCESSFUL ) {
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return sc;
+ }
+
+ /* If the function was successfuly assigned to the pin,
+ * record that information on the gpio_pin_state structure. */
+ gpio_pin_state[bank][pin].pin_function = function;
+ gpio_pin_state[bank][pin].logic_invert = logic_invert;
+
+ if ( function == DIGITAL_OUTPUT ) {
+ if ( output_enabled == true ) {
+ sc = rtems_bsp_gpio_set(bank, pin);
Shouldn't this also check logic_invert?
If the application indicates that this pin is to be enabled upon request, it
should be enabled (set) regardless of the behavior of the next set/clears on
that pin (which may be inverted, depending on the logic_invert flag). That
is how I see it.
I don't know. It's a bit funny. If the pin is low-out
(logic_invert==true), then output_enabled means you want it pulled
low, no?
Since the API does not have platform code, if output_enabled is true
then it will always call gpio_set to have it logical high. If that
platform uses inverted logic then their implementation will do that.
This logic_invert would be a flag to switch the set and clear calls (a
set would become a clear, and vice-versa).
+rtems_status_code rtems_gpio_interrupt_handler_install(
+uint32_t pin_number,
+rtems_gpio_irq_state (*handler) (void *arg),
+void *arg
+)
+{
+ gpio_handler_list *isr_node;
+ gpio_pin* gpio;
+ uint32_t bank;
+ uint32_t pin;
+
+ if ( pin_number < 0 || pin_number >= gpio_count ) {
+ return RTEMS_INVALID_ID;
+ }
+
+ bank = BANK_NUMBER(pin_number);
+ pin = PIN_NUMBER(pin_number);
+
+ AQUIRE_LOCK(bank_lock[bank]);
+
+ gpio = &gpio_pin_state[bank][pin];
+
+ /* If the current pin has no interrupt enabled
+ * then it does not need an handler. */
+ if ( gpio->enabled_interrupt == NONE ) {
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return RTEMS_NOT_CONFIGURED;
+ }
+ /* If the pin already has an enabled interrupt but the installed
handler
+ * is set as unique. */
+ else if ( gpio->handler_flag == UNIQUE_HANDLER && gpio->handler_list
!= NULL ) {
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return RTEMS_RESOURCE_IN_USE;
+ }
+
+ /* Update the pin's ISR list. */
+ isr_node = (gpio_handler_list *) malloc(sizeof(gpio_handler_list));
+
+ if ( isr_node == NULL ) {
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return RTEMS_NO_MEMORY;
+ }
+
+ isr_node->handler = handler;
+ isr_node->arg = arg;
+
+ if ( gpio->handler_flag == SHARED_HANDLER ) {
+ isr_node->next_isr = gpio->handler_list;
+ }
+ else {
+ isr_node->next_isr = NULL;
Isn't this the same thing as gpio->handler_list in case of
!SHARED_HANDLER?
Indeed the gpio->handler_list is always NULL if empty, so yes this if block
can be replaced with
isr_node->next_isr = gpio->handler_list;
Not sure if that would be more readable.
Realistically you should have helper functions for manipulating these
lists, which would make the code eminently more readable. Or use RTEMS
chains.
Yes, rtems chains should be better.
+rtems_status_code rtems_gpio_disable_interrupt(uint32_t pin_number)
+{
+ gpio_handler_list *isr_node;
+ rtems_vector_number vector;
+ rtems_status_code sc;
+ gpio_pin* gpio;
+ uint32_t bank;
+ uint32_t pin;
+
+ if ( pin_number < 0 || pin_number >= gpio_count ) {
+ return RTEMS_INVALID_ID;
+ }
+
+ bank = BANK_NUMBER(pin_number);
+ pin = PIN_NUMBER(pin_number);
+
+ vector = rtems_bsp_gpio_get_vector(bank);
+
+ AQUIRE_LOCK(bank_lock[bank]);
+
+ gpio = &gpio_pin_state[bank][pin];
+
+ if ( interrupt_counter[bank] == 0 || gpio->enabled_interrupt == NONE )
{
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return RTEMS_SUCCESSFUL;
+ }
+
+ sc = rtems_bsp_disable_interrupt(bank, pin, gpio->enabled_interrupt);
+
+ if ( sc != RTEMS_SUCCESSFUL ) {
+ RELEASE_LOCK(bank_lock[bank]);
+
+ return RTEMS_UNSATISFIED;
+ }
+
+ gpio->enabled_interrupt = NONE;
+
+ while ( gpio->handler_list != NULL ) {
+ isr_node = gpio->handler_list;
+
+ gpio->handler_list = isr_node->next_isr;
+
+ free(isr_node);
+ }
+
+ sc = rtems_task_delete(gpio->task_id);
So the handler task cannot be shared by more than one pin? Is this
requirement explicit somewhere? Is this the intent?
The handler task is to handle the (possible) many handlers an individual pin
can have, if it acts as a shared IRQ line. What is shared across multiple
pins is the generic_isr, which is as actual ISR routine (shared with all
pins of a given bank).
I need to understand this handler task and generic_isr framework better.
Each bank has a generic_isr, while each pin (with an active interrupt)
has an handler_task. Every time an interrupt is sensed on a bank,
generic_isr checks which pins from that bank have pending interrupts,
wakes the corresponding pin handler task, clears the interrupt line on
that bank and finishes. The actual action fired by the interrupt is done
by the handler_task, outside of ISR context.
+
+/**
+ * @brief Object containing relevant information for assigning a BSP
specific
+ * function to a pin.
+ *
+ * Encapsulates relevant data for a BSP specific GPIO function.
+ */
+typedef struct
+{
+ /* The bsp defined function code. */
+ uint32_t io_function;
+
+ void* pin_data;
+} rtems_gpio_specific_data;
+
+/**
+ * @brief Object containing relevant information about a BSP's GPIO
layout.
+ */
+typedef struct
+{
+ /* Total number of GPIO pins. */
+ uint32_t pin_count;
+
+ /* Number of pins per bank. The last bank may be smaller,
+ * depending on the total number of pins. */
+ uint32_t pins_per_bank;
Maybe use an array instead. This can also reduce the need for all the
arithmetic computing bank and pin from pin_number.. if you can just
index directly into a struct containing all that information? What
would be the expected cost in size to do it like this?
You mean to replace the rtems_gpio_layout with an array of structs
statically filled with the bank/pin combinations by each BSP? That does not
seem very clean, specially since each all banks have the same number of pins
(the last is the only possible exception), and the layout calculation is
pretty easy/cheap.
One way to improve the bank/pin situation would, maybe, be as follows:
Replace the gpio_pin **gpio_pin_state matrix with an uni-dimensional array,
such that (suposing pins_per_bank = 32) from the index 0 to 31 it would
refer to pins in the first bank, from index 32 to 63 to pins from the second
bank, and so on. This would allow to fetch the right GPIO pin using the
pin_number directly.
For the cases where I need to send a whole bank of pins, I could simply send
a pointer to the index of the first pin of that bank. Then since I know how
many pins each bank has I always know where the bank ends.
That might make sense. I'm just trying to think through the useful
options here. Before we get stuck with something in the API layer that
is inefficient to use.
Gedare
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
