[PATCH 1/2] RTEMS GPIO API definition and implementation.

[Gedare Bloom]

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


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

>
> 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?


>>> +
>>> +  /* 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?

>
>>> +    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?


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

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

>>> +
>>> +/**
>>> + * @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