[alsa-devel] [PATCH 0/1] ALSA: hda - Use acpi_dev_present()

[Pierre-Louis Bossart]

On 2/25/16 10:59 AM, Lukas Wunner wrote:
> Hi Pierre-Louis, Hi Vinod,
>
> On Mon, Feb 22, 2016 at 09:23:31PM +0100, Lukas Wunner wrote:
>> On Mon, Feb 22, 2016 at 08:18:15AM -0600, Pierre-Louis Bossart wrote:
>>> On 02/21/2016 02:17 AM, Takashi Iwai wrote:
>>>> On Sat, 20 Feb 2016 23:47:24 +0100,
>>>> Lukas Wunner wrote:
>>>>>
>>>>> Hi Takashi,
>>>>>
>>>>> On Fri, Jan 15, 2016 at 06:58:19AM +0100, Takashi Iwai wrote:
>>>>>> On Thu, 14 Jan 2016 22:05:03 +0100, Lukas Wunner wrote:
>>>>>>> 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: <5653C291.9090607 at canonical.com>
>>>>>>> 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.
>>>>
>>>> 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.
>
> 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:
>
>> * 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.