Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update wp disable function and add param ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@165 PS1, Line 165: 0xF0
these magics getting changed, looks like they should have a #define
Specifically, 0xF8 -> 0xF0 seems suspect to change that mask, why are we touching the high bit?
You deleted the above comment but are you sure you understood it? `// 0xAB[2:0] = b001` if I draw it out here it maybe more clear:
``` 0xF8 - 1111 1 000 0xF0 - 1111 0 000 ```
this would mean we are overwriting 0x4F[3:0] worth of `val` and not 0x4F[2:0], is that intended and if so why?
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@167 PS1, Line 167: // Set Pin Value to High
the comment was fine and very informative before, leave it.
`/* Toggle pin value, 0xFFF5[0] = |toggle|. */`
and move the comment above the actual masking below.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@176 PS1, Line 176: 0x3F `#define GPIO_CONF 0x3F` `#define GPIO_DIRECTION 0x4F`
or something to that effect. Probably the pin number is encoded somewhere here that you can identify to fully see the whole picture that is happening here?
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@178 PS1, Line 178: 0xFE
0xF0 […]
`0xFE - 1111 111 0`
in other words "keep the rest of val the same and just mask off whatever was in the low bit and or the 0 with 0x01 to toggle it".