Re: [PATCH v2] platform/x86: Add new msi-ec driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux