Attention is currently required from: Michał Żygowski, Paul Menzel.
Patch set 10:Code-Review +1
8 comments:
Patchset:
Thanks for the update and ping. I think this change didn't get
the attention it deserves because it's sitting amidst an unrelated
patch train with status "Merge Conflict".
It looks almost ready, so I think it's best to rebase it on master
directly. One small cosmetic issue (`lp` suffix) and I think adding
`BUS_ESPI` is raising unnecessary questions right now, so I'd just
put a `0` where used it ;)
File chipset_enable.c:
Patch Set #3, Line 659: { "eSPI", BUS_LPC | BUS_FWH } };
Ping
I still wonder if eSPI is a bus for flashes at all? See
comments on the enum change.
Probably easiest to just put a 0 here. The value is only processed by
the calling function and there is nothing to be done for eSPI atm.
File chipset_enable.c:
The `lp` suffix was previously used when there was a low-power version
with different behavior. I don't think this is the case with TGP?
File flash.h:
Patch Set #10, Line 70: BUS_ESPI = 1 << 5,
Checking how all paths react to this change takes some time and
should eventually be reviewed as a separate commit. I had a quick
look and it seems BUS_NONSPI (see below) might be the only incon-
sistency. I guess if you try to fix that some unit-test will break
and fixing that might lead to the question if flashbuses_to_text()
should be updated, etc...
So I wonder if it's worth to add this before we actually act on
the new value anywhere. Coming back to the question if there are
flash chips for eSPI.
Patch Set #10, Line 71: BUS_NONSPI = BUS_PARALLEL | BUS_LPC | BUS_FWH,
This would definitely need an update. Although, I'm not sure if it is
consistently used throughout flashrom code.
File ich_descriptors.c:
Patch Set #5, Line 278: return "reserved";
Kindly asking to check if it does look good now
Ack
Thanks for taking fully care of `freq_read`.
Patch Set #5, Line 967: return CHIPSET_500_SERIES_TIGER_POINT;
Please check
Done
File ich_descriptors.c:
Patch Set #10, Line 316: msg_pdbg2("Read Clock Frequency: %s\n", "reserved");
Nit, I think we can just skip printing anything here.
To view, visit change 55578. To unsubscribe, or for help writing mail filters, visit settings.