[XL] Change in coreboot[main]: ec/dasharo/ec: add Dasharo features

[Krystian Hebel (Code Review)]

Attention is currently required from: Maciej Pijanowski, Michał Kopeć, Michał Żygowski.

Krystian Hebel has posted comments on this change by Michał Kopeć. ( https://review.coreboot.org/c/coreboot/+/82672?usp=email )

Change subject: ec/dasharo/ec: add Dasharo features
......................................................................

Patch Set 10:

(14 comments)

File src/ec/dasharo/ec/acpi/dasharo.asl:

https://review.coreboot.org/c/coreboot/+/82672/comment/ac1476a7_c2648a4d?usp... :
PS10, Line 3: // Notifications:
            : //   0x80 - hardware backlight toggle
            : //   0x81 - backlight toggle
            : //   0x82 - backlight down
            : //   0x83 - backlight up
            : //   0x84 - backlight color change
            : //   0x85 - OLED screen toggle
These have been removed from the code, should be removed from here as well.

https://review.coreboot.org/c/coreboot/+/82672/comment/e1d6e70d_a74e4e42?usp... :
PS10, Line 11: 	Name (_HID, "XXXX0000") // TODO: Update after we get a valid ACPI HID prefix
Don't we have one already? https://uefi.org/ACPI_ID_List?acpi_search=DSHR

https://review.coreboot.org/c/coreboot/+/82672/comment/ee89fe7c_60d53fe2?usp... :
PS10, Line 17: 		Debug = "DASH: RSET"
Here `Debug` is directly written to, in other cases `Printf` is used, even for simple strings. Please be consistent.

File src/ec/dasharo/ec/acpi/ec.asl:

https://review.coreboot.org/c/coreboot/+/82672/comment/8fd14015_21a9c0a9?usp... :
PS10, Line 95: 			^^^^DASH.RSET()
Won't it unconditionally turn off airplane mode LED on wake?

File src/ec/dasharo/ec/dasharo_ec.c:

https://review.coreboot.org/c/coreboot/+/82672/comment/acaf2879_4d16719f?usp... :
PS10, Line 17: #define DASHARO_EC_BASE 0x0E00
Is this reserved from resource allocator?

https://review.coreboot.org/c/coreboot/+/82672/comment/8bbc6d6f_46954167?usp... :
PS10, Line 43: 2
REG_DATA

https://review.coreboot.org/c/coreboot/+/82672/comment/6183a401_65405728?usp... :
PS10, Line 52: REG_CMD + 2
REG_DATA

https://review.coreboot.org/c/coreboot/+/82672/comment/56ba3317_a27b02b8?usp... :
PS10, Line 88: index should be valid due to length test above
I don't see such test. In fact, I believe that this code would overwrite caller's stack, if the data from EC is long enough.

Also, the caller expects to get 0-terminated string, but this code doesn't enforce it, which may cause another overflow.

https://review.coreboot.org/c/coreboot/+/82672/comment/6953e7f6_7fc0194f?usp... :
PS10, Line 122: 	// Read data bytes, index should be valid due to length test above
Same as `dasharo_ec_read_version`.

https://review.coreboot.org/c/coreboot/+/82672/comment/a65ac64a_9149a90e?usp... :
PS10, Line 200: #define SPI_ACCESS_SIZE 252
IIUC this is `(DASHARO_EC_SIZE - REG_DATA - ARRAY_SIZE(read_cmd))` (the last operand defined below, it would be `ARRAY_SIZE(write_cmd)` for writes), perhaps it would be better to write it this way.

https://review.coreboot.org/c/coreboot/+/82672/comment/79c41f85_abf3e5d9?usp... :
PS10, Line 292: 	if ((rv = ec_spi_reset()))
I don't know how this EC works, but I assume that this reset doesn't clear SPI status, right?

https://review.coreboot.org/c/coreboot/+/82672/comment/86505016_df555fc8?usp... :
PS10, Line 323: 	return 0;
This and next function return 0 also when `status < 0`, is this the expected behavior?

https://review.coreboot.org/c/coreboot/+/82672/comment/56cb4c17_4295fee2?usp... :
PS10, Line 553: dasharo_ec_read_board((uint8_t *)cur_board_str);
              : 	dasharo_ec_read_version((uint8_t *)cur_version_str);
Return values are not checked, which makes `cur_*_str` potentially uninitialized.

https://review.coreboot.org/c/coreboot/+/82672/comment/d93a0d54_cb00a212?usp... :
PS10, Line 570: 	if ((ec_read(0x10) & 0x01) != 0x01) {
Magic numbers, please at least add a comment about their meaning.