Re: [PATCH v9 2/4] ASoC: tac5xx2-sdw: add soundwire based codec driver

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

> 
>>> +		/* Detect and set jack type for UAJ path before playback.
>>> +		 * This is required as jack detection does not trigger interrupt
>>> +		 * when device is in runtime_pm suspend with bus in clock stop
>> mode.
>>> +		 */
>>
>> so here we have an interesting logic - or I misunderstood the comment?
>>
>> If a headset is inserted when the device is in runtime_pm suspend, how would
>> applications modify the routing and select playback on the headset, which
>> would then ripple down to this hw_params() call?
>>
>> IOW to play on a headset you first have to know there's a headset.
> 
> This is limitation in the current HW revision. So keeping this as temporary workaround
> to make uaj playback work.

That doesn't really address my point. If you don't have an interrupt to trigger a runtime_pm resume, I am not sure how audio routing would work. You could detect the headset when playing on *another* endpoint but that's a different function/GE entity to deal with...

In any case why not do the detection in the resume callback instead of hw_params?
see more on this below...
 

>>> +	guard(mutex)(&tac_dev->pde_lock);
>>
>> question I mentioned above: when you reach the hw_params phase, do you
>> really have a potential race with firmware download? What does this specific
>> use of pde_lock protect against?
> 
> answer below ...
>>> +static s32 tac_sdw_pcm_hw_free(struct snd_pcm_substream *substream,
>>> +			       struct snd_soc_dai *dai)
>>> +
>>> +	guard(mutex)(&tac_dev->pde_lock);
>>
>> same here, do you really have a race with firmware download?
>>
>> Or is this a case of dependencies between functions that requires all power
>> state transitions to be serialized?
> 
> ... during my testing, I have noted when the playback starts (from clock stop mode),
> device gets "detached" first, tries to resume, .hw_params is called, then gets "attached".

That seems very very odd. The resume should wait on the device being completely operational. Only then would you deal with hw_params.

Note that there are devices that require a soft reset after firmware download, and that does cause an extra detach-attach sequence, but again the resume has to complete fully before hw_params is reached. See examples from Cirrus Logic.

> I am checking slave's initialization completion variable in .hw_params to make sure that the
> firmware download is complete before .hw_params proceeds.

that's not the expected sequence, see all other codec drivers, they wait on the initialization_complete in resume callbacks.

> On the "master" side, looks like the slave status is handled via workqueues.
> So, still I wanted to keep one more protection, just in case, because of the above sequence, for serialization.

I would recommend you start with the expected sequences, I really don't see what's different in your case that would require such a fundamental change in programming sequences?