[L] Change in coreboot[main]: superio/ite/common: Add common driver for GPIO and LED configuration

[Nico Huber (Code Review)]

Attention is currently required from: Michał Żygowski.

Nico Huber has posted comments on this change by Michał Żygowski. ( https://review.coreboot.org/c/coreboot/+/83355?usp=email )

Change subject: superio/ite/common: Add common driver for GPIO and LED configuration
......................................................................

Patch Set 1: Code-Review+1

(9 comments)

Patchset:

PS1: 
Thanks, I know it's a lot of work, much appreciated!

I don't have all the datasheets available, in particular nothing with
GPIO sets > 8 or the 5-bit frequency selection. Otherwise, it looks
pretty good, given the number of datasheets and oddities.

Commit Message:

https://review.coreboot.org/c/coreboot/+/83355/comment/0bcd8a85_5d7aa388?usp... :
PS1, Line 22: - Refactors the mainboards' code to use the new driver where possible
When a patch has hundreds of lines and a list of things it does
that's usually a sign that it could be done easier in smaller
patches. In this case the last two points would have to be in a
single commit, but the others don't.

I'm fine reviewing this one at once, but please keep an eye on
such cases in the future. There's also a chance to get important
parts merged more quickly. For instance, updating the IT8772F
code is a very nice bonus. OTOH, should reviewing if that is
regression free hold up a new board port?

File src/mainboard/google/beltino/variants/mccloud/led.c:

https://review.coreboot.org/c/coreboot/+/83355/comment/f3f443f1_89d4c11e?usp... :
PS1, Line 18: ITE_LED_SHORT_PULSE_DISABLE,
            : 		ITE_LED_PINMAP_CLEAR_DISABLE,
            : 		ITE_LED_OUTPUT_LOW_DISABLE,
            : 		ITE_LED_FREQ_1HZ);
Random thought: Making the enum values masks (i.e. with the 1 shifted already)
would allow a single parameter. A caller that wants to set more of the bits
wouldn't look much different, e.g.
```
ite_gpio_setup_led(IT8772F_GPIO_DEV, 10, ITE_GPIO_LED_1,
    ITE_LED_SHORT_PULSE |
    ITE_OUTPUT_LOW |
    ITE_LED_FREQ_1HZ);
```
And callers that don't want to enable a feature can omit it, i.e. this call
here could just be
```
ite_gpio_setup_led(IT8772F_GPIO_DEV, 10, ITE_GPIO_LED_1, ITE_LED_FREQ_1HZ);
```
I think it might also reduce some of the boilerplate in the implementation.
And things like the `do we have pinmap-clear?' could be implemented by masking
the respective bit.

File src/mainboard/samsung/stumpy/smihandler.c:

https://review.coreboot.org/c/coreboot/+/83355/comment/01ec0b47_f5b36cd5?usp... :
PS1, Line 25: 		ite_reg_write(GPIO_DEV, IT8772F_GPIO_REG_SELECT(3), 0x20);
Now that these are seperate steps, the redundancy becomes obvious: We already
set this during boot. So we could drop these mux settings here.

https://review.coreboot.org/c/coreboot/+/83355/comment/493eafa2_0d0f7bee?usp... :
PS1, Line 39: 		ite_reg_write(GPIO_DEV, IT8772F_GPIO_REG_SELECT(3), 0x20);
Same.

https://review.coreboot.org/c/coreboot/+/83355/comment/dd103b1d_3567b53f?usp... :
PS1, Line 49: 			ITE_LED_FREQ_1HZ);
This looks very odd now, configuring blink when it's not used? Traced
this to CB:12797. We could just drop this call.

File src/superio/ite/common/gpio.c:

https://review.coreboot.org/c/coreboot/+/83355/comment/6a5be451_d985c222?usp... :
PS1, Line 130: 	 */
It looks like IT8718F doesn't have pull-up set 2?
Same for IT8720F, plus sets 7, 8?
IT8783EF not set 6?

File src/superio/ite/it8772f/it8772f.h:

https://review.coreboot.org/c/coreboot/+/83355/comment/3c303899_49dc5dd0?usp... :
PS1, Line 16: 
            : /* GPIO Polarity Select: 1: Inverting, 0: Non-inverting */
            : #define IT8772F_GPIO_REG_POLARITY(x) (0xb0 + (x))
            : 
            : /* GPIO Internal Pull-up: 1: Enable, 0: Disable */
            : #define IT8772F_GPIO_REG_PULLUP(x)   (0xb8 + (x))
            : 
            : /* GPIO Function Select: 1: Simple I/O, 0: Alternate function */
            : #define IT8772F_GPIO_REG_ENABLE(x)   (0xc0 + (x))
            : 
            : /* GPIO Mode: 0: input mode, 1: output mode */
            : #define IT8772F_GPIO_REG_OUTPUT(x)   (0xc8 + (x))
            : 
Why keep them?

File src/superio/ite/it8786e/Kconfig:

https://review.coreboot.org/c/coreboot/+/83355/comment/27bab9c1_0368f9c7?usp... :
PS1, Line 16: 	default 10
I only see 8?