Re: [RFC PATCH 8/8] PNPACPI: create both PNP and Platform device nodes for PNP0C01/PNP0C02

[Bjorn Helgaas <bhelgaas@google.com>]

On Mon, Mar 3, 2014 at 7:17 AM, Zhang Rui <rui.zhang@intel.com> wrote:
> Hi, Bjorn,
>
> do you have comments for this particular patch?

Nope, I'm not paying attention to this area any more.

> On Wed, 2014-02-26 at 17:11 +0800, Zhang Rui wrote:
>> ACPI devices with id "PNP0C01/PNP0C02" means that we need to
>> protect their resources from being allocated by others.
>>
>> Currently, this is done in drivers/pnp/system.c.
>>
>> But the problem is that, there are some devices with extra ids besides
>> PNP0C01/PNP0C02, and for these devices,
>> 1) PNP0C01/PNP0C02 suggest that resource reservation is still needed.
>> 2) the other ids suggest that we should enumerate them to platform bus
>>
>> To reserve resources for those devices, we should either use the current code
>> by exporting the device to PNP bus, or introduce resource reservation support
>> in platform bus/ACPI.
>>
>> This patch follows the first way by enumerating an ACPI device to platform bus
>> AND pnp bus at the same time.
>> Its PNP device node will be probed by drivers/pnp/system.c and do
>> everything as we do today.
>> Its platform device node will also be created so that a platform bus
>> driver can be probed.
>>
>> The advantage is that, it brings little change to the current code,
>> the patch itself looks safe and clear.
>> The disadvantage is that
>> 1) we create two physical device nodes for the same ACPI node,
>>    this is against our effort that has been doing recently.
>> 2) we still depend on PNP bus to do this (resouce reservation) for us,
>>    which is still a problem we need to fix sooner or later.
>>
>> An alternative proposal is to remove the depedency of PNP bus and
>> do resource management in ACPI for all PNP0C01/PNP0C02 devices instead,
>> no matter what bus they are enumerated to.
>> To do this, we need to
>> 1) introduce a fs_initcall() in ACPI, to reserve all PNP0C01/PNP0C02 resources
>> in ACPI, something like we did via drivers/acpi/motherboard.c before
>> (but the code needs to follow drivers/pnp/quirks.c and system.c strictly).
>> This initcall will be run after PCI claiming BARs and before PCI assigning
>> resources for uninitialized devices.
>> 2) skip drivers/pnp/quirks.c and drivers/pnp/system.c for ACPI
>>    enumerted PNP devices, by checking pnp_device->protocal.
>> 3) remove PNP0C01/PNP0C02 from PNPACPI white list.
>>
>> By doing this, we can remove the depedency of PNP bus, but this requires
>> a lot of code duplication(need to copy quirks.c and system.c logic into ACPI),
>> which does not look good neither.
>>
>> Any comments will be appreciated.
>>
>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> ---
>>  drivers/pnp/pnpacpi/core.c |   20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pnp/pnpacpi/core.c b/drivers/pnp/pnpacpi/core.c
>> index 739fa24..5b13600 100644
>> --- a/drivers/pnp/pnpacpi/core.c
>> +++ b/drivers/pnp/pnpacpi/core.c
>> @@ -173,9 +173,6 @@ static const struct acpi_device_id acpi_pnp_device_ids[]= {
>>       {"FUJ02bf"},
>>       {"FUJ02B1"},
>>       {"FUJ02E3"},
>> -     /* system */
>> -     {"PNP0c02"}, /* General ID for reserving resources */
>> -     {"PNP0c01"}, /* memory controller */
>>       /* c6xdigio */
>>          {"PNP0400"}, /* Standard LPT Printer Port */
>>          {"PNP0401"}, /* ECP Printer Port */
>> @@ -677,6 +674,20 @@ static int is_cmos_rtc_device(struct acpi_device *adev)
>>       return !acpi_match_device_ids(adev, ids);
>>  }
>>
>> +/*
>> + * For devices with id "PNP0C01"/"PNP0C02", they will be enumerated
>> + * to PNP bus anyway to do resource reservation.
>> + */
>> +static int is_system_device(struct acpi_device *adev)
>> +{
>> +     struct acpi_device_id ids[] = {
>> +             {"PNP0C02"},
>> +             {"PNP0C01"},
>> +             {""},
>> +     };
>> +     return !acpi_match_device_ids(adev, ids);
>> +}
>> +
>>  static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
>>                                                    u32 lvl, void *context,
>>                                                    void **rv)
>> @@ -685,7 +696,8 @@ static acpi_status __init pnpacpi_add_device_handler(acpi_handle handle,
>>
>>       if (acpi_bus_get_device(handle, &device))
>>               return AE_CTRL_DEPTH;
>> -     if (device->handler == &pnpacpi_handler || is_cmos_rtc_device(device))
>> +     if (device->handler == &pnpacpi_handler || is_system_device(device) ||
>> +         is_cmos_rtc_device(device))
>>               pnpacpi_add_device(device);
>>       return AE_OK;
>>  }
>
>