Re: [PATCH v9 1/4] ASoC: SDCA: Add PDE verification reusable helper

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

On 4/20/26 12:35, Charles Keepax wrote:
> On Mon, Apr 20, 2026 at 11:49:00AM +0200, Pierre-Louis Bossart wrote:
>> On 4/17/26 15:13, Niranjan H Y wrote:
>>> + * This function implements the polling logic but does NOT modify the power state.
>>> + * The caller is responsible for writing REQUESTED_PS before invoking this function.
>>
>> Erm, why not dealing with the write to REQUESTED_PS in this
>> helper? You have all the 'to' and 'from' information in the
>> parameters.
> 
> I have no objections to moving that into the helper as well.
> 
>>> +	static const int polls = 100;
>>> +	static const int default_poll_us = 1000;
>>> +	unsigned int reg, val;
>>> +	int i, poll_us = default_poll_us;
>>> +	int ret;
>>> +
>>> +	if (pde_delays && num_delays > 0) {
>>> +		for (i = 0; i < num_delays; i++) {
>>> +			if (pde_delays[i].from_ps == from_ps && pde_delays[i].to_ps == to_ps) {
>>> +				poll_us = pde_delays[i].us / polls;
>>> +				break;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +	reg = SDW_SDCA_CTL(function_id, entity_id, SDCA_CTL_PDE_ACTUAL_PS, 0);
>>> +
>>> +	for (i = 0; i < polls; i++) {
>>> +		if (i)
>>> +			fsleep(poll_us);
>>
>> This solution will loop for up to 100 times, and the sleep
>> duration could be questionable.
> 
> The duration doesn't have to be precise here, as long as the
> result is longer than the requested time everything is fine.
> 
>> Say for example you have a 10ms transition, do you really want
>> to read ACTUAL_PS every 100us?
> 
> Quite potentially, I imagine it will be fairly common for parts
> to change PS a lot faster than the actual timeouts they provide,
> due to corner cases and people just being conservative in the
> DisCo. So its quite possible something that says 10mS typically
> switches in a couple 100uS.
> 
>> If the pde_delay is 1ms then a read every 10us makes no sense,
>> the SoundWire command protocol would not be able to handle
>> such reads.
>>
>> A minimum threshold on poll_us would make sense IMHO.
> 
> I guess you do reach a point where the soundwire command makes
> the delay effectively meaningless. What would you suggest for a

yep, that was the main point.

> minimum? Something like 100uS feels kinda reasonable to me,
> I would lean towards quite a small value here. Other options
> might be to look at some sort of exponential back off, doing the
> first few polls faster than later ones.
> 
> This is definitely one of those situations where SDCA is a little
> too vague for its own good. But I would also say making a change
> like this should at a minimum be a separate patch rather than
> part of this one. And I am not convinced we need to block this
> series on updating it, although if we just wanted to go with a
> simple minimum that seems easy enough to add.

A minimum of 100us would be fine, we can always optimize for long delays later.