Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

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

On Wed, May 14, 2025 at 02:15:01PM +0200, Pierre-Louis Bossart wrote:
> > I mean the hard definition here is DAPM routes. But roughly
> > speaking each line on the SDCA function diagram will correspond
> > to a route here, with a few tweaks like the GEs only show a
> > single line going to a red box around a bunch of other entities
> > that will be multiple routes one for each entity in the box.
> 
> This could be described in comments. The SDCA spec shows lines
> of different color for pure data paths and 'control' links. Here
> we are bringing everything back to the same concept of DAPM
> routes, but those extra routes only connect to supply widgets.

Yeah I am happy to add extra comments for this.

> >>> +		values[i + 3] = sdca_range(range, SDCA_SELECTED_MODE_INDEX, i);
> >>
> >> Humm, my memory of GEs is that the hardware reports a 'DETECTED_MODE' and
> >> then software (kernel and/or userspace) can choose a "SELECTED_MODE". here
> >> we only deal with SELECTED_MODE, is this intentional?
> > 
> > This code is creating the ALSA control for the SELECTED_MODE
> > control, the DETECTED_MODE is all handled internally and there is
> > no need to export it.
> 
> Humm, that one doesn't seem right. There could be cases
> where the DETECTED_MODE is inconclusive, or that additional
> vendor-specific steps are needed to decide which SELECTED_MODE
> makes sense. I don't think we should assume everything is
> handled internally, or we have to define what 'internally'
> means. This cannot be generic SDCA stuff, it has to be
> configurable for specific implementations/vendors.

Ok yeah I see, the current code reads the DETECTED_MODE in
the IRQ handler and then updates the selected mode. I guess
currently what is missing is the handling for the JACK_UNKNOWN
case, which could probably require user-space. I think it would
be pretty easy to export the DETECTED_MODE as well I will have a
look.

> >>> +static int entity_pde_event(struct snd_soc_dapm_widget *widget,
> >>> +			    struct snd_kcontrol *kctl, int event)
> >>> +{
> >>> +	struct snd_soc_component *component = widget->dapm->component;
> >>> +	struct sdca_entity *entity = widget->priv;
> >>> +	static const int poll_us = 10000;
> >>
> >> This should be a #define, no?
> >>
> > 
> > I could make it a define if you feel strong but I generally quite
> > like if it just a local delay being using in one place to define
> > it locally, means you don't have to look things up when reading
> > the code.
> 
> I guess my main point was to explain where this 10ms value
> comes from. I am not sure what to make of it, it's either very
> large or very small depending on the initial and desired power
> states. Plus I would expect the sleep times to vary by orders
> of magnitude depending on the type of function managed.

The trouble is there isn't a great way to parse from the DisCo
what a good poll interval would be. I guess we could change to
say 1/100th of the maximum, clipped to a minimum time? My concern
is the maximums can be quite long so something based on them
definitely runs the risk of really under polling and introducing
a lot of lag.

> >> Can you remind me in which cases we have a Selector Unit not
> >> controlled by a GE, and what userspace would do with such kcontrols?
> >>
> > 
> > A selector unit not tidied to a GE is just a DAPM mux, it
> > provides an ALSA control which allows user-space to select which
> > input it wants routed through the selector.
> 
> I meant in which topologies is this used/needed?

Its also nice to support topologies above the set ones in the
spec, but the current in spec users of these would be:

UAJ:
SU136 - Which lets you mux the second jack capture onto the primary
capture path.
SU137 - Which lets you sidetone from the second jack capture.

Admittedly our chips don't currently have those but since I
implemented SU support before GE support we had code for this
anyway.

> >>> +		case SDCA_ENTITY_TYPE_GE:
> >>> +			ret = entity_early_parse_ge(dev, function, entity);
> >>
> >> what does 'early' mean here? is there a 'late_parse_ge', or is this
> >> to say that GEs are parsed first?
> > 
> > Yeah we need to parse them first such that the GE controls
> > are ready when the SUs are parsed since they will link to the
> > control generated by the GE.
> 
> ok, worthy of a comment to make this dependency clearer.

Yeah no problem to add a comment for that.

Thanks,
Charles