Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <830d8e26-dbb9-4b9c-bbab-a5c4c49a7ffd@linux.intel.com>
Date:   Tue, 5 Dec 2023 11:22:32 -0600
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] ALSA: hda/tas2563: Add tas2563 HDA driver
Content-Language: en-US
To:     Gergo Koteles <soyer@irl.hu>, Shenghao Ding <shenghao-ding@ti.com>,
        Kevin Lu <kevin-lu@ti.com>, Baojun Xu <baojun.xu@ti.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>
Cc:     linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org
References: <cover.1701733441.git.soyer@irl.hu>
 <4a2f31d4eb8479789ceb1daf2e93ec0e25c23171.1701733441.git.soyer@irl.hu>
 <90765ee0-a814-4852-9b2a-020cda98d930@linux.intel.com>
 <974d41f6c703d9b65ebcd75a2c659cecf13bd877.camel@irl.hu>
 <a530e70a-2491-4270-b582-cd493d1512b1@linux.intel.com>
 <9c3846ae0da417c0fe5d4fa2d9d4134143184dda.camel@irl.hu>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <9c3846ae0da417c0fe5d4fa2d9d4134143184dda.camel@irl.hu>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


>>>>> +static void tas2563_fixup_i2c(struct hda_codec *cdc,
>>>>> +	const struct hda_fixup *fix, int action)
>>>>> +{
>>>>> +	 tas2xxx_generic_fixup(cdc, action, "i2c", "INT8866");
>>>>
>>>> Any specific reason to use an Intel ACPI identifier? Why not use
>>>> "TIAS2563" ?
>>>>
>>> INT8866 is in the ACPI.
>>> I don't know why Lenovo uses this name.
>>> I think it's more internal than intel.
>>>
>>>    Scope (_SB.I2CD)
>>>     {
>>>         Device (TAS)
>>>         {
>>>             Name (_HID, "INT8866")  // _HID: Hardware ID
>>
>> Ouch, I hope they checked with Intel that this isn't an HID already in
>> use...
>>
> It looks the INT prefix is not reserved. (yet)
> https://uefi.org/ACPI_ID_List?acpi_search=INT

It's been de-facto reclaimed by Intel over the years, apparently using
INTC or INTL was too hard for some of my colleagues...

There are lots of INT devices in the kernel today, here's a small list
for sound/soc/codecs only

rt274.c:        { "INT34C2", 0 },
rt286.c:        { "INT343A", 0 },
rt298.c:        { "INT343A", 0 },
ssm4567.c:      { "INT343B", 0 },

Those INT values were added by Intel teams though, it's really odd to
see Lenovo use an INT-based HID. Should really use 104C2563 or something.


>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct dev_pm_ops tas2563_hda_pm_ops = {
>>>>> +	SYSTEM_SLEEP_PM_OPS(tas2563_system_suspend, tas2563_system_resume)
>>>>
>>>> where's the pm_runtime stuff?
>>>>
>>>
>>> The amp stores its state in software shutdown mode.
>>> The tas2563_hda_playback_hook wakes/shutdowns the amp, not the
>>> pm_runtime.
>>
>> My point was that you have all these pm_runtime_ calls in the code, but
>> nothing that provides pm_runtime suspend-resume functions so not sure
>> what exactly the result is?
>>
>>
> I think nothing. I haven't experienced anything unusual recently.

you can probably see from the /sys directory what the pm_runtime power
state is, most likely the status is 'unknown'.