[PATCH 0/5] Add acpi_dev_present

[PATCH 0/5] Add acpi_dev_present

[Lukas Wunner]

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(-)

-- 
1.8.5.2 (Apple Git-48)

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

[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@kernel.org>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Lukas Wunner <lukas@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(-)

diff --git a/sound/soc/intel/atom/sst/sst_acpi.c b/sound/soc/intel/atom/sst/sst_acpi.c
index bb19b58..ca12aa5 100644
--- a/sound/soc/intel/atom/sst/sst_acpi.c
+++ b/sound/soc/intel/atom/sst/sst_acpi.c
@@ -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;
diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
index 4e2fcf1..4036f01 100644
--- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c
+++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
@@ -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 */
diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
index 38d65a3..5e539cd 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -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;
diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c
index 67b6d3d..bc14826 100644
--- a/sound/soc/intel/common/sst-acpi.c
+++ b/sound/soc/intel/common/sst-acpi.c
@@ -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;
-- 
1.8.5.2 (Apple Git-48)

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 398 bytes --]

On Mon, Nov 23, 2015 at 03:34:55PM +0100, Lukas Wunner wrote:
> 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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

On Monday, November 23, 2015 02:48:36 PM Mark Brown wrote:
> On Mon, Nov 23, 2015 at 03:34:55PM +0100, Lukas Wunner wrote:
> > 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[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@canonical.com>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 sound/pci/hda/thinkpad_helper.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index 0a4ad5f..59ab6ce 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -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)
-- 
1.8.5.2 (Apple Git-48)

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

[Hui Wang]

On 11/23/2015 10:34 PM, Lukas Wunner wrote:
> 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@canonical.com>
The [PATCH 4/5] looks fine to me.

Acked-by: Hui Wang <hui.wang@canonical.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>   sound/pci/hda/thinkpad_helper.c | 17 ++---------------
>   1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
> index 0a4ad5f..59ab6ce 100644
> --- a/sound/pci/hda/thinkpad_helper.c
> +++ b/sound/pci/hda/thinkpad_helper.c
> @@ -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)

[PATCH 1/5] ACPICA: Add acpi_dev_present

[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 <lukas@wunner.de>
---
 drivers/acpi/acpica/nsxfeval.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpixf.h          |  7 +++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/acpi/acpica/nsxfeval.c b/drivers/acpi/acpica/nsxfeval.c
index 6ee1e52..19293fa 100644
--- a/drivers/acpi/acpica/nsxfeval.c
+++ b/drivers/acpi/acpica/nsxfeval.c
@@ -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
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 3aaaa86..f299347 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -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))
-- 
1.8.5.2 (Apple Git-48)

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

[Rafael J. Wysocki]

On Monday, November 23, 2015 03:34:55 PM Lukas Wunner wrote:
> 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 <lukas@wunner.de>

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


> ---
>  drivers/acpi/acpica/nsxfeval.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpixf.h          |  7 +++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/acpi/acpica/nsxfeval.c b/drivers/acpi/acpica/nsxfeval.c
> index 6ee1e52..19293fa 100644
> --- a/drivers/acpi/acpica/nsxfeval.c
> +++ b/drivers/acpi/acpica/nsxfeval.c
> @@ -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
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 3aaaa86..f299347 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -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))
> 

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

[Moore, Robert]

>>acpi_dev_present

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

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Monday, November 23, 2015 2:35 PM
> To: Lukas Wunner
> Cc: linux-acpi@vger.kernel.org; devel@acpica.org; platform-driver-
> x86@vger.kernel.org; Darren Hart; Corentin Chary; Lee, Chun-Yi; alsa-
> devel@alsa-project.org; Takashi Iwai; Hui Wang; Mark Brown; Zheng, Lv;
> Moore, Robert; David Box
> Subject: Re: [PATCH 1/5] ACPICA: Add acpi_dev_present
> 
> On Monday, November 23, 2015 03:34:55 PM Lukas Wunner wrote:
> > 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 <lukas@wunner.de>
> 
> 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
> 
> 
> > ---
> >  drivers/acpi/acpica/nsxfeval.c | 46
> ++++++++++++++++++++++++++++++++++++++++++
> >  include/acpi/acpixf.h          |  7 +++++++
> >  2 files changed, 53 insertions(+)
> >
> > diff --git a/drivers/acpi/acpica/nsxfeval.c
> > b/drivers/acpi/acpica/nsxfeval.c index 6ee1e52..19293fa 100644
> > --- a/drivers/acpi/acpica/nsxfeval.c
> > +++ b/drivers/acpi/acpica/nsxfeval.c
> > @@ -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
> > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index
> > 3aaaa86..f299347 100644
> > --- a/include/acpi/acpixf.h
> > +++ b/include/acpi/acpixf.h
> > @@ -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))
> >

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

[Lukas Wunner]

Hi Robert,

On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote:
> >>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

> 
> > -----Original Message-----
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Sent: Monday, November 23, 2015 2:35 PM
> > To: Lukas Wunner
> > Cc: linux-acpi@vger.kernel.org; devel@acpica.org; platform-driver-
> > x86@vger.kernel.org; Darren Hart; Corentin Chary; Lee, Chun-Yi; alsa-
> > devel@alsa-project.org; Takashi Iwai; Hui Wang; Mark Brown; Zheng, Lv;
> > Moore, Robert; David Box
> > Subject: Re: [PATCH 1/5] ACPICA: Add acpi_dev_present
> > 
> > On Monday, November 23, 2015 03:34:55 PM Lukas Wunner wrote:
> > > 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 <lukas@wunner.de>
> > 
> > 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
> > 
> > 
> > > ---
> > >  drivers/acpi/acpica/nsxfeval.c | 46
> > ++++++++++++++++++++++++++++++++++++++++++
> > >  include/acpi/acpixf.h          |  7 +++++++
> > >  2 files changed, 53 insertions(+)
> > >
> > > diff --git a/drivers/acpi/acpica/nsxfeval.c
> > > b/drivers/acpi/acpica/nsxfeval.c index 6ee1e52..19293fa 100644
> > > --- a/drivers/acpi/acpica/nsxfeval.c
> > > +++ b/drivers/acpi/acpica/nsxfeval.c
> > > @@ -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
> > > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index
> > > 3aaaa86..f299347 100644
> > > --- a/include/acpi/acpixf.h
> > > +++ b/include/acpi/acpixf.h
> > > @@ -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))
> > >
> 

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

[Hanjun Guo]

On 2015/11/24 7:32, Lukas Wunner wrote:
> Hi Robert,
>
> On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote:
>>>> 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

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

[Moore, Robert]

> -----Original Message-----
> From: Hanjun Guo [mailto:guohanjun@huawei.com]
> Sent: Monday, November 23, 2015 8:41 PM
> To: Lukas Wunner; Moore, Robert
> Cc: Rafael J. Wysocki; linux-acpi@vger.kernel.org; devel@acpica.org;
> platform-driver-x86@vger.kernel.org; Darren Hart; Corentin Chary; Lee,
> Chun-Yi; alsa-devel@alsa-project.org; Takashi Iwai; Hui Wang; Mark Brown;
> Zheng, Lv; David Box
> Subject: Re: [PATCH 1/5] ACPICA: Add acpi_dev_present
> 
> On 2015/11/24 7:32, Lukas Wunner wrote:
> > Hi Robert,
> >
> > On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote:
> >>>> 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(),


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



> 
> 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

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

[Rafael J. Wysocki]

On Tuesday, November 24, 2015 12:40:51 PM Hanjun Guo wrote:
> On 2015/11/24 7:32, Lukas Wunner wrote:
> > Hi Robert,
> >
> > On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote:
> >>>> 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

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

Thanks,
Rafael

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

[Hanjun Guo]

On 11/24/2015 10:22 PM, Rafael J. Wysocki wrote:
> On Tuesday, November 24, 2015 12:40:51 PM Hanjun Guo wrote:
>> On 2015/11/24 7:32, Lukas Wunner wrote:
>>> Hi Robert,
>>>
>>> On Mon, Nov 23, 2015 at 10:22:27PM +0000, Moore, Robert wrote:
>>>>>> 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
>
> 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