Hi,
On 3/25/21 8:52 AM, Yuan, Perry wrote:
> Hi Hans.
>
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@xxxxxxxxxx>
>> Sent: Wednesday, March 24, 2021 3:40 AM
>> To: Pierre-Louis Bossart; Yuan, Perry; pobrn@xxxxxxxxxxxxxx;
>> oder_chiou@xxxxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx;
>> mgross@xxxxxxxxxxxxxxx; Limonciello, Mario
>> Cc: alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>> lgirdwood@xxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx;
>> broonie@xxxxxxxxxx
>> 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
