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

[Holalu Yogendra, Niranjan]

On 16:57-20260420, Pierre-Louis Bossart wrote:
> Sent: Monday, April 20, 2026 4:57 PM
> Subject: Re: [PATCH v9 1/4] ASoC: SDCA: Add PDE verification reusable helper
> 
> 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:
> >>> + * 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.

Ok, I did not want to disturb the widget's event handler in this patch.
I will add this in the next patch.

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

Understood, I will change it to 100us and send new patch.
I realized that at @4.8MHz to 6MHz, 50x10, I think the read 
itself takes roughly ~42us to 50us. 
And with 100us it is 10ms before timeout happens
 (practically maybe it is roughly 15ms).

Regards
Niranjan