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

[Yuan, Perry]

Hi Pierre

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: 2021年2月17日 22:24
> To: Perry Yuan; Yuan, Perry; oder_chiou@xxxxxxxxxxx; perex@xxxxxxxx;
> tiwai@xxxxxxxx; hdegoede@xxxxxxxxxx; mgross@xxxxxxxxxxxxxxx
> Cc: alsa-devel@xxxxxxxxxxxxxxxx; Limonciello, Mario; linux-
> kernel@xxxxxxxxxxxxxxx; lgirdwood@xxxxxxxxx; platform-driver-
> x86@xxxxxxxxxxxxxxx; broonie@xxxxxxxxxx
> Subject: Re: [PATCH v3 1/3] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> 
> 
> On 2/17/21 6:47 AM, Perry Yuan wrote:
> > Hi Pierre:
> > On 2021/2/16 22:56, Pierre-Louis Bossart wrote:
> >>
> >>>>> +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> >>>>> +    {"PNP0C09", 0},
> >>>>> +    { },
> >>>>> +};
> >>>>> +MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);
> >>>>> +
> >>>>> +static struct platform_driver dell_privacy_platform_drv = {
> >>>>> +    .driver = {
> >>>>> +        .name = PRIVACY_PLATFORM_NAME,
> >>>>> +        .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
> >>>>> +    },
> >>>>
> >>>> no .probe?
> >>> Originally i added the probe here, but it cause the driver  .probe
> >>> called twice. after i use platform_driver_probe to register the
> >>> driver loading process, the duplicated probe issue resolved.
> >>>
> >>> I
> >>>>
> >>>>> +    .remove = dell_privacy_acpi_remove, };
> >>>>> +
> >>>>> +int __init dell_privacy_acpi_init(void) {
> >>>>> +    int err;
> >>>>> +    struct platform_device *pdev;
> >>>>> +    int privacy_capable = wmi_has_guid(DELL_PRIVACY_GUID);
> >>>>> +
> >>>>> +    if (!wmi_has_guid(DELL_PRIVACY_GUID))
> >>>>> +        return -ENODEV;
> >>>>> +
> >>>>> +    privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
> >>>>> +    if (!privacy_acpi)
> >>>>> +        return -ENOMEM;
> >>>>> +
> >>>>> +    pdev = platform_device_register_simple(
> >>>>> +            PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL,
> 0);
> >>>>> +    if (IS_ERR(pdev)) {
> >>>>> +        err = PTR_ERR(pdev);
> >>>>> +        goto pdev_err;
> >>>>> +    }
> >>>>> +    err = platform_driver_probe(&dell_privacy_platform_drv,
> >>>>> +            dell_privacy_acpi_probe);
> >>>>> +    if (err)
> >>>>> +        goto pdrv_err;
> >>>>
> >>>> why is the probe done here? Put differently, what prevents you from
> >>>> using a 'normal' platform driver, and do the leds_setup in the
> >>>> .probe()?
> >>> At first ,I used the normal platform driver framework, however tt
> >>> cause the driver  .probe called twice. after i use
> >>> platform_driver_probe to register the driver loading process, the
> >>> duplicated probe issue resolved.
> >>
> >> This sounds very odd...
> >>
> >> this looks like a conflict with the ACPI subsystem finding a device
> >> and probing the driver that's associated with the PNP0C09 HID, and
> >> then this module __init  creating a device manually which leads to a
> >> second probe
> >>
> >> Neither the platform_device_register_simple() nor
> >> platform_driver_probe() seem necessary?Because this privacy acpi
> >> driver file has dependency on the ec handle,
> > so i want to determine if the driver can be loaded basing on the EC ID
> > PNP0C09 matching.
> >
> > So far,It works well for me to register the privacy driver with  the
> > register sequence.
> > Dose it hurt to keep current registering process with
> > platform_driver_probe used?
> 
> Sorry, I don't understand why you need to list PNP0C09 HID in a matching
> table if it's not used to probe anything.
> 
> The purpose of those matching tables is that when this HID is found, the core
> will invoke the probe callback with no need for any manual intervention.
> 
> If you want to do things manually with the module init, that's fine, it's the
> combination of the two that I find questionable.
> 
> It's like having both a manual and automatic transmission in a car, with the
> automatic transmission not coupled to the wheels.

I understand your point,I have removed the PNP0C09 ID from V4 patch.
Thanks for your suggestion very much !

Perry