Patch set 1:Code-Review -1
7 comments:
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.
Patch Set #1, 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.
Patch Set #1, 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.
these magics getting changed, looks like they should have a #define
Patch Set #1, Line 167: // Set Pin Value to High
the comment was fine and very informative before, leave it.
0xF0
0xFE
there is a pattern here.
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.
To view, visit change 46089. To unsubscribe, or for help writing mail filters, visit settings.