ALSA: hda - Use acpi_dev_present()

PATCH In review Lukas Wunner 3 participants 9 messages Jan 14, 2016 – Feb 25, 2016

Subsystems (2)

Sound ACPI

lore

Share & tools

lore

Lukas Wunner

author

2016-01-14 21:06 UTC #0 lore raw

cover ALSA: hda - Use acpi_dev_present()

Hi Takashi,

the acpi_dev_present() API has now landed in Linus' tree. Thus, after Linus' tree gets merged back into yours, it would be possible to use the API in the Thinkpad hda drivers as per the following patch.

I've also pushed it to GitHub in case anyone prefers perusing it in a browser:
https://github.com/l1k/linux/commit/a1473d726b57eaf97c4de8812c5967603068e261

An ack for this patch was kindly provided by Hui Wang with:
Message-ID: <[email protected]>
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/100962.html

Thanks,

Lukas

Lukas Wunner (1):
ALSA: hda - Use acpi_dev_present()

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

Signature · 1 line

2.1.0

Lukas Wunner

author

2016-01-14 21:06 UTC #1 lore raw

PATCH 1/1 ALSA: hda - Use acpi_dev_present()

Use shiny new acpi_dev_present() and remove all the boilerplate to search for a particular ACPI device. No functional change. Cf. 2d12b6b381ba ("ACPI / utils: Add acpi_dev_present()").

Cc Hui Wang <[email protected]>

Cc Takashi Iwai <[email protected]>

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

Acked-by Hui Wang <[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)

Signature · 1 line

2.1.0

Takashi Iwai

2016-01-15 05:58 UTC #2 lore raw

In reply to Lukas Wunner

Quoted 8 lines · Lukas Wunner

Hi Takashi,
the acpi_dev_present() API has now landed in Linus' tree. Thus, after Linus' tree gets merged back into yours, it would be possible to use the API in the Thinkpad hda drivers as per the following patch.

Quoted 7 lines

I've also pushed it to GitHub in case anyone prefers perusing it in a browser:
https://github.com/l1k/linux/commit/a1473d726b57eaf97c4de8812c5967603068e261
An ack for this patch was kindly provided by Hui Wang with:
Message-ID: <[email protected]>
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/100962.html

A back merge is ugly and I'd like to avoid it.
This is no urgent fix but rather a cleanup, right? If so, I'd postpone this to 4.6.

thanks,

Takashi

Lukas Wunner

author

2016-02-20 22:46 UTC #3 lore raw

Hi Takashi,

Quoted 12 lines · Takashi Iwai

Quoted 11 lines · Lukas Wunner
Hi Takashi,
the acpi_dev_present() API has now landed in Linus' tree. Thus, after Linus' tree gets merged back into yours, it would be possible to use the API in the Thinkpad hda drivers as per the following patch.
I've also pushed it to GitHub in case anyone prefers perusing it in a browser:
https://github.com/l1k/linux/commit/a1473d726b57eaf97c4de8812c5967603068e261

Quoted 7 lines

Quoted 3 lines
An ack for this patch was kindly provided by Hui Wang with:
Message-ID: <[email protected]>
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/100962.html
A back merge is ugly and I'd like to avoid it.
This is no urgent fix but rather a cleanup, right? If so, I'd postpone this to 4.6.

I've noticed this patch isn't in one of your trees, so it looks like it's not queued for 4.6 yet. If there are objections against it please let me know. If there aren't, I'd like to gently remind of the patch's existence.

Thanks,

Lukas

Takashi Iwai

2016-02-21 08:17 UTC #4 lore raw

Quoted 20 lines · Lukas Wunner

Hi Takashi,
Quoted 16 lines · Takashi Iwai
Quoted 14 lines · Lukas Wunner
Hi Takashi,
the acpi_dev_present() API has now landed in Linus' tree. Thus, after Linus' tree gets merged back into yours, it would be possible to use the API in the Thinkpad hda drivers as per the following patch.
I've also pushed it to GitHub in case anyone prefers perusing it in a browser:
https://github.com/l1k/linux/commit/a1473d726b57eaf97c4de8812c5967603068e261
An ack for this patch was kindly provided by Hui Wang with:
Message-ID: <[email protected]>
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/100962.html

Quoted 8 lines

Quoted 3 lines
A back merge is ugly and I'd like to avoid it.
This is no urgent fix but rather a cleanup, right? If so, I'd postpone this to 4.6.
I've noticed this patch isn't in one of your trees, so it looks like it's not queued for 4.6 yet. If there are objections against it please let me know. If there aren't, I'd like to gently remind of the patch's existence.

Sorry for the delay, it's merged now.

thanks,

Takashi

Pierre-Louis Bossart

2016-02-22 14:18 UTC #5 lore raw

Quoted 26 lines · Takashi Iwai

Quoted 24 lines · Lukas Wunner
Hi Takashi,
Quoted 19 lines · Takashi Iwai
Quoted 7 lines · Lukas Wunner
Hi Takashi,
the acpi_dev_present() API has now landed in Linus' tree. Thus, after Linus' tree gets merged back into yours, it would be possible to use the API in the Thinkpad hda drivers as per the following patch.
Quoted 7 lines
I've also pushed it to GitHub in case anyone prefers perusing it in a browser:
https://github.com/l1k/linux/commit/a1473d726b57eaf97c4de8812c5967603068e261
An ack for this patch was kindly provided by Hui Wang with:
Message-ID: <[email protected]>
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/100962.html
A back merge is ugly and I'd like to avoid it.
This is no urgent fix but rather a cleanup, right? If so, I'd postpone this to 4.6.

Quoted 6 lines

Quoted 4 lines
I've noticed this patch isn't in one of your trees, so it looks like it's not queued for 4.6 yet. If there are objections against it please let me know. If there aren't, I'd like to gently remind of the patch's existence.
Sorry for the delay, it's merged now.

heads-up: we've identified that the ACPI subsystem reports devices as present even if they are explicitly disabled in the BIOS _STA routine. we have a couple of WIP patches to work around this issue that is blocking for some CHT-T devices, and they pretty much amount to a revert and addition of an explicit presence test -Pierre

Lukas Wunner

author

2016-02-22 20:22 UTC #6 lore raw

[cc: += Rafael J. Wysocki, Robert Moore, [email protected]]

Hi Pierre-Louis,

Quoted 32 lines · Pierre-Louis Bossart

Quoted 31 lines · Takashi Iwai
Quoted 28 lines · Lukas Wunner
Hi Takashi,
Quoted 12 lines · Takashi Iwai
Quoted 11 lines · Lukas Wunner
Hi Takashi,
the acpi_dev_present() API has now landed in Linus' tree. Thus, after Linus' tree gets merged back into yours, it would be possible to use the API in the Thinkpad hda drivers as per the following patch.
I've also pushed it to GitHub in case anyone prefers perusing it in a browser:
https://github.com/l1k/linux/commit/a1473d726b57eaf97c4de8812c5967603068e261
Quoted 7 lines
Quoted 3 lines
An ack for this patch was kindly provided by Hui Wang with:
Message-ID: <[email protected]>
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/100962.html
A back merge is ugly and I'd like to avoid it.
This is no urgent fix but rather a cleanup, right? If so, I'd postpone this to 4.6.
I've noticed this patch isn't in one of your trees, so it looks like it's not queued for 4.6 yet. If there are objections against it please let me know. If there aren't, I'd like to gently remind of the patch's existence.

Quoted 7 lines

Quoted 1 line
Sorry for the delay, it's merged now.
heads-up: we've identified that the ACPI subsystem reports devices as present even if they are explicitly disabled in the BIOS _STA routine. we have a couple of WIP patches to work around this issue that is blocking for some CHT-T devices, and they pretty much amount to a revert and addition of an explicit presence test

Thanks for the heads-up. I'm confused though that you're sending this in reply to the thinkpad_helper.c patch, I assume this only concerns the ASoC patch?

As you've correctly observed, acpi_dev_present() only checks presence of a HID in the namespace and does not invoke the _STA control method. However the code that it replaced also only checked presence in the namespace, so this is not an issue introduced by my patch but rather one which was present all along.

If you need to check the "device is present" bit returned by _STA, you need a pointer to the struct acpi_device. This will allow you to call acpi_bus_get_status() and check its return value for ACPI_STA_DEVICE_PRESENT.

We cannot easily add this to acpi_dev_present() because that function no longer does an expensive namespace walk but rather a cheap list iteration which does not yield a pointer to the struct acpi_device.

In the first version of acpi_dev_present() I was in fact doing a namespace walk with acpi_get_devices() but Robert Moore objected to that, calling it "truly brute force":
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/101046.html

Hence if possible you should try to avoid that as well. You may want to consider adding a helper to drivers/acpi/utils.c which takes a HID and returns a struct acpi_device*, it might come in handy for others as well.

Best regards,

Lukas

Lukas Wunner

author

2016-02-25 16:58 UTC #7 lore raw

Hi Pierre-Louis, Hi Vinod,

Quoted 65 lines · Lukas Wunner

Quoted 32 lines · Pierre-Louis Bossart
Quoted 31 lines · Takashi Iwai
Quoted 28 lines · Lukas Wunner
Hi Takashi,
Quoted 12 lines · Takashi Iwai
Quoted 11 lines · Lukas Wunner
Hi Takashi,
the acpi_dev_present() API has now landed in Linus' tree. Thus, after Linus' tree gets merged back into yours, it would be possible to use the API in the Thinkpad hda drivers as per the following patch.
I've also pushed it to GitHub in case anyone prefers perusing it in a browser:
https://github.com/l1k/linux/commit/a1473d726b57eaf97c4de8812c5967603068e261
Quoted 7 lines
Quoted 3 lines
An ack for this patch was kindly provided by Hui Wang with:
Message-ID: <[email protected]>
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/100962.html
A back merge is ugly and I'd like to avoid it.
This is no urgent fix but rather a cleanup, right? If so, I'd postpone this to 4.6.
I've noticed this patch isn't in one of your trees, so it looks like it's not queued for 4.6 yet. If there are objections against it please let me know. If there aren't, I'd like to gently remind of the patch's existence.
Quoted 7 lines
Quoted 1 line
Sorry for the delay, it's merged now.
heads-up: we've identified that the ACPI subsystem reports devices as present even if they are explicitly disabled in the BIOS _STA routine. we have a couple of WIP patches to work around this issue that is blocking for some CHT-T devices, and they pretty much amount to a revert and addition of an explicit presence test
Thanks for the heads-up. I'm confused though that you're sending this in reply to the thinkpad_helper.c patch, I assume this only concerns the ASoC patch?
As you've correctly observed, acpi_dev_present() only checks presence of a HID in the namespace and does not invoke the _STA control method. However the code that it replaced also only checked presence in the namespace, so this is not an issue introduced by my patch but rather one which was present all along.
If you need to check the "device is present" bit returned by _STA, you need a pointer to the struct acpi_device. This will allow you to call acpi_bus_get_status() and check its return value for ACPI_STA_DEVICE_PRESENT.
We cannot easily add this to acpi_dev_present() because that function no longer does an expensive namespace walk but rather a cheap list iteration which does not yield a pointer to the struct acpi_device.
In the first version of acpi_dev_present() I was in fact doing a namespace walk with acpi_get_devices() but Robert Moore objected to that, calling it "truly brute force":
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/101046.html

Quoted 4 lines

Hence if possible you should try to avoid that as well. You may want to consider adding a helper to drivers/acpi/utils.c which takes a HID and returns a struct acpi_device*, it might come in handy for others as well.

Forgot to mention:

There's one driver where you currently check for the presence of a specific ACPI device twice. It would probably be good if you could eliminate the second check.

I've explained this in detail in an e-mail to linux-acpi in December but only cc'ed Mark Brown as I wasn't sure who's responsible for the
driver: http://www.spinics.net/lists/linux-acpi/msg62068.html

Quote:

Quoted 10 lines

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

Best regards,

Lukas

Pierre-Louis Bossart

2016-02-25 17:42 UTC #8 lore raw

Quoted 79 lines · Lukas Wunner

Hi Pierre-Louis, Hi Vinod,
Quoted 65 lines · Lukas Wunner
Quoted 32 lines · Pierre-Louis Bossart
Quoted 31 lines · Takashi Iwai
Quoted 28 lines · Lukas Wunner
Hi Takashi,
Quoted 12 lines · Takashi Iwai
Quoted 11 lines · Lukas Wunner
Hi Takashi,
the acpi_dev_present() API has now landed in Linus' tree. Thus, after Linus' tree gets merged back into yours, it would be possible to use the API in the Thinkpad hda drivers as per the following patch.
I've also pushed it to GitHub in case anyone prefers perusing it in a browser:
https://github.com/l1k/linux/commit/a1473d726b57eaf97c4de8812c5967603068e261
Quoted 7 lines
Quoted 3 lines
An ack for this patch was kindly provided by Hui Wang with:
Message-ID: <[email protected]>
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/100962.html
A back merge is ugly and I'd like to avoid it.
This is no urgent fix but rather a cleanup, right? If so, I'd postpone this to 4.6.
I've noticed this patch isn't in one of your trees, so it looks like it's not queued for 4.6 yet. If there are objections against it please let me know. If there aren't, I'd like to gently remind of the patch's existence.
Quoted 7 lines
Quoted 1 line
Sorry for the delay, it's merged now.
heads-up: we've identified that the ACPI subsystem reports devices as present even if they are explicitly disabled in the BIOS _STA routine. we have a couple of WIP patches to work around this issue that is blocking for some CHT-T devices, and they pretty much amount to a revert and addition of an explicit presence test
Thanks for the heads-up. I'm confused though that you're sending this in reply to the thinkpad_helper.c patch, I assume this only concerns the ASoC patch?
As you've correctly observed, acpi_dev_present() only checks presence of a HID in the namespace and does not invoke the _STA control method. However the code that it replaced also only checked presence in the namespace, so this is not an issue introduced by my patch but rather one which was present all along.
If you need to check the "device is present" bit returned by _STA, you need a pointer to the struct acpi_device. This will allow you to call acpi_bus_get_status() and check its return value for ACPI_STA_DEVICE_PRESENT.
We cannot easily add this to acpi_dev_present() because that function no longer does an expensive namespace walk but rather a cheap list iteration which does not yield a pointer to the struct acpi_device.
In the first version of acpi_dev_present() I was in fact doing a namespace walk with acpi_get_devices() but Robert Moore objected to that, calling it "truly brute force":
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-November/101046.html
Quoted 4 lines
Hence if possible you should try to avoid that as well. You may want to consider adding a helper to drivers/acpi/utils.c which takes a HID and returns a struct acpi_device*, it might come in handy for others as well.
Forgot to mention:
There's one driver where you currently check for the presence of a specific ACPI device twice. It would probably be good if you could eliminate the second check.

Quoted 16 lines

I've explained this in detail in an e-mail to linux-acpi in December but only cc'ed Mark Brown as I wasn't sure who's responsible for the
driver: http://www.spinics.net/lists/linux-acpi/msg62068.html
Quote:
Quoted 10 lines
* 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.

Yes we are aware of this.
But again we are not going to use acpi_dev_present() anyway since it just doesn't filter out devices that aren't enabled by the BIOS, we need additional functionality for this.