Hi,
First of all, sorry if any of you got the message more than once.
Gmail (and I) messed some stuff up.
> Usually commas are omitted after sentinel entries.
I see, makes sense.
> Alternatively:
>
> .allowed_fw = (const char * const []) {
> "...",
> "...",
> NULL
> },
>
> (although this won't inherit the __initdata attribute as far as I can see)
This looks nicer so the question is: how important is it to put those
strings into initdata, as they don't take much memory.
> It's a small thing, but I would make `i` and `len` be the same type.
Okay. I should also put the `i` declaration into the for loop header.
(I'm not the original creator of this function so I didn't touch it
yet)
> Why not
>
> return device_add_groups(...);
>
> ?
Agreed. Didn't look into this one too.
> Furthermore, is it possible that there are two or more batteries?
So far all laptops we've tested only have one, controlled by a single
EC parameter.
> Have you checked if `match_string()` from string.h works here?
Just checked, it does.
> This will start poking the embedded controller when the module is loaded, regardless of the platform. I am not sure that is desirable.
It only reads though, can it cause any harm?
> static int __init load_configuration(void)
> {
> int result;
>
> // get firmware version
> u8 ver[MSI_EC_FW_VERSION_LENGTH + 1];
> result = ec_get_firmware_version(ver);
> if (result < 0) {
> return result;
> }
Also a note from myself: I think this should return -EOPNOTSUPP if
ec_get_firmware() returns -ENODEV
--
Regards,
Nikita
