Add acpi_dev_present

PATCH Superseded Lukas Wunner 8 participants 22 messages Nov 23, 2015 – Nov 24, 2015

Subsystems (8)

Sound ACPI ASoC x86 platform drivers +4 more subsystems

lore

Share & tools

lore

Lukas Wunner

author

2015-11-23 14:34 UTC #0 lore raw

cover Add acpi_dev_present

Hi everyone,

I need to detect the presence of apple-gmux (ACPI HID "APP000B") in several DRM drivers to handle backlight, runtime pm and deferred probing properly.

There's an idiom in use by 7 other drivers to detect presence of a particular ACPI HID which involves calling acpi_get_devices() and passing it a minimal callback that does no further filtering. However this approach results in lots of duplicate code.

This series adds acpi_dev_present(), the ACPI equivalent to pci_dev_present(). It takes a HID string and returns a bool, allowing us to simplify the drivers considerably by removing all the duplicate callbacks and other boilerplate.

I've pushed these patches to GitHub to ease reviewing:
https://github.com/l1k/linux/commits/acpi_dev_present

Should this series find acceptance, it would probably be best if we could agree to merge everything through a single repository, e.g. acpi (git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm). or alternatively alsa (which contains 5 of the 7 drivers involved). Consequently I would like to invite the driver maintainers to kindly provide acks (or raise objections, if any).

The series should not result in any functional changes, however I lack the hardware to actually verify this for every driver involved. I did test it with apple-gmux though.

Thanks,

Lukas

Lukas Wunner (5):
ACPICA: Add acpi_dev_present
eeepc-wmi: Use acpi_dev_present
acer-wmi: Use acpi_dev_present
ALSA: hda - Use acpi_dev_present
ASoC: Intel: Use acpi_dev_present

 drivers/acpi/acpica/nsxfeval.c               | 46 ++++++++++++++++++++++++++++
 drivers/platform/x86/acer-wmi.c              | 16 +++-------
 drivers/platform/x86/eeepc-wmi.c             | 24 ++-------------
 include/acpi/acpixf.h                        |  7 +++++
 sound/pci/hda/thinkpad_helper.c              | 17 ++--------
 sound/soc/intel/atom/sst/sst_acpi.c          | 12 +-------
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 17 ++--------
 sound/soc/intel/boards/cht_bsw_rt5645.c      | 13 +-------
 sound/soc/intel/common/sst-acpi.c            | 12 +-------
 9 files changed, 66 insertions(+), 98 deletions(-)

Signature · 1 line

1.8.5.2 (Apple Git-48)

Lukas Wunner

author

2015-11-23 14:34 UTC #1 lore raw

PATCH 1/5 ACPICA: Add acpi_dev_present

There are 7 drivers which call acpi_get_devices to check for the presence of a particular ACPI HID, each defining its own copy of a mostly identical callback.

Add acpi_dev_present, the ACPI equivalent to pci_dev_present, allowing us to deduplicate all that boilerplate in the drivers.

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/acpi/acpica/nsxfeval.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpixf.h          |  7 +++++++
 2 files changed, 53 insertions(+)

drivers/acpi/acpica/nsxfeval.c +46 -0

index 6ee1e52..19293fa 100644
@@ -828,6 +828,52 @@ ACPI_EXPORT_SYMBOL(acpi_get_devices)
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_ns_dev_present_callback
+ *
+ * PARAMETERS:  Callback from acpi_get_devices
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Minimal callback to be passed to acpi_get_devices which
+ *              performs no further filtering and terminates the search
+ *              immediately.
+ *
+ ******************************************************************************/
+static acpi_status acpi_ns_dev_present_callback(acpi_handle handle, u32 level,
+						void *context, void **retval)
+{
+	*(bool *)context = true;
+	return AE_CTRL_TERMINATE;
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_dev_present
+ *
+ * PARAMETERS:  HID                 - HID to search for.
+ *
+ * RETURNS      True if a matching object of type Device was found.
+ *
+ * DESCRIPTION: Performs a walk of the namespace tree. When a matching object
+ *              of type Device is found, the search is terminated immediately.
+ *
+ ******************************************************************************/
+
+bool
+acpi_dev_present(const char *HID)
+{
+	acpi_status status;
+	bool found = false;
+
+	status = acpi_get_devices(HID, acpi_ns_dev_present_callback, &found,
+				  NULL);
+	return ACPI_SUCCESS(status) && found;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_dev_present)
+
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_attach_data
  *
  * PARAMETERS:  obj_handle          - Namespace node

include/acpi/acpixf.h +7 -0

index 3aaaa86..f299347 100644
@@ -115,6 +115,11 @@
 	prototype;
 #endif
 
+#ifndef ACPI_EXTERNAL_RETURN_BOOL
+#define ACPI_EXTERNAL_RETURN_BOOL(prototype) \
+	prototype;
+#endif
+
 /*****************************************************************************
  *
  * Public globals and runtime configuration options

@@ -483,6 +488,8 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 					      acpi_walk_callback user_function,
 					      void *context,
 					      void **return_value))
+ACPI_EXTERNAL_RETURN_BOOL(bool
+			     acpi_dev_present(const char *HID))
 ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 			     acpi_get_name(acpi_handle object, u32 name_type,
 					   struct acpi_buffer *ret_path_ptr))

Signature · 1 line

1.8.5.2 (Apple Git-48)

Lukas Wunner

author

2015-11-23 14:34 UTC #9 lore raw

PATCH 2/5 eeepc-wmi: Use acpi_dev_present

In reply to Lukas Wunner

Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.

Cc Corentin Chary <corentin.chary(a)gmail.com>

Signed-off-by Lukas Wunner <lukas(a)wunner.de>

---
 drivers/platform/x86/eeepc-wmi.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

drivers/platform/x86/eeepc-wmi.c +2 ~~-22~~

index 14fd2ec..e1490db 100644
@@ -204,30 +204,10 @@ static void eeepc_wmi_key_filter(struct asus_wmi_driver *asus_wmi, int *code,
 	}
 }
 
-static acpi_status eeepc_wmi_parse_device(acpi_handle handle, u32 level,
-						 void *context, void **retval)
-{
-	pr_warn("Found legacy ATKD device (%s)\n", EEEPC_ACPI_HID);
-	*(bool *)context = true;
-	return AE_CTRL_TERMINATE;
-}
-
-static int eeepc_wmi_check_atkd(void)
-{
-	acpi_status status;
-	bool found = false;
-
-	status = acpi_get_devices(EEEPC_ACPI_HID, eeepc_wmi_parse_device,
-				  &found, NULL);
-
-	if (ACPI_FAILURE(status) || !found)
-		return 0;
-	return -1;
-}
-
 static int eeepc_wmi_probe(struct platform_device *pdev)
 {
-	if (eeepc_wmi_check_atkd()) {
+	if (acpi_dev_present(EEEPC_ACPI_HID)) {
+		pr_warn("Found legacy ATKD device (%s)\n", EEEPC_ACPI_HID);
 		pr_warn("WMI device present, but legacy ATKD device is also "
 			"present and enabled\n");
 		pr_warn("You probably booted with acpi_osi=\"Linux\" or "

Signature · 1 line

1.8.5.2 (Apple Git-48)

Lukas Wunner

author

2015-11-23 14:34 UTC #15 lore raw

PATCH 3/5 acer-wmi: Use acpi_dev_present

In reply to Lukas Wunner

Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.

Cc Lee, Chun-Yi <jlee(a)suse.com>

Signed-off-by Lukas Wunner <lukas(a)wunner.de>

---
 drivers/platform/x86/acer-wmi.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

drivers/platform/x86/acer-wmi.c +4 ~~-12~~

index 1062fa4..3deadd5 100644
@@ -793,15 +793,6 @@ static acpi_status __init AMW0_find_mailled(void)
 	return AE_OK;
 }
 
-static int AMW0_set_cap_acpi_check_device_found __initdata;
-
-static acpi_status __init AMW0_set_cap_acpi_check_device_cb(acpi_handle handle,
-	u32 level, void *context, void **retval)
-{
-	AMW0_set_cap_acpi_check_device_found = 1;
-	return AE_OK;
-}
-
 static const struct acpi_device_id norfkill_ids[] __initconst = {
 	{ "VPC2004", 0},
 	{ "IBM0068", 0},

@@ -816,9 +807,10 @@ static int __init AMW0_set_cap_acpi_check_device(void)
 	const struct acpi_device_id *id;
 
 	for (id = norfkill_ids; id->id[0]; id++)
-		acpi_get_devices(id->id, AMW0_set_cap_acpi_check_device_cb,
-				NULL, NULL);
-	return AMW0_set_cap_acpi_check_device_found;
+		if (acpi_dev_present(id->id))
+			return true;
+
+	return false;
 }
 
 static acpi_status __init AMW0_set_capabilities(void)

Signature · 1 line

1.8.5.2 (Apple Git-48)

Lukas Wunner

author

2015-11-23 14:34 UTC #17 lore raw

PATCH 4/5 ALSA: hda - Use acpi_dev_present

In reply to Lukas Wunner

Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.

Cc Hui Wang <hui.wang(a)canonical.com>

Cc Takashi Iwai <tiwai(a)suse.de>

Signed-off-by Lukas Wunner <lukas(a)wunner.de>

---
 sound/pci/hda/thinkpad_helper.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

sound/pci/hda/thinkpad_helper.c +2 ~~-15~~

index 0a4ad5f..59ab6ce 100644
@@ -10,23 +10,10 @@
 static int (*led_set_func)(int, bool);
 static void (*old_vmaster_hook)(void *, int);
 
-static acpi_status acpi_check_cb(acpi_handle handle, u32 lvl, void *context,
-				 void **rv)
-{
-	bool *found = context;
-	*found = true;
-	return AE_OK;
-}
-
 static bool is_thinkpad(struct hda_codec *codec)
 {
-	bool found = false;
-	if (codec->core.subsystem_id >> 16 != 0x17aa)
-		return false;
-	if (ACPI_SUCCESS(acpi_get_devices("LEN0068", acpi_check_cb, &found, NULL)) && found)
-		return true;
-	found = false;
-	return ACPI_SUCCESS(acpi_get_devices("IBM0068", acpi_check_cb, &found, NULL)) && found;
+	return (codec->core.subsystem_id >> 16 == 0x17aa) &&
+	       (acpi_dev_present("LEN0068") || acpi_dev_present("IBM0068"));
 }
 
 static void update_tpacpi_mute_led(void *private_data, int enabled)

Signature · 1 line

1.8.5.2 (Apple Git-48)

Lukas Wunner

author

2015-11-23 14:34 UTC #19 lore raw

PATCH 5/5 ASoC: Intel: Use acpi_dev_present

In reply to Lukas Wunner

Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.

Cc Mark Brown <broonie(a)kernel.org>

Cc Takashi Iwai <tiwai(a)suse.de>

Signed-off-by Lukas Wunner <lukas(a)wunner.de>

---
 sound/soc/intel/atom/sst/sst_acpi.c          | 12 +-----------
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 17 ++---------------
 sound/soc/intel/boards/cht_bsw_rt5645.c      | 13 +------------
 sound/soc/intel/common/sst-acpi.c            | 12 +-----------
 4 files changed, 5 insertions(+), 49 deletions(-)

sound/soc/intel/atom/sst/sst_acpi.c +1 ~~-11~~

index bb19b58..ca12aa5 100644
@@ -223,23 +223,13 @@ static int sst_platform_get_resources(struct intel_sst_drv *ctx)
 	return 0;
 }
 
-static acpi_status sst_acpi_mach_match(acpi_handle handle, u32 level,
-				       void *context, void **ret)
-{
-	*(bool *)context = true;
-	return AE_OK;
-}
-
 static struct sst_machines *sst_acpi_find_machine(
 	struct sst_machines *machines)
 {
 	struct sst_machines *mach;
-	bool found = false;
 
 	for (mach = machines; mach->codec_id; mach++)
-		if (ACPI_SUCCESS(acpi_get_devices(mach->codec_id,
-						  sst_acpi_mach_match,
-						  &found, NULL)) && found)
+		if (acpi_dev_present(mach->codec_id))
 			return mach;
 
 	return NULL;

sound/soc/intel/boards/cht_bsw_max98090_ti.c +2 ~~-15~~

index 4e2fcf1..4036f01 100644
@@ -278,33 +278,20 @@ static struct snd_soc_card snd_soc_card_cht = {
 	.num_controls = ARRAY_SIZE(cht_mc_controls),
 };
 
-static acpi_status snd_acpi_codec_match(acpi_handle handle, u32 level,
-						void *context, void **ret)
-{
-	*(bool *)context = true;
-	return AE_OK;
-}
-
 static int snd_cht_mc_probe(struct platform_device *pdev)
 {
 	int ret_val = 0;
-	bool found = false;
 	struct cht_mc_private *drv;
 
 	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_ATOMIC);
 	if (!drv)
 		return -ENOMEM;
 
-	if (ACPI_SUCCESS(acpi_get_devices(
-					"104C227E",
-					snd_acpi_codec_match,
-					&found, NULL)) && found) {
-		drv->ts3a227e_present = true;
-	} else {
+	drv->ts3a227e_present = acpi_dev_present("104C227E");
+	if (!drv->ts3a227e_present) {
 		/* no need probe TI jack detection chip */
 		snd_soc_card_cht.aux_dev = NULL;
 		snd_soc_card_cht.num_aux_devs = 0;
-		drv->ts3a227e_present = false;
 	}
 
 	/* register the soc card */

sound/soc/intel/boards/cht_bsw_rt5645.c +1 ~~-12~~

index 38d65a3..5e539cd 100644
@@ -324,20 +324,12 @@ static struct cht_acpi_card snd_soc_cards[] = {
 	{"10EC5650", CODEC_TYPE_RT5650, &snd_soc_card_chtrt5650},
 };
 
-static acpi_status snd_acpi_codec_match(acpi_handle handle, u32 level,
-				       void *context, void **ret)
-{
-	*(bool *)context = true;
-	return AE_OK;
-}
-
 static int snd_cht_mc_probe(struct platform_device *pdev)
 {
 	int ret_val = 0;
 	int i;
 	struct cht_mc_private *drv;
 	struct snd_soc_card *card = snd_soc_cards[0].soc_card;
-	bool found = false;
 	char codec_name[16];
 
 	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_ATOMIC);

@@ -345,10 +337,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(snd_soc_cards); i++) {
-		if (ACPI_SUCCESS(acpi_get_devices(
-						snd_soc_cards[i].codec_id,
-						snd_acpi_codec_match,
-						&found, NULL)) && found) {
+		if (acpi_dev_present(snd_soc_cards[i].codec_id)) {
 			dev_dbg(&pdev->dev,
 				"found codec %s\n", snd_soc_cards[i].codec_id);
 			card = snd_soc_cards[i].soc_card;

sound/soc/intel/common/sst-acpi.c +1 ~~-11~~

index 67b6d3d..bc14826 100644
@@ -88,23 +88,13 @@ static void sst_acpi_fw_cb(const struct firmware *fw, void *context)
 	return;
 }
 
-static acpi_status sst_acpi_mach_match(acpi_handle handle, u32 level,
-				       void *context, void **ret)
-{
-	*(bool *)context = true;
-	return AE_OK;
-}
-
 static struct sst_acpi_mach *sst_acpi_find_machine(
 	struct sst_acpi_mach *machines)
 {
 	struct sst_acpi_mach *mach;
-	bool found = false;
 
 	for (mach = machines; mach->id[0]; mach++)
-		if (ACPI_SUCCESS(acpi_get_devices(mach->id,
-						  sst_acpi_mach_match,
-						  &found, NULL)) && found)
+		if (acpi_dev_present(mach->id))
 			return mach;
 
 	return NULL;

Signature · 1 line

1.8.5.2 (Apple Git-48)

Mark Brown

maintainer

2015-11-23 14:48 UTC #20 lore raw

Re: [PATCH 5/5] ASoC: Intel: Use acpi_dev_present

Quoted 2 lines · Lukas Wunner

Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.

This will collide with some other work done on the Intel code in -next I expect, probably best to merge this via ASoC (so pulling a shared branch for the new API) or just wait till 4.5 to do the conversion.

Darren Hart

Acked-by on v1 2/5

2015-11-23 19:04 UTC #10 lore raw

Re: [PATCH 2/5] eeepc-wmi: Use acpi_dev_present

In reply to Lukas Wunner

Quoted 2 lines · Lukas Wunner

Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.

You did add a pr_warn, which is technically a functional change. Did you intend to leave that in?

I have no objection for platform-drivers-x86, but please include all maintainers listed by get_maintainer.pl for the fastest response.

+Rafael

Rafael, I assume you will pick this up along with the acpi_dev_present ACPI change if you take that. Pleaes let me know if not.

Otherwise,

Acked-by Darren Hart <[email protected]>

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Darren Hart

Acked-by on v1 3/5

2015-11-23 19:06 UTC #16 lore raw

Re: [PATCH 3/5] acer-wmi: Use acpi_dev_present

In reply to Lukas Wunner

Quoted 5 lines · Lukas Wunner

Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.
Cc Lee, Chun-Yi <[email protected]>
Signed-off-by Lukas Wunner <[email protected]>

Rafael, I presume you'll pick this one up as well?

For platform-drivers-x86:

Acked-by Darren Hart <[email protected]>

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Lukas Wunner

author

2015-11-23 19:55 UTC #11 lore raw

Re: [Devel] [PATCH 2/5] eeepc-wmi: Use acpi_dev_present

In reply to Darren Hart

Hi Darren,

Quoted 6 lines · Darren Hart

Quoted 2 lines · Lukas Wunner
Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.
You did add a pr_warn, which is technically a functional change. Did you intend to leave that in?

That's not an addition of mine, I moved it from eeepc_wmi_parse_device() to eeepc_wmi_probe() so that everything behaves exactly as before. (See the deleted lines further up in the patch.)

Quoted 2 lines

Rafael, I assume you will pick this up along with the acpi_dev_present ACPI change if you take that. Pleaes let me know if not.

The last patch in the series concerned Intel ASoC (sound/soc/intel/) and the maintainer Mark Brown <broonie(a)kernel.org> has replied that "This will collide with some other work done on the Intel code in -next I expect, probably best to merge this via ASoC (so pulling a shared branch for the new API) or just wait till 4.5 to do the conversion." [full quote of his message as it wasn't cc: platform-driver-x86]

So I guess Mark might pull it in, haven't heard back from the ACPI maintainers yet.

Quoted 2 lines

Otherwise,
Acked-by: Darren Hart <dvhart(a)linux.intel.com>

Awesome, thanks!

Best regards,

Lukas

Darren Hart

2015-11-23 21:00 UTC #12 lore raw

Re: [PATCH 2/5] eeepc-wmi: Use acpi_dev_present

Quoted 7 lines · Lukas Wunner

Hi Darren,
Quoted 4 lines · Darren Hart
Quoted 2 lines · Lukas Wunner
Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.

Quoted 6 lines

Quoted 2 lines
You did add a pr_warn, which is technically a functional change. Did you intend to leave that in?
That's not an addition of mine, I moved it from eeepc_wmi_parse_device() to eeepc_wmi_probe() so that everything behaves exactly as before. (See the deleted lines further up in the patch.)

Ah, apologies.

Quoted 12 lines

Quoted 4 lines
Rafael, I assume you will pick this up along with the acpi_dev_present ACPI change if you take that. Pleaes let me know if not.
The last patch in the series concerned Intel ASoC (sound/soc/intel/) and the maintainer Mark Brown <[email protected]> has replied that "This will collide with some other work done on the Intel code in -next I expect, probably best to merge this via ASoC (so pulling a shared branch for the new API) or just wait till 4.5 to do the conversion." [full quote of his message as it wasn't cc: platform-driver-x86]

Quoted 3 lines

So I guess Mark might pull it in, haven't heard back from the ACPI maintainers yet.

I'll defer to Rafael's preference here.

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Rafael J. Wysocki

2015-11-23 22:05 UTC #2 lore raw

Re: [PATCH 1/5] ACPICA: Add acpi_dev_present

In reply to Lukas Wunner

Quoted 8 lines · Lukas Wunner

There are 7 drivers which call acpi_get_devices to check for the presence of a particular ACPI HID, each defining its own copy of a mostly identical callback.
Add acpi_dev_present, the ACPI equivalent to pci_dev_present, allowing us to deduplicate all that boilerplate in the drivers.
Signed-off-by Lukas Wunner <[email protected]>

We have a general rule for ACPICA patches that they go in through upstream ACPICA.

So if you really want this changes in ACPICA, this is the way to handle it.

Now, of course, for that to happen, you should have CCed the ACPICA maintainers too (now done).

Thanks,
Rafael

Quoted 88 lines

---
 drivers/acpi/acpica/nsxfeval.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpixf.h          |  7 +++++++
 2 files changed, 53 insertions(+)

drivers/acpi/acpica/nsxfeval.c +46 -0

index 6ee1e52..19293fa 100644
@@ -828,6 +828,52 @@ ACPI_EXPORT_SYMBOL(acpi_get_devices)
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_ns_dev_present_callback
+ *
+ * PARAMETERS:  Callback from acpi_get_devices
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Minimal callback to be passed to acpi_get_devices which
+ *              performs no further filtering and terminates the search
+ *              immediately.
+ *
+ ******************************************************************************/
+static acpi_status acpi_ns_dev_present_callback(acpi_handle handle, u32 level,
+						void *context, void **retval)
+{
+	*(bool *)context = true;
+	return AE_CTRL_TERMINATE;
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_dev_present
+ *
+ * PARAMETERS:  HID                 - HID to search for.
+ *
+ * RETURNS      True if a matching object of type Device was found.
+ *
+ * DESCRIPTION: Performs a walk of the namespace tree. When a matching object
+ *              of type Device is found, the search is terminated immediately.
+ *
+ ******************************************************************************/
+
+bool
+acpi_dev_present(const char *HID)
+{
+	acpi_status status;
+	bool found = false;
+
+	status = acpi_get_devices(HID, acpi_ns_dev_present_callback, &found,
+				  NULL);
+	return ACPI_SUCCESS(status) && found;
+}
+
+ACPI_EXPORT_SYMBOL(acpi_dev_present)
+
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_attach_data
  *
  * PARAMETERS:  obj_handle          - Namespace node

include/acpi/acpixf.h +7 -0

index 3aaaa86..f299347 100644
@@ -115,6 +115,11 @@
 	prototype;
 #endif
 
+#ifndef ACPI_EXTERNAL_RETURN_BOOL
+#define ACPI_EXTERNAL_RETURN_BOOL(prototype) \
+	prototype;
+#endif
+
 /*****************************************************************************
  *
  * Public globals and runtime configuration options

@@ -483,6 +488,8 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 					      acpi_walk_callback user_function,
 					      void *context,
 					      void **return_value))
+ACPI_EXTERNAL_RETURN_BOOL(bool
+			     acpi_dev_present(const char *HID))
 ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 			     acpi_get_name(acpi_handle object, u32 name_type,
 					   struct acpi_buffer *ret_path_ptr))

Rafael J. Wysocki

2015-11-23 22:06 UTC #13 lore raw

Re: [PATCH 2/5] eeepc-wmi: Use acpi_dev_present

In reply to Darren Hart

Quoted 10 lines · Darren Hart

Quoted 2 lines · Lukas Wunner
Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.
You did add a pr_warn, which is technically a functional change. Did you intend to leave that in?
I have no objection for platform-drivers-x86, but please include all maintainers listed by get_maintainer.pl for the fastest response.

Quoted 8 lines

+Rafael
Rafael, I assume you will pick this up along with the acpi_dev_present ACPI change if you take that. Pleaes let me know if not.
Otherwise,
Acked-by Darren Hart <[email protected]>

In fact, patch [1/5] as is needs to go in through upstream ACPICA, so I'm not taking this series and please don't take it too.

Thanks,
Rafael

Rafael J. Wysocki

2015-11-23 22:07 UTC #21 lore raw

Re: [PATCH 5/5] ASoC: Intel: Use acpi_dev_present

In reply to Mark Brown

Quoted 7 lines · Mark Brown

Quoted 2 lines · Lukas Wunner
Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.
This will collide with some other work done on the Intel code in -next I expect, probably best to merge this via ASoC (so pulling a shared branch for the new API) or just wait till 4.5 to do the conversion.

If patch [1/5] is done the way it is, then it needs to go to upstream ACPICA first and from there to Linux, so I guess no worries for now. :-)

Thanks,
Rafael

Moore

2015-11-23 22:22 UTC #3 lore raw

Re: [Devel] [PATCH 1/5] ACPICA: Add acpi_dev_present

In reply to Rafael J. Wysocki

Quoted 1 line

Quoted 1 line
acpi_dev_present

Do you really want to be walking the ACPICA namespace for every call?

Quoted 133 lines

-----Original Message-----
From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Monday, November 23, 2015 2:35 PM
To: Lukas Wunner
Cc: [email protected]; [email protected]; platform-driver- [email protected]; Darren Hart; Corentin Chary; Lee, Chun-Yi; alsa- [email protected]; Takashi Iwai; Hui Wang; Mark Brown; Zheng, Lv; Moore, Robert; David Box
Subject: Re: [PATCH 1/5] ACPICA: Add acpi_dev_present
Quoted 8 lines · Lukas Wunner
There are 7 drivers which call acpi_get_devices to check for the presence of a particular ACPI HID, each defining its own copy of a mostly identical callback.
Add acpi_dev_present, the ACPI equivalent to pci_dev_present, allowing us to deduplicate all that boilerplate in the drivers.
Signed-off-by Lukas Wunner <[email protected]>
We have a general rule for ACPICA patches that they go in through upstream ACPICA.
So if you really want this changes in ACPICA, this is the way to handle it.
Now, of course, for that to happen, you should have CCed the ACPICA maintainers too (now done).
Thanks,
Rafael
Quoted 2 lines
---
 drivers/acpi/acpica/nsxfeval.c | 46
++++++++++++++++++++++++++++++++++++++++++
drivers/acpi/acpica/nsxfeval.c ACPI_EXPORT_SYMBOL(acpi_get_devices) · line 829
Quoted 10 lines
 include/acpi/acpixf.h          |  7 +++++++
 2 files changed, 53 insertions(+)
(quoted diff) +0 -0
b/drivers/acpi/acpica/nsxfeval.c index 6ee1e52..19293fa 100644
drivers/acpi/acpica/nsxfeval.c +0 -0
@@ -828,6 +828,52 @@ ACPI_EXPORT_SYMBOL(acpi_get_devices)
 
 
/*************************************************************************
******
Quoted 21 lines
drivers/acpi/acpica/nsxfeval.c +20 -0
  *
+ * FUNCTION:    acpi_ns_dev_present_callback
+ *
+ * PARAMETERS:  Callback from acpi_get_devices
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Minimal callback to be passed to acpi_get_devices which
+ *              performs no further filtering and terminates the search
+ *              immediately.
+ *
+
+*********************************************************************
+*********/ static acpi_status
+acpi_ns_dev_present_callback(acpi_handle handle, u32 level,
+						void *context, void **retval)
+{
+	*(bool *)context = true;
+	return AE_CTRL_TERMINATE;
+}
+
drivers/acpi/acpica/nsxfeval.c ACPI_EXPORT_SYMBOL(acpi_get_devices)
Quoted 10 lines
drivers/acpi/acpica/nsxfeval.c +10 -0
+/********************************************************************
+***********
+ *
+ * FUNCTION:    acpi_dev_present
+ *
+ * PARAMETERS:  HID                 - HID to search for.
+ *
+ * RETURNS      True if a matching object of type Device was found.
+ *
+ * DESCRIPTION: Performs a walk of the namespace tree. When a matching
object
drivers/acpi/acpica/nsxfeval.c ACPI_EXPORT_SYMBOL(acpi_get_devices) · line 861
Quoted 1 line
drivers/acpi/acpica/nsxfeval.c +1 -0
+ *              of type Device is found, the search is terminated
immediately.
Quoted 28 lines
drivers/acpi/acpica/nsxfeval.c +20 -0
+ *
+
+*********************************************************************
+*********/
+
+bool
+acpi_dev_present(const char *HID)
+{
+	acpi_status status;
+	bool found = false;
+
+	status = acpi_get_devices(HID, acpi_ns_dev_present_callback, &found,
+				  NULL);
+	return ACPI_SUCCESS(status) && found; }
+
+ACPI_EXPORT_SYMBOL(acpi_dev_present)
+
+/********************************************************************
+***********
+ *
  * FUNCTION:    acpi_attach_data
  *
  * PARAMETERS:  obj_handle          - Namespace node
include/acpi/acpixf.h index renamed from include/acpi/acpixf.h +0 -0
3aaaa86..f299347 100644
include/acpi/acpixf.h line 123
Quoted 9 lines
include/acpi/acpixf.h +5 -0
@@ -115,6 +115,11 @@
 	prototype;
 #endif
 
+#ifndef ACPI_EXTERNAL_RETURN_BOOL
+#define ACPI_EXTERNAL_RETURN_BOOL(prototype) \
+	prototype;
+#endif
+
 
/*************************************************************************
****
Quoted 12 lines
include/acpi/acpixf.h +0 -0
  *
  * Public globals and runtime configuration options @@ -483,6 +488,8
@@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status
include/acpi/acpixf.h +2 -0
 					      acpi_walk_callback user_function,
 					      void *context,
 					      void **return_value))
+ACPI_EXTERNAL_RETURN_BOOL(bool
+			     acpi_dev_present(const char *HID))
 ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 			     acpi_get_name(acpi_handle object, u32 name_type,
 					   struct acpi_buffer *ret_path_ptr))

Lukas Wunner

author

2015-11-23 23:32 UTC #4 lore raw

Re: [Devel] [PATCH 1/5] ACPICA: Add acpi_dev_present

Hi Robert,

Quoted 3 lines · Robert

Quoted 1 line
Quoted 1 line
acpi_dev_present
Do you really want to be walking the ACPICA namespace for every call?

That's what the drivers currently do. Typically this is called only once on initialization by the driver's ->probe callback.

What did you have in mind instead, cache the result? Or store the HIDs in the namespace in a hash that can be queried faster?

Sorry for not cc'ing you right away, if you want to take a look at the patches for the drivers they're browsable on GitHub or in the acpica.org mailing list archive:
https://github.com/l1k/linux/commits/acpi_dev_present
https://lists.acpica.org/pipermail/devel/2015-November/000851.html

Patches [0/5] and [1/5] are not visible in the mailing list archive:
They landed in the moderation queue because they had too many recipients. I guess if I had cc'ed you all, none of the patches would have come through.

Thanks,

Lukas

Quoted 135 lines

Quoted 134 lines
-----Original Message-----
From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Monday, November 23, 2015 2:35 PM
To: Lukas Wunner
Cc: [email protected]; [email protected]; platform-driver- [email protected]; Darren Hart; Corentin Chary; Lee, Chun-Yi; alsa- [email protected]; Takashi Iwai; Hui Wang; Mark Brown; Zheng, Lv; Moore, Robert; David Box
Subject: Re: [PATCH 1/5] ACPICA: Add acpi_dev_present
Quoted 8 lines · Lukas Wunner
There are 7 drivers which call acpi_get_devices to check for the presence of a particular ACPI HID, each defining its own copy of a mostly identical callback.
Add acpi_dev_present, the ACPI equivalent to pci_dev_present, allowing us to deduplicate all that boilerplate in the drivers.
Signed-off-by Lukas Wunner <[email protected]>
We have a general rule for ACPICA patches that they go in through upstream ACPICA.
So if you really want this changes in ACPICA, this is the way to handle it.
Now, of course, for that to happen, you should have CCed the ACPICA maintainers too (now done).
Thanks,
Rafael
Quoted 2 lines
---
 drivers/acpi/acpica/nsxfeval.c | 46
++++++++++++++++++++++++++++++++++++++++++
drivers/acpi/acpica/nsxfeval.c ACPI_EXPORT_SYMBOL(acpi_get_devices) · line 829
Quoted 10 lines
 include/acpi/acpixf.h          |  7 +++++++
 2 files changed, 53 insertions(+)
(quoted diff) +0 -0
b/drivers/acpi/acpica/nsxfeval.c index 6ee1e52..19293fa 100644
drivers/acpi/acpica/nsxfeval.c +0 -0
@@ -828,6 +828,52 @@ ACPI_EXPORT_SYMBOL(acpi_get_devices)
 
 
/*************************************************************************
******
Quoted 21 lines
drivers/acpi/acpica/nsxfeval.c +20 -0
  *
+ * FUNCTION:    acpi_ns_dev_present_callback
+ *
+ * PARAMETERS:  Callback from acpi_get_devices
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Minimal callback to be passed to acpi_get_devices which
+ *              performs no further filtering and terminates the search
+ *              immediately.
+ *
+
+*********************************************************************
+*********/ static acpi_status
+acpi_ns_dev_present_callback(acpi_handle handle, u32 level,
+						void *context, void **retval)
+{
+	*(bool *)context = true;
+	return AE_CTRL_TERMINATE;
+}
+
drivers/acpi/acpica/nsxfeval.c ACPI_EXPORT_SYMBOL(acpi_get_devices)
Quoted 10 lines
drivers/acpi/acpica/nsxfeval.c +10 -0
+/********************************************************************
+***********
+ *
+ * FUNCTION:    acpi_dev_present
+ *
+ * PARAMETERS:  HID                 - HID to search for.
+ *
+ * RETURNS      True if a matching object of type Device was found.
+ *
+ * DESCRIPTION: Performs a walk of the namespace tree. When a matching
object
drivers/acpi/acpica/nsxfeval.c ACPI_EXPORT_SYMBOL(acpi_get_devices) · line 861
Quoted 1 line
drivers/acpi/acpica/nsxfeval.c +1 -0
+ *              of type Device is found, the search is terminated
immediately.
Quoted 28 lines
drivers/acpi/acpica/nsxfeval.c +20 -0
+ *
+
+*********************************************************************
+*********/
+
+bool
+acpi_dev_present(const char *HID)
+{
+	acpi_status status;
+	bool found = false;
+
+	status = acpi_get_devices(HID, acpi_ns_dev_present_callback, &found,
+				  NULL);
+	return ACPI_SUCCESS(status) && found; }
+
+ACPI_EXPORT_SYMBOL(acpi_dev_present)
+
+/********************************************************************
+***********
+ *
  * FUNCTION:    acpi_attach_data
  *
  * PARAMETERS:  obj_handle          - Namespace node
include/acpi/acpixf.h index renamed from include/acpi/acpixf.h +0 -0
3aaaa86..f299347 100644
include/acpi/acpixf.h line 123
Quoted 9 lines
include/acpi/acpixf.h +5 -0
@@ -115,6 +115,11 @@
 	prototype;
 #endif
 
+#ifndef ACPI_EXTERNAL_RETURN_BOOL
+#define ACPI_EXTERNAL_RETURN_BOOL(prototype) \
+	prototype;
+#endif
+
 
/*************************************************************************
****
Quoted 12 lines
include/acpi/acpixf.h +0 -0
  *
  * Public globals and runtime configuration options @@ -483,6 +488,8
@@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status
include/acpi/acpixf.h +2 -0
 					      acpi_walk_callback user_function,
 					      void *context,
 					      void **return_value))
+ACPI_EXTERNAL_RETURN_BOOL(bool
+			     acpi_dev_present(const char *HID))
 ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 			     acpi_get_name(acpi_handle object, u32 name_type,
 					   struct acpi_buffer *ret_path_ptr))

Hui Wang

Acked-by on v1 4/5

2015-11-24 01:51 UTC #18 lore raw

Re: [alsa-devel] [PATCH 4/5] ALSA: hda - Use acpi_dev_present

In reply to Lukas Wunner

Quoted 4 lines · Lukas Wunner

Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.
Cc Hui Wang <[email protected]>

The [PATCH 4/5] looks fine to me.

Acked-by Hui Wang <[email protected]>

Quoted 36 lines

Cc Takashi Iwai <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
  sound/pci/hda/thinkpad_helper.c | 17 ++---------------
  1 file changed, 2 insertions(+), 15 deletions(-)

sound/pci/hda/thinkpad_helper.c +2 ~~-15~~

index 0a4ad5f..59ab6ce 100644
@@ -10,23 +10,10 @@
  static int (*led_set_func)(int, bool);
  static void (*old_vmaster_hook)(void *, int);
 
-static acpi_status acpi_check_cb(acpi_handle handle, u32 lvl, void *context,
-				 void **rv)
-{
-	bool *found = context;
-	*found = true;
-	return AE_OK;
-}
-
  static bool is_thinkpad(struct hda_codec *codec)
  {
-	bool found = false;
-	if (codec->core.subsystem_id >> 16 != 0x17aa)
-		return false;
-	if (ACPI_SUCCESS(acpi_get_devices("LEN0068", acpi_check_cb, &found, NULL)) && found)
-		return true;
-	found = false;
-	return ACPI_SUCCESS(acpi_get_devices("IBM0068", acpi_check_cb, &found, NULL)) && found;
+	return (codec->core.subsystem_id >> 16 == 0x17aa) &&
+	       (acpi_dev_present("LEN0068") || acpi_dev_present("IBM0068"));
  }
 
  static void update_tpacpi_mute_led(void *private_data, int enabled)

Hanjun Guo

2015-11-24 04:40 UTC #5 lore raw

Re: [PATCH 1/5] ACPICA: Add acpi_dev_present

In reply to Lukas Wunner

Quoted 10 lines · Lukas Wunner

Hi Robert,
Quoted 2 lines · Robert
Quoted 1 line
Quoted 1 line
acpi_dev_present
Do you really want to be walking the ACPICA namespace for every call?
That's what the drivers currently do. Typically this is called only once on initialization by the driver's ->probe callback.
What did you have in mind instead, cache the result? Or store the HIDs in the namespace in a hash that can be queried faster?

Will those drivers be loaded before the acpi namespace is scanned? if not, I think those IDs already cached, in acpi_init_device_object(),

INIT_LIST_HEAD(&device->pnp.ids);
...
acpi_set_pnp_ids(handle, &device->pnp, type);

please see API acpi_device_hid(), so I think you can introduce a API with acpi_device and HID passed as arguments in scan.c

Thanks
Hanjun

Rafael J. Wysocki

2015-11-24 13:52 UTC #6 lore raw

Re: [PATCH 1/5] ACPICA: Add acpi_dev_present

Quoted 15 lines · Hanjun Guo

Quoted 10 lines · Lukas Wunner
Hi Robert,
Quoted 2 lines · Robert
Quoted 1 line
Quoted 1 line
acpi_dev_present
Do you really want to be walking the ACPICA namespace for every call?
That's what the drivers currently do. Typically this is called only once on initialization by the driver's ->probe callback.
What did you have in mind instead, cache the result? Or store the HIDs in the namespace in a hash that can be queried faster?
Will those drivers be loaded before the acpi namespace is scanned? if not, I think those IDs already cached, in acpi_init_device_object(),

Quoted 6 lines

INIT_LIST_HEAD(&device->pnp.ids);
...
acpi_set_pnp_ids(handle, &device->pnp, type);
please see API acpi_device_hid(), so I think you can introduce a API with acpi_device and HID passed as arguments in scan.c

I'd prefer that to go to utils.c to be honest, even if the namespace needs to be walked.

Thanks,
Rafael

Moore

2015-11-24 14:15 UTC #8 lore raw

Re: [Devel] [PATCH 1/5] ACPICA: Add acpi_dev_present

In reply to Hanjun Guo

Quoted 18 lines

-----Original Message-----
From: Hanjun Guo [mailto:[email protected]]
Sent: Monday, November 23, 2015 8:41 PM
To: Lukas Wunner; Moore, Robert
Cc: Rafael J. Wysocki; [email protected]; [email protected]; [email protected]; Darren Hart; Corentin Chary; Lee, Chun-Yi; [email protected]; Takashi Iwai; Hui Wang; Mark Brown; Zheng, Lv; David Box
Subject: Re: [PATCH 1/5] ACPICA: Add acpi_dev_present
Quoted 5 lines · Lukas Wunner
Hi Robert,
Quoted 2 lines · Robert
Quoted 1 line
Quoted 1 line
acpi_dev_present
Do you really want to be walking the ACPICA namespace for every call?

Quoted 8 lines

Quoted 5 lines
That's what the drivers currently do. Typically this is called only once on initialization by the driver's ->probe callback.
What did you have in mind instead, cache the result? Or store the HIDs in the namespace in a hash that can be queried faster?
Will those drivers be loaded before the acpi namespace is scanned? if not, I think those IDs already cached, in acpi_init_device_object(),

I was thinking about something like this. Traversing the namespace over and over is truly brute force.

Quoted 10 lines

INIT_LIST_HEAD(&device->pnp.ids);
...
acpi_set_pnp_ids(handle, &device->pnp, type);
please see API acpi_device_hid(), so I think you can introduce a API with acpi_device and HID passed as arguments in scan.c
Thanks
Hanjun

Hanjun Guo

2015-11-24 14:54 UTC #7 lore raw

Re: [Devel] [PATCH 1/5] ACPICA: Add acpi_dev_present

In reply to Rafael J. Wysocki

Quoted 20 lines · Rafael J. Wysocki

Quoted 19 lines · Hanjun Guo
Quoted 10 lines · Lukas Wunner
Hi Robert,
Quoted 2 lines · Robert
Quoted 1 line
Quoted 1 line
acpi_dev_present
Do you really want to be walking the ACPICA namespace for every call?
That's what the drivers currently do. Typically this is called only once on initialization by the driver's ->probe callback.
What did you have in mind instead, cache the result? Or store the HIDs in the namespace in a hash that can be queried faster?
Will those drivers be loaded before the acpi namespace is scanned? if not, I think those IDs already cached, in acpi_init_device_object(),
INIT_LIST_HEAD(&device->pnp.ids);
...
acpi_set_pnp_ids(handle, &device->pnp, type);

Quoted 5 lines

Quoted 2 lines
please see API acpi_device_hid(), so I think you can introduce a API with acpi_device and HID passed as arguments in scan.c
I'd prefer that to go to utils.c to be honest, even if the namespace needs to be walked.

I agree, utils.c is the better place to go.

Thanks
Hanjun

Darren Hart

2015-11-24 17:08 UTC #14 lore raw

Re: [PATCH 2/5] eeepc-wmi: Use acpi_dev_present

In reply to Rafael J. Wysocki

Quoted 16 lines · Rafael Wysocki

Quoted 15 lines · Darren Hart
Quoted 2 lines · Lukas Wunner
Use shiny new acpi_dev_present and remove all the boilerplate to search for a particular ACPI device. No functional change.
You did add a pr_warn, which is technically a functional change. Did you intend to leave that in?
I have no objection for platform-drivers-x86, but please include all maintainers listed by get_maintainer.pl for the fastest response.
+Rafael
Rafael, I assume you will pick this up along with the acpi_dev_present ACPI change if you take that. Pleaes let me know if not.

Quoted 6 lines

Quoted 3 lines
Otherwise,
Acked-by Darren Hart <[email protected]>
In fact, patch [1/5] as is needs to go in through upstream ACPICA, so I'm not taking this series and please don't take it too.

Ah, thanks, my mistake.

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center