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

[Lukas Wunner <lukas@wunner.de>]

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

Best regards,

Lukas