Change in flashrom[master]: sio.c: Put generic Super I/O access scattered throughout files together
Attention is currently required from: Michał Żygowski. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55849 )
Change subject: sio.c: Put generic Super I/O access scattered throughout files together ......................................................................
Patch Set 1:
(5 comments)
File programmer.h:
https://review.coreboot.org/c/flashrom/+/55849/comment/e9cfcbb0_ded41c42 PS1, Line 215: /* board_enable.c */ You just moved them out of there.
https://review.coreboot.org/c/flashrom/+/55849/comment/d054d2cd_14e0af99 PS1, Line 224: void sio_switch_ldn(uint16_t port, uint8_t ldn); : uint16_t sio_get_iobase(uint16_t port, uint8_t io_bar_number); : uint16_t sio_read_id(uint16_t port, uint8_t io_bar_number); : bool sio_is_ldn_enabled(uint16_t port); Why create APIs before you have a user?
File sio.c:
https://review.coreboot.org/c/flashrom/+/55849/comment/8248f2f2_ad08e511 PS1, Line 4: * Copyright (C) 2021, TUXEDO Computers GmbH If you got this code from somewhere else, how can it be copyrighted by TUXEDO?
https://review.coreboot.org/c/flashrom/+/55849/comment/c8f96218_17043aac PS1, Line 8: * the Free Software Foundation; version 2 of the License. It's an unfortunate situation, but if you pull code from files with different licenses, you should maintain them individually (or keep the code in separate files).
https://review.coreboot.org/c/flashrom/+/55849/comment/628f7090_48f74b38 PS1, Line 31: } I don't think we should encourage anyone to use such a function by making it a public API. I expect you only need a tiny bit of code out of probe_superio_ite() to confirm if the EC you expect is really there? Given that we are supposed to know the board flashrom runs on, we should already know the EC. Probing is for the case that we don't know the board.
-
Nico Huber (Code Review)