Attention is currently required from: Edward O'Callaghan. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59276 )
Change subject: pcidev: Move pci_get_dev() logic into canonical place ......................................................................
Patch Set 4:
(1 comment)
File board_enable.c:
https://review.coreboot.org/c/flashrom/+/59276/comment/baa6fcc1_6154cef6 PS3, Line 1097: dev = pcidev_clonedev(dev);
The direction should be fairly clear, I rebased the follow on patches to give clarity. You realise every time you change your mind here I need to keep wasting time rebasing.
How you organize your patch trains is up to you. If constant rebasing is an issue, then don't rebase constantly. Are you really making reviewers responsible for necessary changes during review here? Can you please tell me in your own words what is the point of reviews?
Give actionable feedback, you wanted it renamed, then you changed your mind but agreed that 1 in the function name will do it now your comment is sort of high-Z.
Sorry, your patches look like you are spending little time to write them. So please don't expect me to take much more time to write and repeat endless explanations. I changed my mind because my original suggestion was based on the idea that your original naming choice (pcidev_get_dev()) made sense which it made little. I'll try to do better in the future and not to run into that pitfall again.
The change here is not particularly controversial, it is just isolating libpci stuff into pcidev.c which is pretty logical - look in pcidev.c for libpci stuff. So I don't know what you mean by "I have to look in another file", do you want the whole project in one file?
I fear, we are talking past each other. What I wanted to say is that before this patch, when reading the code in `board_status.c` I could see what the code does: it queries function 1 of the same PCI device. Now, after this change, when reading the code in `board_status.c` I can't see that anymore because the fact that it's about function 1 is hidden in another file. Putting `function` in the API name could fix that. Other languages than C might not have that problem, AFAIR you know Ada: There you could just write that parameter name in the call e.g. `pcidev_getdev(dev, function => 1)`. But in C all we can do is to choose good function and variable names. (Or add code comments, but I kind of hate those (not checked by the compiler.))
-
Nico Huber (Code Review)