| Message ID | e6b4c58be0bc78dbc9ca8b14acf0448eda6b325c.1448480385.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
On 2015/11/26 4:19, Lukas Wunner wrote: > There's an idiom in use by 7 Linux drivers to detect the presence of a > particular ACPI HID by walking the namespace with acpi_get_devices(). > The callback passed to acpi_get_devices() is mostly identical across > the drivers, leading to lots of duplicate code. > > Add acpi_dev_present(), the ACPI equivalent to pci_dev_present(), > allowing us to deduplicate all that boilerplate in the drivers. > > v2: * Instead of walking the namespace, iterate over acpi_bus_id_list > which has all HIDs cached (Hanjun Guo, Robert Moore). > * Move to utils.c (Rafael J. Wysocki). This necessitates non-static > declaration of acpi_bus_id_list. > > Cc: Hanjun Guo <guohanjun@huawei.com> > Cc: Robert Moore <robert.moore@intel.com> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/acpi/internal.h | 8 ++++++++ > drivers/acpi/scan.c | 8 +------- > drivers/acpi/utils.c | 31 +++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 2 ++ > 4 files changed, 42 insertions(+), 7 deletions(-) > > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index 11d87bf..60bda0d 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -86,6 +86,14 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent); > #define ACPI_STA_DEFAULT (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED | \ > ACPI_STA_DEVICE_UI | ACPI_STA_DEVICE_FUNCTIONING) > > +extern struct list_head acpi_bus_id_list; > + > +struct acpi_device_bus_id{ > + char bus_id[15]; > + unsigned int instance_no; > + struct list_head node; > +}; > + > int acpi_device_add(struct acpi_device *device, > void (*release)(struct device *)); > void acpi_init_device_object(struct acpi_device *device, acpi_handle handle, > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index a0e942b..4be2179 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -39,7 +39,7 @@ static const char *dummy_hid = "device"; > > static LIST_HEAD(acpi_dep_list); > static DEFINE_MUTEX(acpi_dep_list_lock); > -static LIST_HEAD(acpi_bus_id_list); > +LIST_HEAD(acpi_bus_id_list); > static DEFINE_MUTEX(acpi_scan_lock); > static LIST_HEAD(acpi_scan_handlers_list); > DEFINE_MUTEX(acpi_device_lock); > @@ -52,12 +52,6 @@ struct acpi_dep_data { > acpi_handle slave; > }; > > -struct acpi_device_bus_id{ > - char bus_id[15]; > - unsigned int instance_no; > - struct list_head node; > -}; > - > void acpi_scan_lock_acquire(void) > { > mutex_lock(&acpi_scan_lock); > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 475c907..f2f9873 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -29,6 +29,7 @@ > #include <linux/dynamic_debug.h> > > #include "internal.h" > +#include "sleep.h" > > #define _COMPONENT ACPI_BUS_COMPONENT > ACPI_MODULE_NAME("utils"); > @@ -709,6 +710,36 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, int rev, u64 funcs) > } > EXPORT_SYMBOL(acpi_check_dsm); > > +/** > + * acpi_dev_present - Detect presence of a given ACPI device in the system. > + * @hid: Hardware ID of the device. > + * > + * Return %true if the device was present at the moment of invocation. > + * Note that if the device is pluggable, it may since have disappeared. > + * > + * For this function to work, acpi_bus_scan() must have been executed > + * which happens in the subsys_initcall() subsection. Hence, do not > + * call from a subsys_initcall() or earlier (use acpi_get_devices() > + * instead). Calling from module_init() is fine (which is synonymous > + * with device_initcall()). > + */ > +bool acpi_dev_present(const char *hid) If the driver can't pass the "dev" as the argument form acpi device or platform device, this is the way to match HIDs. But if we get the pointer to acpi device or platform device structure before calling this function, I think acpi_match_device_ids() can match what you need, and we don't need to invent another function. Thanks Hanjun -- 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
Hi Hanjun Guo, thank you for taking a look at the patches. On Mon, Nov 30, 2015 at 04:27:23PM +0800, Hanjun Guo wrote: > If the driver can't pass the "dev" as the argument form acpi device or > platform device, this is the way to match HIDs. > > But if we get the pointer to acpi device or platform device structure > before calling this function, I think acpi_match_device_ids() can match > what you need, and we don't need to invent another function. Of the 7 users, * 4 detect the presence of a particular HID to activate platform- specific quirks (acer-wmi.c, eeepc-wmi.c, thinkpad_helper.c, cht_bsw_max98090_ti.c). The drivers never obtain a pointer to the struct acpi_device. This is also the reason why I need to detect the presence of apple-gmux in several DRM drivers, to activate quirks specific to MacBook Pros with dual GPUs, and the one thing they have in common is the presence of the gmux controller. The DRM drivers likewise never have a need to hold a pointer to apple-gmux' struct acpi_device. * 2 detect the presence of a particular HID and *afterwards* instantiate a platform_device (atom/sst/sst_acpi.c and common/sst-acpi.c). So we can't use acpi_match_device_ids() there either. * 1 is a driver for a platform_device (cht_bsw_rt5645.c) which was instantiated by atom/sst/sst_acpi.c. The driver is responsible for two chips and differentiates between the two by detecting the presence of a particular HID. It would be possible to refactor the code so that atom/sst/sst_acpi.c passes down the matched HID to cht_bsw_rt5645.c, then it wouldn't be necessary to match for a second time. Also, the only difference between the two chipsets seems to be a minute change in struct snd_soc_dapm_route, so I'm wondering if it's necessary to differentiate between the chipsets at all. Mark Brown may want to poke the developers of the driver to refactor the code. TL;DR: 6 of the 7 users can't use acpi_match_device_ids() and the 7th could be refactored so that it doesn't have to detect the presence of a specific HID at all. Best regards, Lukas -- 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 2015/12/1 20:58, Lukas Wunner wrote: > Hi Hanjun Guo, > > thank you for taking a look at the patches. > > On Mon, Nov 30, 2015 at 04:27:23PM +0800, Hanjun Guo wrote: >> If the driver can't pass the "dev" as the argument form acpi device or >> platform device, this is the way to match HIDs. >> >> But if we get the pointer to acpi device or platform device structure >> before calling this function, I think acpi_match_device_ids() can match >> what you need, and we don't need to invent another function. > Of the 7 users, > > * 4 detect the presence of a particular HID to activate platform- > specific quirks (acer-wmi.c, eeepc-wmi.c, thinkpad_helper.c, > cht_bsw_max98090_ti.c). The drivers never obtain a pointer to the > struct acpi_device. This is also the reason why I need to detect > the presence of apple-gmux in several DRM drivers, to activate > quirks specific to MacBook Pros with dual GPUs, and the one thing > they have in common is the presence of the gmux controller. > The DRM drivers likewise never have a need to hold a pointer to > apple-gmux' struct acpi_device. > > * 2 detect the presence of a particular HID and *afterwards* > instantiate a platform_device (atom/sst/sst_acpi.c and > common/sst-acpi.c). So we can't use acpi_match_device_ids() > there either. > > * 1 is a driver for a platform_device (cht_bsw_rt5645.c) which was > instantiated by atom/sst/sst_acpi.c. The driver is responsible > for two chips and differentiates between the two by detecting the > presence of a particular HID. It would be possible to refactor the > code so that atom/sst/sst_acpi.c passes down the matched HID to > cht_bsw_rt5645.c, then it wouldn't be necessary to match for a > second time. Also, the only difference between the two chipsets > seems to be a minute change in struct snd_soc_dapm_route, so I'm > wondering if it's necessary to differentiate between the chipsets > at all. Mark Brown may want to poke the developers of the driver to > refactor the code. > > TL;DR: 6 of the 7 users can't use acpi_match_device_ids() and the > 7th could be refactored so that it doesn't have to detect the presence > of a specific HID at all. Thanks for the clarify, in this case, this patch makes sense to me. Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org> Hanjun -- 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/internal.h b/drivers/acpi/internal.h index 11d87bf..60bda0d 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -86,6 +86,14 @@ bool acpi_scan_is_offline(struct acpi_device *adev, bool uevent); #define ACPI_STA_DEFAULT (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED | \ ACPI_STA_DEVICE_UI | ACPI_STA_DEVICE_FUNCTIONING) +extern struct list_head acpi_bus_id_list; + +struct acpi_device_bus_id{ + char bus_id[15]; + unsigned int instance_no; + struct list_head node; +}; + int acpi_device_add(struct acpi_device *device, void (*release)(struct device *)); void acpi_init_device_object(struct acpi_device *device, acpi_handle handle, diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index a0e942b..4be2179 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -39,7 +39,7 @@ static const char *dummy_hid = "device"; static LIST_HEAD(acpi_dep_list); static DEFINE_MUTEX(acpi_dep_list_lock); -static LIST_HEAD(acpi_bus_id_list); +LIST_HEAD(acpi_bus_id_list); static DEFINE_MUTEX(acpi_scan_lock); static LIST_HEAD(acpi_scan_handlers_list); DEFINE_MUTEX(acpi_device_lock); @@ -52,12 +52,6 @@ struct acpi_dep_data { acpi_handle slave; }; -struct acpi_device_bus_id{ - char bus_id[15]; - unsigned int instance_no; - struct list_head node; -}; - void acpi_scan_lock_acquire(void) { mutex_lock(&acpi_scan_lock); diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index 475c907..f2f9873 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -29,6 +29,7 @@ #include <linux/dynamic_debug.h> #include "internal.h" +#include "sleep.h" #define _COMPONENT ACPI_BUS_COMPONENT ACPI_MODULE_NAME("utils"); @@ -709,6 +710,36 @@ bool acpi_check_dsm(acpi_handle handle, const u8 *uuid, int rev, u64 funcs) } EXPORT_SYMBOL(acpi_check_dsm); +/** + * acpi_dev_present - Detect presence of a given ACPI device in the system. + * @hid: Hardware ID of the device. + * + * Return %true if the device was present at the moment of invocation. + * Note that if the device is pluggable, it may since have disappeared. + * + * For this function to work, acpi_bus_scan() must have been executed + * which happens in the subsys_initcall() subsection. Hence, do not + * call from a subsys_initcall() or earlier (use acpi_get_devices() + * instead). Calling from module_init() is fine (which is synonymous + * with device_initcall()). + */ +bool acpi_dev_present(const char *hid) +{ + struct acpi_device_bus_id *acpi_device_bus_id; + bool found = false; + + mutex_lock(&acpi_device_lock); + list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) + if (!strcmp(acpi_device_bus_id->bus_id, hid)) { + found = true; + break; + } + mutex_unlock(&acpi_device_lock); + + return found; +} +EXPORT_SYMBOL(acpi_dev_present); + /* * acpi_backlight= handling, this is done here rather then in video_detect.c * because __setup cannot be used in modules. diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index ad0a5ff..0fe7bab 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -87,6 +87,8 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const u8 *uuid, int rev, int func, .package.elements = (eles) \ } +bool acpi_dev_present(const char *hid); + #ifdef CONFIG_ACPI #include <linux/proc_fs.h>
There's an idiom in use by 7 Linux drivers to detect the presence of a particular ACPI HID by walking the namespace with acpi_get_devices(). The callback passed to acpi_get_devices() is mostly identical across the drivers, leading to lots of duplicate code. Add acpi_dev_present(), the ACPI equivalent to pci_dev_present(), allowing us to deduplicate all that boilerplate in the drivers. v2: * Instead of walking the namespace, iterate over acpi_bus_id_list which has all HIDs cached (Hanjun Guo, Robert Moore). * Move to utils.c (Rafael J. Wysocki). This necessitates non-static declaration of acpi_bus_id_list. Cc: Hanjun Guo <guohanjun@huawei.com> Cc: Robert Moore <robert.moore@intel.com> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/acpi/internal.h | 8 ++++++++ drivers/acpi/scan.c | 8 +------- drivers/acpi/utils.c | 31 +++++++++++++++++++++++++++++++ include/acpi/acpi_bus.h | 2 ++ 4 files changed, 42 insertions(+), 7 deletions(-)