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

[Yuan, Perry]

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 .



> 
> 
> >
> >> +{
> >> +    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@xxxxxxxx>");
> >> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> >> +MODULE_LICENSE("GPL");
> >
> >