Re: [PATCH v10 1/4] ASoC: SDCA: Add PDE state transition helper

[Pierre-Louis Bossart]

On 4/22/26 10:26, Charles Keepax wrote:
> On Tue, Apr 21, 2026 at 06:21:09PM +0200, Pierre-Louis Bossart wrote:
>> On 4/21/26 17:57, Charles Keepax wrote:
>>> On Tue, Apr 21, 2026 at 09:18:01PM +0530, Niranjan H Y wrote:
>>>> Per SDCA specification, after writing REQUESTED_PS, drivers must
>>>> poll ACTUAL_PS until the target power state is reached.
>>>> Implement sdca_asoc_set_pde_state_sync() helper function for
>>>> changing the power state of the PDE widget and wait for the power
>>>> transition to happen or timeout.
>>>>
>>>> Changes include:
>>>> - Add sdca_asoc_set_pde_poll_sync() to handle write
>>>>   REQUESTED_PS register and poll ACTUAL_PS register until
>>>>   the target state is reached or timed out.
>>>> - Export function via sdca_asoc.h for use by SDCA-compliant drivers
>>>> - Refactor entity_pde_event() in sdca_asoc.c to use the helper
>>>>
>>>> Signed-off-by: Niranjan H Y <niranjan.hy@xxxxxx>
>>>> ---
>>>>  static int entity_parse_pde(struct device *dev,
>>>> @@ -449,8 +492,8 @@ static int entity_parse_pde(struct device *dev,
>>>>  	}
>>>>  
>>>>  	(*widget)->id = snd_soc_dapm_supply;
>>>> -	(*widget)->reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
>>>> -	(*widget)->mask = GENMASK(control->nbits - 1, 0);
>>>> +	(*widget)->reg = SND_SOC_NOPM;
>>>> +	(*widget)->mask = 0;
>>>
>>> Hmm... yeah I am really sorry I totally overlooked this. Yeah we
>>> should leave the write outside the helper it is much nicer to
>>> have DAPM do it.
>>
>> Not following that comment, there are quite a few codecs that
>> trap a DAPM event, do a regmap_write then wait with a loop,
>> see e.g. rt722_sdca_pde47_event
> 
> If you have a good reason to do the write manually you can
> (typically because it needs sequencing with other writes), but
> generally if the DAPM widget can do the write its better to do it
> that way. Just cleaner.

The existing code I was referring to pre-dates the introduction of sdca_parse_pde()...

It doesn't really need sequencing with other writes, DAPM is used to deal with PRE_PMU and POST_PMD events, it does not deal with register writes in all cases.

> Apologies totally my bad on this I should have realised the
> implications of this change the first time.
> 
>> I haven't seen DAPM deal directly with the write?
>> what am I missing?
> 
> That is what is happening in the above code setting the
> reg/mask/on_val/off_val, DAPM uses that information to do the
> register write.

Since we have two different ways of doing this PDE management, do we need two helpers, one with the write and one without?