Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932073AbcLaUps (ORCPT ); Sat, 31 Dec 2016 15:45:48 -0500 Received: from mail-oi0-f65.google.com ([209.85.218.65]:35612 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754575AbcLaUpo (ORCPT ); Sat, 31 Dec 2016 15:45:44 -0500 MIME-Version: 1.0 In-Reply-To: <58663C04.9000603@huawei.com> References: <1482384922-21507-1-git-send-email-guohanjun@huawei.com> <1482384922-21507-10-git-send-email-guohanjun@huawei.com> <585E24FB.9050805@huawei.com> <586072E9.3060609@huawei.com> <58663C04.9000603@huawei.com> From: "Rafael J. Wysocki" Date: Sat, 31 Dec 2016 21:45:42 +0100 X-Google-Sender-Auth: 7r3XrZO4fYOIwm78YWBkFoIhGCw Message-ID: Subject: Re: [PATCH v5 09/14] ACPI: platform: setup MSI domain for ACPI based platform device To: Hanjun Guo Cc: "Rafael J. Wysocki" , Marc Zyngier , "Rafael J. Wysocki" , Lorenzo Pieralisi , ACPI Devel Maling List , "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List , Thomas Gleixner , Greg KH , Tomasz Nowicki , Ma Jun , Kefeng Wang , Agustin Vega-Frias , Sinan Kaya , Charles Garcia-Tobin , huxinwei@huawei.com, yimin@huawei.com, linuxarm@huawei.com, Jon Masters , Hanjun Guo Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4442 Lines: 135 On Fri, Dec 30, 2016 at 11:50 AM, Hanjun Guo 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 >>>> #include >>>> #include >>>> +#include >>>> #include >>>> >>>> #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