Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 3/23/21 7:57 PM, Pierre-Louis Bossart wrote:
> Minor comments below.

<snip<

>> +int __init dell_privacy_acpi_init(void)
> 
> is the __init necessary? You call this routine from another which already has this qualifier.

Yes this is necessary, all functions which are only used during module_load / from the
module's init function must be marked as __init so that the kernel can unload them
after module loading is done.

I do wonder if this one should not be static though (I've not looked at this patch in
detail yet).

> 
>> +{
>> +    int err;
>> +    struct platform_device *pdev;
>> +
>> +    if (!wmi_has_guid(DELL_PRIVACY_GUID))
>> +        return -ENODEV;
>> +
>> +    privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
>> +    if (!privacy_acpi)
>> +        return -ENOMEM;
>> +
>> +    err = platform_driver_register(&dell_privacy_platform_drv);
>> +    if (err)
>> +        goto pdrv_err;
>> +
>> +    pdev = platform_device_register_simple(
>> +            PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL, 0);
>> +    if (IS_ERR(pdev)) {
>> +        err = PTR_ERR(pdev);
>> +        goto pdev_err;
>> +    }
>> +
>> +    return 0;
>> +
>> +pdev_err:
>> +    platform_device_unregister(pdev);
>> +pdrv_err:
>> +    kfree(privacy_acpi);
>> +    return err;
>> +}
>> +
>> +void __exit dell_privacy_acpi_exit(void)
> 
> is the __exit required here?

Idem. Also static ?

Regards,

Hans



> 
>> +{
>> +    struct platform_device *pdev = to_platform_device(privacy_acpi->dev);
>> +
>> +    platform_device_unregister(pdev);
>> +    platform_driver_unregister(&dell_privacy_platform_drv);
>> +    kfree(privacy_acpi);
>> +}
>> +
>> +MODULE_AUTHOR("Perry Yuan <perry_yuan@dell.com>");
>> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
>> +MODULE_LICENSE("GPL");
> 
>