Date:   Sat, 31 Dec 2016 21:45:42 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Hanjun Guo <guohanjun@...wei.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Marc Zyngier <marc.zyngier@....com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Greg KH <gregkh@...uxfoundation.org>,
        Tomasz Nowicki <tn@...ihalf.com>, Ma Jun <majun258@...wei.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Agustin Vega-Frias <agustinv@...eaurora.org>,
        Sinan Kaya <okaya@...eaurora.org>,
        Charles Garcia-Tobin <charles.garcia-tobin@....com>,
        huxinwei@...wei.com, yimin@...wei.com, linuxarm@...wei.com,
        Jon Masters <jcm@...hat.com>,
        Hanjun Guo <hanjun.guo@...aro.org>
Subject: Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based
 platform device

On Fri, Dec 30, 2016 at 11:50 AM, Hanjun Guo <guohanjun@...wei.com> wrote:
> Hi Rafael,
>
> On 2016/12/26 9:31, Hanjun Guo wrote:
> [cut]
>>
>> +       if (pdevinfo->pre_add_cb)
>> +               pdevinfo->pre_add_cb(&pdev->dev);
>> +
>>>>> -> because it looks like this might be done in acpi_platform_notify()
>>>>> for platform devices.
>>>> It works and I just simply add the code below:
>>>>
>>>> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
>>>> index f8d6564..e0cd649 100644
>>>> --- a/drivers/acpi/glue.c
>>>> +++ b/drivers/acpi/glue.c
>>>> @@ -13,6 +13,7 @@
>>>>  #include <linux/slab.h>
>>>>  #include <linux/rwsem.h>
>>>>  #include <linux/acpi.h>
>>>> +#include <linux/acpi_iort.h>
>>>>  #include <linux/dma-mapping.h>
>>>>
>>>>  #include "internal.h"
>>>> @@ -315,6 +316,8 @@ static int acpi_platform_notify(struct device *dev)
>>>>         if (!adev)
>>>>                 goto out;
>>>>
>>>> + acpi_configure_pmsi_domain(dev);
>>>> +
>>> But that should apply to platform devices only I suppose?
>> Yes, it's only for the platform device.
>>
>>>>         if (type && type->setup)
>>>>                 type->setup(dev);
>>>>         else if (adev->handler && adev->handler->bind)
>>>>
>>>> Do you suggesting to configure the msi domain in this way?
>>>> or add the function in the type->setup() callback (which needs
>>>> to introduce a new acpi bus type)?
>>> A type->setup() would be somewhat cleaner I think, but then it's more
>>> code.  Whichever works better I guess. :-)
>> Agree, I will demo the type->setup() way and send out the patch for review,
>> also I find one minor issue for the IORT code, will update that also for next
>> version.
>
> Just demo the code and find out it's seems to cut the feet to the type->setup() code,
> because we need a match function (it's ok) and a find_companion() (we don't need that
> and make the code worse because we will call the find_companion callback which it not needed
> for platform devices:
>
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index 96983c9..654021d9b 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -138,3 +138,31 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
>         return pdev;
>  }
>  EXPORT_SYMBOL_GPL(acpi_create_platform_device);
> +
> +static bool platform_acpi_bus_match(struct device *dev)
> +{
> + return dev->bus == &platform_bus_type;
> +}
> +
> +static struct acpi_device *platform_acpi_bus_find_companion(struct device *dev)
> +{
> + /* demo code, do nothing here */
> + return NULL;
> +}
> +
> +static void platform_acpi_setup(struct device *dev)
> +{
> + acpi_configure_pmsi_domain(dev);
> +}
> +
> +static struct acpi_bus_type acpi_platform_bus = {
> + .name = "Platform",
> + .match = platform_acpi_bus_match,
> + .find_companion = platform_acpi_bus_find_companion,
> + .setup = platform_acpi_setup,
> +};
> +
> +int acpi_platform_bus_register(void)
> +{
> + return register_acpi_bus_type(&acpi_platform_bus);
> +}
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 95855cb..0a0a639 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -1199,6 +1199,7 @@ static int __init acpi_init(void)
>         }
>
>         pci_mmcfg_late_init();
> + acpi_platform_bus_register();
>         acpi_iort_init();
>         acpi_scan_init();
>         acpi_ec_init();
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 809b536..1d05f92 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -597,6 +597,8 @@ extern bool acpi_driver_match_device(struct device *dev,
>
>  struct platform_device *acpi_create_platform_device(struct acpi_device *,
>                                                     struct property_entry *);
> +int acpi_platform_bus_register(void);
> +
>  #define ACPI_PTR(_ptr) (_ptr)
>
>  static inline void acpi_device_set_enumerated(struct acpi_device *adev)
>
>
> So how about just add the code as below?

Works for me.

> diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
> index 11e63dd..37a8dfe 100644
> --- a/drivers/acpi/glue.c
> +++ b/drivers/acpi/glue.c
> @@ -316,7 +316,8 @@ static int acpi_platform_notify(struct device *dev)
>         if (!adev)
>                 goto out;
>
> + if (dev->bus == &platform_bus_type)
> +         acpi_configure_pmsi_domain(dev);
>
>         if (type && type->setup)
>                 type->setup(dev);

Thanks,
Rafael

Powered by blists - more mailing lists