Hi,
On 3/11/23 21:35, Nikita Kravets wrote:
> Hi,
>
>> The above two functions are inconsistent with the rest of the file because
>> they have the return type in a separate line.
>
> I believe it was originally made to not exceed the limit of 80 columns
>
>> It's a small thing, but usually /* ... */ comments are preferred.
>> Hans will tell you if you need to change it.
>
> Okay
Yes I noticed these too during my review, but I'm fine with them as long as a single driver uses a single comment style consistently.
Note most drivers/platform/x86 drivers do indeed use /* ... */, so I would not mind if you convert the comments, but this is not a blocker for merging the driver.
>> Previously you said `match_string()` works here. Has something changed?
>
> No, I implemented it in the main repo but forgot to do it in the kernel patch..
> I'll do it in the v3 after we're finished reviewing v2
Thanks, sounds good.
Only remark which I have is that the driver still has no modaliases so if this gets enabled as a module by distros, which is what most distros will do it will never load.
I think you should add a dmi_system_id table together with MODULE_DEVICE_TABLE() e.g. something like this:
static const struct dmi_system_id msi_dmi_table[] __initconst = {
{
matches = {
DMI_MATCH(DMI_SYS_VENDOR, "MICRO-STAR INT"),
}
},
{
matches = {
DMI_MATCH(DMI_SYS_VENDOR, "Micro-Star International"),
}
},
{}
};
MODULE_DEVICE_TABLE(dmi, msi_dmi_table);
To make the module auto-load. 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).
Regards,
Hans
