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

[Nico Huber (Code Review)]

Attention is currently required from: Maxim Polyakov, Angel Pons.
Nico Huber 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+2

(5 comments)

Patchset:

PS12: 
Angel, I marked some of your comments as resolved, hope you don't mind.

One remark: Sometimes it's called `pin` sometimes `pad` that's a bit
irritating (however I only noticed after going through the earlier
comments).

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

https://review.coreboot.org/c/coreboot/+/47595/comment/5b5e2427_63f64233 
PS5, Line 38: gpio_pad
> I would prefer gpio in the option name (array), if you don't mind?

SGTM.

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

https://review.coreboot.org/c/coreboot/+/47595/comment/6e520f59_77213350 
PS5, Line 16: offset
> I would prefer pin_num as the variable name.

LGTM.

https://review.coreboot.org/c/coreboot/+/47595/comment/5eb0e2ef_302c3dd1 
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;
            : }
> Thanks for your time on this work, but I would like to separate operations and use functions, if you […]

New code looks good to me.

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

https://review.coreboot.org/c/coreboot/+/47595/comment/cd8ff1c9_a883b1e7 
PS12, Line 35: const 
Did I write this? It's not meaningful in a forward declaration.