[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0b6aa842-ca82-f269-acfc-11624a0e787f@linux.intel.com>
Date: Mon, 21 Aug 2023 10:04:54 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Takashi Iwai <tiwai@...e.de>
Cc: Shenghao Ding <shenghao-ding@...com>, robh+dt@...nel.org,
lgirdwood@...il.com, perex@...ex.cz, kevin-lu@...com,
13916275206@....com, alsa-devel@...a-project.org,
linux-kernel@...r.kernel.org, liam.r.girdwood@...el.com,
mengdong.lin@...el.com, baojun.xu@...com,
thomas.gfeller@...rop.com, peeyush@...com, navada@...com,
broonie@...nel.org, gentuser@...il.com
Subject: Re: [PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver
On 8/21/23 09:57, Takashi Iwai wrote:
> On Mon, 21 Aug 2023 16:43:31 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>>>>> +static void tas2781_hda_playback_hook(struct device *dev, int action)
>>>>> +{
>>>>> + struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
>>>>> +
>>>>> + dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action);
>>>>> + switch (action) {
>>>>> + case HDA_GEN_PCM_ACT_OPEN:
>>>>> + pm_runtime_get_sync(dev);
>>>>
>>>> test if this actually works?
>>>
>>> To be fair, most of driver codes don't check it, including the
>>> HD-audio core. (Actually, over 900 of 1300 calls have no check in the
>>> whole tree.)
>>>
>>> It implies that forcing the check in each place is moot; rather the
>>> helper needs to be coded not to fail, IMO.
>>
>> Maybe that's true for HDaudio, for the SoundWire parts we absolutely
>> need to detect if the resume worked. There are more steps involved, the
>> clock-stop mode entry/exit, context restoration, re-enumeration, etc.
>>
>> I think it'd be a mistake to sit on our hands and assume the world is
>> perfect. We have to track cases where the codec isn't properly resumed
>> and prevent it from accessing resources that are just unavailable.
>
> Yeah, I don't mean that it's wrong or bad to have the check. The
> check should be there.
>
> But, I feel that it's time to rather switch to the proper call.
> Basically pm_runtime_resume_and_get() is the better alternative
> (except for its long naming), and we may think of converting the
> whole.
Oh, I broke so many drivers by trying a well-indented conversion to
pm_runtime_resume_and_get().
The flow is different wrt -EACCESs and we ended-up with multiple errors.
In hindsight I wish we had left the legacy code alone.
>>>>> +static int tas2781_system_suspend(struct device *dev)
>>>>> +{
>>>>> + struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
>>>>> + int ret;
>>>>> +
>>>>> + dev_dbg(tas_priv->dev, "System Suspend\n");
>>>>> +
>>>>> + ret = pm_runtime_force_suspend(dev);
>>>>> + if (ret)
>>>>> + return ret;
>>>>
>>>> that's usually the other way around, for system suspend you either want
>>>> the device to be pm_runtime active, or if it's already suspended do nothing.
>>>>
>>>> This is very odd to me.
>>>
>>> This is a normal procedure, as stated in pm_runtime_force_suspend()
>>> definition:
>>>
>>> /**
>>> * pm_runtime_force_suspend - Force a device into suspend state if needed.
>>> ....
>>> * Typically this function may be invoked from a system suspend callback to make
>>> * sure the device is put into low power state and it should only be used during
>>> * system-wide PM transitions to sleep states. It assumes that the analogous
>>> * pm_runtime_force_resume() will be used to resume the device.
>>
>> It's possible that it's fine for HDaudio, it wouldn't work in all cases
>> for SoundWire where we have to make sure all pm_runtime suspended
>> devices are brought back to D0 and then the regular system suspend
>> happens. That's mainly because pm_runtime suspend relies on clock stop
>> and system suspend does not.
>>
>> In other words, this isn't a generic solution at all.
>
> Well, I suppose rather that soundwire is an exception :)
>
> For majority of devices, the system suspend/resume is nothing but
> pm_runtime_force_*() calls. e.g. take a look at
> DEFINE_RUNTIME_DEV_PM_OPS() in linux/pm_runtime.h.
I guess you are right. SoundWire has indeed these quirky modes and
differences between SOC vendors that will force us to be extra careful
in what the codec driver implements.
Powered by blists - more mailing lists