[coreboot-gerrit] Change in coreboot[master]: ec/kontron/kempld: Add minimal GPIO driver

[Angel Pons (Code Review)]

Attention is currently required from: Maxim Polyakov.
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 12: Code-Review+1

(4 comments)

File src/ec/kontron/kempld/chip.h:

https://review.coreboot.org/c/coreboot/+/47595/comment/ae0eaf1b_bc4558f5 
PS5, Line 38: gpio_pad
> SGTM.

Ack

File src/ec/kontron/kempld/kempld_gpio.c:

https://review.coreboot.org/c/coreboot/+/47595/comment/ee016130_ef24ae3c 
PS5, Line 16: offset
> LGTM.

Ack

https://review.coreboot.org/c/coreboot/+/47595/comment/e3972214_b0d3b827 
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;
            : }
> New code looks good to me.

Looks good to me as well. While my approach skips acquiring the kempld mutex for `KEMPLD_GPIO_DEFAULT` (a micro-optimisation), your solution is much more elegant and easier to understand. And I prefer easy-to-maintain code over code that's sliiiiightly faster but considerably messier.

File src/ec/kontron/kempld/kempld_internal.h:

https://review.coreboot.org/c/coreboot/+/47595/comment/e05b2232_c01c1597 
PS12, Line 31: #define KEMPLD_GPIO_MASK(x)		(1 << ((x) % 8))
             : #define KEMPLD_GPIO_DIR(pad_num)	(0x40 + (pad_num) / 8)
             : #define KEMPLD_GPIO_LVL(pad_num)	(0x42 + (pad_num) / 8)
nit: Use the same name for the macro parameters, e.g. `pin_num`?