Attention is currently required from: Cliff Huang, David Ruth, Lance Zhao, Paul Menzel, Subrata Banik, Subrata Banik, Tim Wawrzynczak.
Patch set 8:Code-Review +1
5 comments:
Patchset:
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:
Patch Set #8, 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.
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:
Patch Set #6, 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:
Patch Set #6, 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.
To view, visit change 80170. To unsubscribe, or for help writing mail filters, visit settings.