Attention is currently required from: Cliff Huang, David Ruth, Lance Zhao, Paul Menzel, Subrata Banik, Subrata Banik, Tim Wawrzynczak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80170?usp=email )
Change subject: Add MTCL function to ACPI SSDT tables ......................................................................
Patch Set 8: Code-Review+1
(5 comments)
Patchset:
PS6:
My answer applied to the data that is being stored in coreboot. […]
First of all, don't worry about this comment. I'm merely curious. If you think it's taking too much of your time, please ignore it.
The reason I ask is because I looked into the kernel driver and know too much already: There are three things coming together when the driver gets loaded: 1. driver code itself, obviously, 2. the information queried from ACPI, which your patch adds, 3. a firmware binary, I assume that gets loaded into the wifi chip.
So there are three places where this country list could be stored, all available at the same time. But somebody picked ACPI for it and that usually has a reason. Thought you might know it.
ACPI usually describes hardware, not countries. So this would only make sense if the country list is actually derived from some hardware setting (e.g. straps or registers configured by coreboot). Do you happen to know if the CBFS files you're going to add are specific to the board design or is it just "the file Mediatek provides us"?
File src/acpi/acpigen.c:
https://review.coreboot.org/c/coreboot/+/80170/comment/2723e204_b1ad1812 : PS8, Line 1940: * Generate ACPI AML code for MTCL method. Looking around, it seems there really are only standardized things in this file. If MTCL means what I think, in particular MT = Mediatek, it might be better to move it or maybe write a more generalized version, e.g. ``` void acpigen_write_buffer_as_int_package(const char *name, uint8_t *, size_t) ```
Don't know if this would be useful for any other use-case, though. It looks much like somebody confused `Buffer()` and `Package()` and now Mediatek is stuck with it.
https://review.coreboot.org/c/coreboot/+/80170/comment/770e28a2_89458e92 : PS8, Line 1953: * } You could achieve the same with ``` Name (MTCL, Package() { // data } ``` i.e. without a method. In ACPI it's all just objects that evaluate to something. And that something has to be of the correct type. But what the actual object (named or method) is, doesn't really matter.
File src/drivers/wifi/generic/acpi.c:
https://review.coreboot.org/c/coreboot/+/80170/comment/027f36d8_3dba4596 : PS6, Line 592: uint8_t mtcl_package[sizeof(struct wifi_mtcl)];
Addressing the organization of the code (and updated the commit message to hopefully make what I'm t […]
Thanks for moving. I think everything is in a good place now.
File src/vendorcode/google/chromeos/mtcl.c:
https://review.coreboot.org/c/coreboot/+/80170/comment/98597b9d_cb864cac : PS6, Line 59: if (mtcl_bin_len != sizeof(struct wifi_mtcl)) {
and keep them localized to this file.
You didn't, though. There's a `struct wifi_mtcl` at the calling site, even if it's just for a `sizeof()`. And that can make readers stumble. Upstream code is more often read than changed.
I don't want to bikeshed this, though. It just looked odd that both sites knew the struct but it wasn't used in the signature, hence I asked.