Attention is currently required from: Felix Singer, Alex Badea.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/73037 )
Change subject: asm106x: add programmer for ASM106x SATA controllers ......................................................................
Patch Set 3:
(7 comments)
Patchset:
PS3: Hi Alex, thank you for the contribution! The code looks already quite good. Anyway I have a questions and some minor annotations.
On which device have you tested this driver? Can you mention it in the commit message.
File asm106x.c:
https://review.coreboot.org/c/flashrom/+/73037/comment/ba48e766_d6e120a3 PS3, Line 35: {PCI_VENDOR_ID_ASMEDIA, 0x0612, OK, "ASMedia", "ASM106x"}, This PCI-Id is assigned to the ASM1062 Serial ATA Controller, the PCI-Id 0x625 is assigned to '106x SATA/RAID Controller'. Beside that there is a PCI-Id 0x0611 for 'ASM1061 SATA IDE Controller'. For which one is this driver? Or does it work on all of them?
PCI-Ids from pciutils (https://pci-ids.ucw.cz/v2.2/pci.ids)
https://review.coreboot.org/c/flashrom/+/73037/comment/9db02c32_d99bd84a PS3, Line 51: programmer_delay(flash, 10); `programmer_delay()` should be used outside of the programmer. It is there to determine if an programmer specific delay function should be called or the default one.
Here you can use `void default_delay(unsigned int usec)`.
With it you can also pass only the `struct pci_dev *pci` to this function. ``` static int asm106x_wait_ready(const struct pci_dev *pci, uint8_t *pval) ```
https://review.coreboot.org/c/flashrom/+/73037/comment/b1ebaf4f_82ac445b PS3, Line 59: if (pval) You could move this 'if, assignment, return' where the break is. Then everything below the loop is a timeout.
https://review.coreboot.org/c/flashrom/+/73037/comment/5522f531_3ac722ae PS3, Line 74: writecnt, readcnt); This can be moved in the line above.
https://review.coreboot.org/c/flashrom/+/73037/comment/6156e402_58c4eb14 PS3, Line 115: ctrl | ASM106X_CTRL_CSN); This function call can be in one line
File meson.build:
https://review.coreboot.org/c/flashrom/+/73037/comment/7143d1a9_afdb5109 PS3, Line 152: 'cpu_families' : cpus_port_io, This line should be removed. The driver uses only PCI register to communicate with the flash, not I/O ports.
-
Thomas Heijligen (Code Review)