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

Zhang Rui
2014-03-03T14:17:50Z

Hi, Bjorn,

do you have comments for this particular patch?

thanks,
rui

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;
> }