[GitHub] [incubator-nuttx] robertlipe commented on issue #5810: GPIO issues on BL602 + suggestions for RISC-V, maybe more.

[GitBox <gi...@apache.org>]

robertlipe commented on issue #5810:
URL: https://github.com/apache/incubator-nuttx/issues/5810#issuecomment-1098665977

   I'm a fan of arguments being 'real' data types instead of everything being
   ints (and remember that passing chars *often* means passing an int anyway)
   for everything. That lets the compiler enforce the contract between the
   parts, making this whole class of problems more obvious at compile time.
   
   Does bl602_expander_intmask() need to inhibit interrupts/context switches
   between the read of BL602_GPIO_INT_MASK1 and the store to BL602_GPIO_INT_MASK1?
   That may suggest performing the shift outside the block just to shorten
   that window by a few opcodes. Maybe a   irqstate_t flags =
   spin_lock_irqsave(NULL);
   ... do the thing
     spin_unlock_irqrestore(NULL, flags);
   
   Actually, it looks like there's a LOT of that protection missing in the
   BL602 code. If you agree that's sane, please create a PR and assign it to
   me and I'll pencil-whip the code and make a pass at improving that.
   
   
   On Wed, Apr 13, 2022 at 8:55 PM Lee Lup Yuen ***@***.***>
   wrote:
   
   > Documenting a bug in BL602 EVB GPIO: This call to bl602_gpio_set_intmod
   > is incorrect: bl602evb/bl602_gpio.c
   > <https://github.com/apache/incubator-nuttx/blob/master/boards/risc-v/bl602/bl602evb/src/bl602_gpio.c#L597-L598>
   >
   > int bl602_gpio_initialize(void) {
   >   ...
   >   bl602_gpio_set_intmod(g_gpiointinputs[i], 1, GLB_GPIO_INT_TRIG_NEG_PULSE);
   >
   > This passes a pinset (uint32_t) to bl602_gpio_set_intmod
   > <https://github.com/apache/incubator-nuttx/blob/master/boards/risc-v/bl602/bl602evb/src/bl602_gpio.c#L179-L180>.
   > But bl602_gpio_set_intmod
   > <https://github.com/apache/incubator-nuttx/blob/master/boards/risc-v/bl602/bl602evb/src/bl602_gpio.c#L179-L180>
   > accepts a GPIO Pin Number gpio_pin (uint8_t), not a pinset (uint32_t).
   >
   > We need to convert the pinset to gpio_pin like so:
   >
   >   uint8_t gpio_pin = (g_gpiointinputs[i] & GPIO_PIN_MASK) >> GPIO_PIN_SHIFT;
   >   bl602_gpio_set_intmod(gpio_pin, 1, GLB_GPIO_INT_TRIG_NEG_PULSE);
   >
   > To catch such issues, we might implement Assertion Checks on GPIO Numbers
   > in these functions:
   >
   >    - bl602_gpio_intmask
   >    <https://github.com/apache/incubator-nuttx/blob/master/boards/risc-v/bl602/bl602evb/src/bl602_gpio.c#L143-L169>
   >    - bl602_gpio_set_intmod
   >    <https://github.com/apache/incubator-nuttx/blob/master/boards/risc-v/bl602/bl602evb/src/bl602_gpio.c#L171-L211>
   >    - bl602_gpio_get_intstatus
   >    <https://github.com/apache/incubator-nuttx/blob/master/boards/risc-v/bl602/bl602evb/src/bl602_gpio.c#L213-L233>
   >    - bl602_gpio_intclear
   >    <https://github.com/apache/incubator-nuttx/blob/master/boards/risc-v/bl602/bl602evb/src/bl602_gpio.c#L235-L253>
   >
   > Here are the Assertion Checks that I'm testing:
   >
   >    - bl602_gpio_intmask
   >    <https://github.com/lupyuen/cst816s-nuttx/blob/0a517f6bbe5e393a095da3748c6c5cf18a1709c6/cst816s.c#L1259-L1291>
   >    - bl602_gpio_set_intmod
   >    <https://github.com/lupyuen/cst816s-nuttx/blob/0a517f6bbe5e393a095da3748c6c5cf18a1709c6/cst816s.c#L1293-L1340>
   >    - bl602_gpio_get_intstatus
   >    <https://github.com/lupyuen/cst816s-nuttx/blob/0a517f6bbe5e393a095da3748c6c5cf18a1709c6/cst816s.c#L1342-L1368>
   >    - bl602_gpio_intclear
   >    <https://github.com/lupyuen/cst816s-nuttx/blob/0a517f6bbe5e393a095da3748c6c5cf18a1709c6/cst816s.c#L1370-L1394>
   >
   > GPIO Pin Numbers like 28 have been changed to GPIO_PIN28 for consistency.
   >
   > Also note that bl602_gpio_intmask
   > <https://github.com/apache/incubator-nuttx/blob/master/boards/risc-v/bl602/bl602evb/src/bl602_gpio.c#L143-L169>
   > should accept a gpio_pin (uint8_t), not uint32_t.
   >
   > Here's the fix
   > <https://github.com/lupyuen/cst816s-nuttx/blob/0a517f6bbe5e393a095da3748c6c5cf18a1709c6/cst816s.c#L1259-L1291>
   >
   > —
   > Reply to this email directly, view it on GitHub
   > <https://github.com/apache/incubator-nuttx/issues/5810#issuecomment-1098633538>,
   > or unsubscribe
   > <https://github.com/notifications/unsubscribe-auth/ACCSD336EXEHUKM5SOIQOPLVE53KLANCNFSM5RGMP4IQ>
   > .
   > You are receiving this because you authored the thread.Message ID:
   > ***@***.***>
   >
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org