Attention is currently required from: Angel Pons. Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47595 )
Change subject: ec/kontron/kempld: Add minimal GPIO driver ......................................................................
Patch Set 8:
(12 comments)
This change is ready for review.
Commit Message:
https://review.coreboot.org/c/coreboot/+/47595/comment/91bab639_c3566075 PS5, Line 11: kempel/kempeld
typos? `kempld/kempld`
It was removed from the message
https://review.coreboot.org/c/coreboot/+/47595/comment/ef8b9753_c46d805b PS5, Line 12: configure/manage each pad using the API
Are there plans to use this API?
Yes, but I decided to do it in a separate patch.
Sometimes carrier board manufacturers need to read the ID, and the easiest way is to use GPIO. In this case, the COMe board uses only GPIO from CPLD.
File src/ec/kontron/kempld/chip.h:
https://review.coreboot.org/c/coreboot/+/47595/comment/50235183_87d98a6d PS5, Line 7: KEMPLD_NUM_GPIO_PADS
Why not `KEMPLD_NUM_GPIOS`?
KEMPLD_NUM_GPIOS Done
Also, I have specified the number of pads - 16. But on my board, I can only check the 8 that are used in the connectors of the COMe module.
https://review.coreboot.org/c/coreboot/+/47595/comment/8357328a_3adb137e 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? […]
Ok, done. Thanks
https://review.coreboot.org/c/coreboot/+/47595/comment/d5d49011_9065ff07 PS5, Line 38: gpio_pad
I'd call this `gpios`. Also, the type should be `enum kempld_gpio_mode`.
I would prefer gpio in the option name (array), if you don't mind?
File src/ec/kontron/kempld/kempld_gpio.c:
https://review.coreboot.org/c/coreboot/+/47595/comment/d7b9381f_14017866 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?
Yes, from the kernel. In the beginning I tried to keep this code unchanged (the following comments are related to this reason). After the fixes, this is no longer required.
https://review.coreboot.org/c/coreboot/+/47595/comment/d2fe048d_38a19d28 PS5, Line 16: offset
Why is this called `offset`? Unless I'm missing something, it's just the GPIO index?
I would prefer pin_num as the variable name.
https://review.coreboot.org/c/coreboot/+/47595/comment/f9e02f8e_4d1630c9 PS5, Line 18: int status
Why is this an int (should be u8)? Why is it called `status`?
Reworked.
https://review.coreboot.org/c/coreboot/+/47595/comment/90471879_e71918fe PS5, Line 23: (u8)
Are these casts needed? I guess they might be because `offset` is an `unsigned int`.
Reworked. At the moment, this is not needed, since the variables already have the type u8.
https://review.coreboot.org/c/coreboot/+/47595/comment/30730d6d_691d43e6 PS5, Line 33: int status
Why is this an int (should be u8)? Why is it called `status`?
Reworked
https://review.coreboot.org/c/coreboot/+/47595/comment/ec4bd459_254fa7b0 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: […]
Thanks for your time on this work, but I would like to separate operations and use functions, if you don't mind? Perhaps this is only the opinion of the author, but in this case, the code will be easier to read if it is divided into functions )
https://review.coreboot.org/c/coreboot/+/47595/comment/3b8a9650_105f9ff8 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: […]
Done. Thanks for your help on this.