Hi, On 3/25/21 8:52 AM, Yuan, Perry wrote: > Hi Hans. > >> -----Original Message----- >> From: Hans de Goede <[email protected]> >> Sent: Wednesday, March 24, 2021 3:40 AM >> To: Pierre-Louis Bossart; Yuan, Perry; [email protected]; >> [email protected]; [email protected]; [email protected]; >> [email protected]; Limonciello, Mario >> Cc: [email protected]; [email protected]; >> [email protected]; [email protected]; >> [email protected] >> Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell >> hardware privacy >> >> >> [EXTERNAL EMAIL] >> >> 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 >> > > If adding static to the function, I will be worried about that the init and > exit cannot be called by another file .
That is right, which is why I added the "?". But this is no longer relevant after my detailed review of the patch, so lets discuss things further in the detailed review email-thread. Regards, Hans
