Hi,
On 3/15/23 15:29, Nikita Kravets wrote:
> Hi,
>
>> Note most drivers/platform/x86 drivers do indeed use /* ... */, so I would not mind if you convert the comments
>
> Okay, I'll do it then
Thank you.
>> But I guess it might be better to put that on the TODO list somewhere and do it once the module has had some more testing.
>>
>> E.g. I should really test this on my desktop's MSI B550M PRO-VDH board and see what it does there (hopefully nothing).
>
> By design it should fail to load on your motherboard since ec_read()
> will return with
> ENODEV if EC isn't present, or read an incompatible version if it is.
> Some testing would be good, so we can add a DMI table in the next set
> of patches.
Ok I have tested this on the 2 MSI devices I have access to:
MSI B550M PRO-VDH desktop motherboard:
-Does not have an ACPI EC device, so the first ec_read()
fails with -ENODEV
MSI S270 (ancient) laptop
-To my surprise this one does have an ACPI EC device,
load_configuration() fails with:
"msi_ec: Your firmware version is not supported!"
Note:
1) It would be good to log the not supported version.
2) Since hitting this is expected on unsupported models this should IMHO be changed
from a pr_err to a pr_warn
3) This laptop's EC is currently supported by the old msi-laptop kernel driver
and this driver is necessary for backlight control at least.
> I have a question about naming. We have the fn_super_swap
> configuration parameter,
> but I noticed that in the kernel code the Super key is often called Meta.
> Should I also rename it to fn_meta_swap or stick with 'Super'?
Either way is fine meta/super are both used in various places of the stack. Probably best to keep it as is to keep compatibility for existing msi-ec users.
Regards,
Hans
