| Message ID | 1386115178-7559-3-git-send-email-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Dec 03, 2013 at 03:59:38PM -0800, David E. Box wrote: > From: "David E. Box" <david.e.box@linux.intel.com> > > Adds ACPI ID for Intel IOSF-SB MailBox device found in BayTrail platforms. Little bit confused here. This is a new driver and only declares modaliases for the ACPI IDs - why does it need to be added here?
On Wed, Dec 04, 2013 at 01:30:01AM +0000, Matthew Garrett wrote: > On Tue, Dec 03, 2013 at 03:59:38PM -0800, David E. Box wrote: > > From: "David E. Box" <david.e.box@linux.intel.com> > > > > Adds ACPI ID for Intel IOSF-SB MailBox device found in BayTrail platforms. > > Little bit confused here. This is a new driver and only declares > modaliases for the ACPI IDs - why does it need to be added here? > This is per the requirement in Documentation/acpi/enumeration.txt: "Currently the kernel is not able to automatically determine from which ACPI device it should make the corresponding platform device so we need to add the ACPI device explicitly to acpi_platform_device_ids list defined in drivers/acpi/acpi_platform.c" Without adding the device here it would not be discovered and probe would not be called during init. David Box > -- > Matthew Garrett | mjg59@srcf.ucam.org -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 03, 2013 at 06:17:03PM -0800, David E. Box wrote: > This is per the requirement in Documentation/acpi/enumeration.txt: > > "Currently the kernel is not able to automatically determine from which ACPI > device it should make the corresponding platform device so we need to add > the ACPI device explicitly to acpi_platform_device_ids list defined in > drivers/acpi/acpi_platform.c" Well sure, but why do you need to be a platform device at all? This functionality was intended for cases where we already have a driver for the part that enumerated it via some other mechanism. If the driver's only intended for ACPI systems then why not just be an ACPI device?
On Wed, Dec 04, 2013 at 02:21:30AM +0000, Matthew Garrett wrote: > On Tue, Dec 03, 2013 at 06:17:03PM -0800, David E. Box wrote: > > This is per the requirement in Documentation/acpi/enumeration.txt: > > > > "Currently the kernel is not able to automatically determine from which ACPI > > device it should make the corresponding platform device so we need to add > > the ACPI device explicitly to acpi_platform_device_ids list defined in > > drivers/acpi/acpi_platform.c" > > Well sure, but why do you need to be a platform device at all? This > functionality was intended for cases where we already have a driver for > the part that enumerated it via some other mechanism. If the driver's > only intended for ACPI systems then why not just be an ACPI device? > It was my understanding that with ACPI 5.0 it was becoming more common to use ACPI ID's exclusively for device enumeration. I originally wrote this as an acpi_bus driver but Rafeal advised me that the model is being phased out and suggeted the platform model instead. > -- > Matthew Garrett | mjg59@srcf.ucam.org -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 03, 2013 at 06:44:52PM -0800, David E. Box wrote: > On Wed, Dec 04, 2013 at 02:21:30AM +0000, Matthew Garrett wrote: > > Well sure, but why do you need to be a platform device at all? This > > functionality was intended for cases where we already have a driver for > > the part that enumerated it via some other mechanism. If the driver's > > only intended for ACPI systems then why not just be an ACPI device? > > > > It was my understanding that with ACPI 5.0 it was becoming more common to use > ACPI ID's exclusively for device enumeration. I originally wrote this as an > acpi_bus driver but Rafeal advised me that the model is being phased out and > suggeted the platform model instead. If you're not adding ACPI support to an existing platform driver, you shouldn't be adding entries to acpi_platform.c. I'm not actually happy that I merged the ideapad-laptop patch that did the same thing - I'm inclined to revert it, because this really is an ugly way to do things. Rafael, why did we convert the AC driver this way? It means we have to keep track of ACPI IDs in multiple places, which is worth it when it avoids having to write a pile of new code (such as the sdhci case) but doesn't seem to provide benefits otherwise.
On Wednesday, December 04, 2013 02:54:07 AM Matthew Garrett wrote: > On Tue, Dec 03, 2013 at 06:44:52PM -0800, David E. Box wrote: > > On Wed, Dec 04, 2013 at 02:21:30AM +0000, Matthew Garrett wrote: > > > Well sure, but why do you need to be a platform device at all? This > > > functionality was intended for cases where we already have a driver for > > > the part that enumerated it via some other mechanism. If the driver's > > > only intended for ACPI systems then why not just be an ACPI device? Because struct acpi_device things are firmawre objects that shouldn't be treated as devices. > > > > It was my understanding that with ACPI 5.0 it was becoming more common to use > > ACPI ID's exclusively for device enumeration. I originally wrote this as an > > acpi_bus driver but Rafeal advised me that the model is being phased out and > > suggeted the platform model instead. > > If you're not adding ACPI support to an existing platform driver, you > shouldn't be adding entries to acpi_platform.c. I'm not actually happy > that I merged the ideapad-laptop patch that did the same thing - I'm > inclined to revert it, because this really is an ugly way to do things. > > Rafael, why did we convert the AC driver this way? It means we have to > keep track of ACPI IDs in multiple places, Yes, in two places to be precise. > which is worth it when it avoids having to write a pile of new code (such as > the sdhci case) but doesn't seem to provide benefits otherwise. Well, I thought we discussed that, but perhaps we didn't. There's some confusion about what a struct acpi_device is, because sometimes it is treated as a "companion" of a physical device and sometimes it is treated as a physical device itself. This leads to incorrect code as well sometimes, because ACPI drivers may attempt to bind to things that already have physical comapnions, for example. The approach to this I thought we could use, and that we've been using to some extent already, is to create "physical" companions for all struct acpi_device objects. In this particular this "physical companion" is a platform device, so it needs to be added to the list in acpi_platform.c. [Otherwise the ACPI PNP core would create a PNP device for it (unless an ACPI driver is bound to that struct acpi_device before, but that makes the correctness of the thing depend on the ordering of driver probing, which is not awesome).] In this approach device drivers bind to the "physical" devices (platform devices etc.) instead of struct acpi_device themselves and at least everything is consistent. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c index 1bde127..12aa55c 100644 --- a/drivers/acpi/acpi_platform.c +++ b/drivers/acpi/acpi_platform.c @@ -29,7 +29,8 @@ ACPI_MODULE_NAME("platform"); static const struct acpi_device_id acpi_platform_device_ids[] = { { "PNP0D40" }, - + /* Intel IOSF-SB Message Bus */ + { "INT33BD" }, { } };