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

[Charles Keepax <ckeepax@opensource.cirrus.com>]

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

Thanks,
Charles