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: Code-Review-1
(7 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@a336 PS1, Line 336: ret = realtek_mst_i2c_spi_disable_protection(fd); : if (ret) : return ret; This change should just generalise this functions implementation and not too much else. A second follow up patch then leverages those generalisations.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@149 PS1, Line 149: disable this function shouldn't be just _disable_, it should toggle and be parametric with a bool to toggle the bit mask '0x01' as seen below.
`static int realtek_mst_i2c_spi_toggle_gpioXX_strap(int fd, bool toggle)` where XX denotes the gpio #, or if you can indentify it below perhaps have that as a param as well.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@154 PS1, Line 154: // Configure Pin to Push-Pull GPIO `/* 0x4F[2:0] = b001 */`
what do you mean push-pull? avoid these C++ style comments btw as it is inconsistent.
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
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.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@178 PS1, Line 178: 0xFE 0xF0 0xFE
there is a pattern here.
https://review.coreboot.org/c/flashrom/+/46089/1/realtek_mst_i2c_spi.c@495 PS1, Line 495: if (wp_disable) { : ret |= realtek_mst_i2c_spi_disable_hw_protection(fd); : if (ret != 0) { : msg_perr("%s: failed to disable the write protection.\n", __func__); : return ret; : } : } : I don't think wp should be toggled via this mechanism by using a bespoke spi master parameter. Rather it should be via the --wp-[en|dis]able parameter. Just as I mentioned before, perhaps we could catch the request in the spi_send_command call back and dispatch to the local implementation detail internally here.