[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SJ0PR19MB4528ED97AA4A5D0821A3E68584629@SJ0PR19MB4528.namprd19.prod.outlook.com>
Date: Thu, 25 Mar 2021 07:52:15 +0000
From: "Yuan, Perry" <Perry.Yuan@...l.com>
To: Hans de Goede <hdegoede@...hat.com>,
Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
"pobrn@...tonmail.com" <pobrn@...tonmail.com>,
"oder_chiou@...ltek.com" <oder_chiou@...ltek.com>,
"perex@...ex.cz" <perex@...ex.cz>,
"tiwai@...e.com" <tiwai@...e.com>,
"mgross@...ux.intel.com" <mgross@...ux.intel.com>,
"Limonciello, Mario" <Mario.Limonciello@...l.com>
CC: "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"lgirdwood@...il.com" <lgirdwood@...il.com>,
"platform-driver-x86@...r.kernel.org"
<platform-driver-x86@...r.kernel.org>,
"broonie@...nel.org" <broonie@...nel.org>
Subject: RE: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
hardware privacy
Hi Hans.
> -----Original Message-----
> From: Hans de Goede <hdegoede@...hat.com>
> Sent: Wednesday, March 24, 2021 3:40 AM
> To: Pierre-Louis Bossart; Yuan, Perry; pobrn@...tonmail.com;
> oder_chiou@...ltek.com; perex@...ex.cz; tiwai@...e.com;
> mgross@...ux.intel.com; Limonciello, Mario
> Cc: alsa-devel@...a-project.org; linux-kernel@...r.kernel.org;
> lgirdwood@...il.com; platform-driver-x86@...r.kernel.org;
> broonie@...nel.org
> 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@...l.com>");
> >> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> >> +MODULE_LICENSE("GPL");
> >
> >
Powered by blists - more mailing lists