Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46089 )
Change subject: realtek_mst_i2c_spi.c: Update write protection toggle function ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c File realtek_mst_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@151 PS6, Line 151: realtek_mst_i2c_spi_toggle_gpio_88_strap
realtek_mst_i2c_spi_toggle_write_protect_gpio
I suggested to Shiyu to use this name instead Angel as gpio88 need not be connected to the wp pin of the external spi flash, this just happens to be the reference design.
Since the purpose of the function is general and not specific to toggling wp but rather a general gpio that happens to be routed to the wp pin I think we should keep the name as it is here and instead have a commit what its doing at the call site of the function. For example, the gpio could be routed to a led to indicate programming is occurring and is thus up to the designer. As a result we try to delineate those competing principles out here between function purpose and call site intention.
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@157 PS6, Line 157: ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, 0x9F); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF5, GPIO_CONFIG_ADDRESS >> 8); : ret |= realtek_mst_i2c_spi_write_register(fd, 0xF4, GPIO_CONFIG_ADDRESS & 0xFF);
Maybe turn these groups of three register writes into an auxiliary function? […]
That's a good idea and thanks for identifying the pattern Angel.
Shiyu, I think Angel is right here and perhaps if you could prepare a follow up commit that refactors to collapse this pattern out of common places.
https://review.coreboot.org/c/flashrom/+/46089/6/realtek_mst_i2c_spi.c@182 PS6, Line 182: (toggle ? 0x01 : 0x00)
This can be simplified as follows: […]
agreed, and toggle could really be typed as a bool as well imho.