Attention is currently required from: Felix Singer, Nico Huber, Maxim Polyakov, Werner Zeh. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 5:
(16 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/47595/comment/9b888f68_57f5494f PS5, Line 11: kempel/kempeld typos? `kempld/kempld`
https://review.coreboot.org/c/coreboot/+/47595/comment/c32cf311_b621ce9b PS5, Line 12: configure/manage each pad using the API Are there plans to use this API?
File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/2df06114_f525a113 PS5, Line 7: KEMPLD_NUM_GPIO_PADS Why not `KEMPLD_NUM_GPIOS`?
https://review.coreboot.org/c/coreboot/+/47595/comment/89fbd0cb_36820d73 PS5, Line 9: #define KEMPLD_PAD_GPIO_DIR_IN 0 : #define KEMPLD_PAD_GPIO_DIR_OUT 1 : #define KEMPLD_PAD_GPIO_CFG_DIR(in_out) ((KEMPLD_PAD_GPIO_DIR_##in_out) << 7) : #define KEMPLD_PAD_GPIO_CFG_VAL(v) (v) : #define KEMPLD_PAD_GPIO_CFG_IN KEMPLD_PAD_GPIO_CFG_DIR(IN) : #define KEMPLD_PAD_GPIO_CFG_OUT(v) \ : (KEMPLD_PAD_GPIO_CFG_DIR(OUT) | KEMPLD_PAD_GPIO_CFG_VAL(v)) Do we really need to use macros for this? Why not go with enums, like the rest of the code does?
enum kempld_gpio_mode { KEMPLD_GPIO_DEFAULT, KEMPLD_GPIO_INPUT, KEMPLD_GPIO_OUTPUT_LOW, KEMPLD_GPIO_OUTPUT_HIGH, };
The `KEMPLD_GPIO_DEFAULT` value allows skipping config for unspecified GPIOs, which preserves current behavior.
https://review.coreboot.org/c/coreboot/+/47595/comment/e6a79fdf_91a0fd11 PS5, Line 38: gpio_pad I'd call this `gpios`. Also, the type should be `enum kempld_gpio_mode`.
File src/ec/kontron/kempld/kempld.c:
https://review.coreboot.org/c/coreboot/+/47595/comment/015fe73b_c8d95b76 PS5, Line 99: GPIOs configuration nit: either `GPIO configuration` or `Configuring GPIOs`. I prefer the former.
https://review.coreboot.org/c/coreboot/+/47595/comment/0a3d3fa7_841e5f77 PS5, Line 100: } else, warn? it would be consistent with the code above:
} else { printk(BIOS_WARNING, "KEMPLD: Spurious GPIO device %s.\n", dev_path(dev)); }
File src/ec/kontron/kempld/kempld_gpio.c:
https://review.coreboot.org/c/coreboot/+/47595/comment/2681c9d1_717c1b08 PS5, Line 6: * Author: Michael Brunner michael.brunner@kontron.com Is this comment because the code was taken from the Linux kernel? Do we need it?
https://review.coreboot.org/c/coreboot/+/47595/comment/4b8629ed_b0f424ff PS5, Line 16: offset Why is this called `offset`? Unless I'm missing something, it's just the GPIO index?
https://review.coreboot.org/c/coreboot/+/47595/comment/2404e3b8_f87450b0 PS5, Line 18: int status Why is this an int (should be u8)? Why is it called `status`?
https://review.coreboot.org/c/coreboot/+/47595/comment/98fd12fc_4092af41 PS5, Line 23: (u8) Are these casts needed? I guess they might be because `offset` is an `unsigned int`.
https://review.coreboot.org/c/coreboot/+/47595/comment/1f4a7d17_666913af PS5, Line 33: int status Why is this an int (should be u8)? Why is it called `status`?
https://review.coreboot.org/c/coreboot/+/47595/comment/2504c7de_b5f720fd PS5, Line 49: int kempld_gpio_direction_input(unsigned int offset) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : } : : int kempld_gpio_direction_output(unsigned int offset, int value) : { : int status; : : if (kempld_get_mutex(100) < 0) : return -1; : : status = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(offset)); : if (value) : status |= KEMPLD_GPIO_MASK(offset); : else : status &= ~KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(offset), (u8)status); : : status = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(offset)); : status |= KEMPLD_GPIO_MASK(offset); : kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(offset), (u8)status); : : kempld_release_mutex(); : return 0; : } This would need to be adapted to use the enum type I suggested in another comment:
enum kempld_gpio_mode { KEMPLD_GPIO_DEFAULT, KEMPLD_GPIO_INPUT, KEMPLD_GPIO_OUTPUT_LOW, KEMPLD_GPIO_OUTPUT_HIGH, };
With the enum, one function is enough:
int kempld_configure_gpio(unsigned int gpio, enum kempld_gpio_mode mode) { const u8 mask = KEMPLD_GPIO_MASK(gpio); u8 dir, lvl, val;
switch (mode) { case KEMPLD_GPIO_DEFAULT: return 0; case KEMPLD_GPIO_INPUT: dir = 0; lvl = 0; break; case KEMPLD_GPIO_OUTPUT_LOW: dir = mask; lvl = 0; break; case KEMPLD_GPIO_OUTPUT_HIGH: dir = mask; lvl = mask; break; default: return -1; } if (kempld_get_mutex(100) < 0) return -1;
/* For outputs, set level before direction to avoid glitches */ if (dir) { val = kempld_read8((u8)KEMPLD_GPIO_LVL_NUM(gpio)); val &= ~mask; val |= lvl; kempld_write8((u8)KEMPLD_GPIO_LVL_NUM(gpio), val); }
val = kempld_read8((u8)KEMPLD_GPIO_DIR_NUM(gpio)); val &= ~mask; val |= dir; kempld_write8((u8)KEMPLD_GPIO_DIR_NUM(gpio), val);
kempld_release_mutex(); return 0; }
I know the switch-block is rather verbose. I did it on purpose, I feel it's clearer this way.
https://review.coreboot.org/c/coreboot/+/47595/comment/1f9ee1f1_d5ef6881 PS5, Line 86: int kempld_gpio_pad_config(struct device *const dev) : { : const struct ec_kontron_kempld_config *const config = dev->chip_info; : int i, status; : : if (!config) : return -1; : : for (i = 0; i < KEMPLD_NUM_GPIO_PADS; ++i) { : if (config->gpio_pad[i] & KEMPLD_PAD_GPIO_CFG_DIR(OUT)) : status = kempld_gpio_direction_output(i, config->gpio_pad[i] & 0x1); : else : status = kempld_gpio_direction_input(i); : : if (status < 0) : return status; : } : return 0; : } With the changes suggested above, I'd rewrite this as follows:
int kempld_gpio_pad_config(struct device *const dev) { const struct ec_kontron_kempld_config *const config = dev->chip_info; if (!config) return -1;
for (unsigned int i = 0; i < KEMPLD_NUM_GPIO_PADS; ++i) { if (kempld_configure_gpio(i, config->gpios[i]) < 0) return -1; } return 0; }
File src/ec/kontron/kempld/kempld_internal.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/15acf943_0770c581 PS5, Line 33: 8 Given that `KEMPLD_NUM_GPIO_PADS` is 8, the parametrization seems pointless. Or do other kempld's support more than 8 GPIOs? If so, I wouldn't define two macros for each register:
#define KEMPLD_GPIO_DIR(gpio) (0x40 + (gpio) / 8) #define KEMPLD_GPIO_LVL(gpio) (0x42 + (gpio) / 8)
https://review.coreboot.org/c/coreboot/+/47595/comment/628e83dc_47a06a0f PS5, Line 33: / nit: spaces around `/`