Re: [PATCH 1/3] ASoC: SDCA: Create DAPM widgets and routes from DisCo

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

On 3/25/25 06:19, Charles Keepax wrote:
> On Mon, Mar 24, 2025 at 04:15:24PM -0500, Pierre-Louis Bossart wrote:
>>> The primary point of interest is the SDCA Power Domain Entities
>>> (PDEs), which actually control the power status of the device. Whilst
>>> these PDEs are the primary widgets the other parts of the SDCA graph
>>> are added to maintain a consistency with the hardware abstract, and
>>> allow routing to take effect.
>>>
>>> Other minor points of slightly complexity include, the Group Entities
>>> (GEs) these set the value of several other controls, typically
>>> Selector Units (SUs) for enabling a cetain jack configuration. These
>>> are easily modelled creating a single control and sharing it among
>>> the controlled muxes.
>>
>> It wasn't able to follow the last sentence, what are 'these'?
> 
> I will attempt to rephrase this paragraph a little, but 'these'
> are situations where you have a bunch of SUs controlled by a GE.

In most cases the purpose of a GE is to control multiple SU states. However the SDCA spec took liberties with this concept and added new properties for the NDAI topologies, where a GE is present even if there is a single endpoint. It'd be worth double-checking that the way the GE is exposed in this patchset is forward-compatible with the 1.1 topologies.

>> I am not sure we can expose and control any SUs since their
>> configuration is set in hardware depending on the GE settings. IIRC
>> the SU values should be considered as read-only.
> 
> The SUs are modelled as DAPM widgets but the control linked
> to all of the SUs is the GE control. So yes the SU registers
> are never accessed only the GE register.

How would the state of those DAPM SU widgets be updated though? I think we need to 'translate' the GE settings to tell DAPM which paths can become active, but the SUs state is set by hardware so I could see a possible racy disconnect if we make a path activable but hardware hasn't done so yet.

>>> SDCA also has a slight habit of having fully connected paths, relying
>>> more on activating the PDEs to enable functionality. This doesn't map
>>> quite so perfectly to DAPM which considers the path a reason to power
>>> the PDE. Whilst in the current specification Mixer Units are defined as
>>> fixed-function, in DAPM we create a virtual control for each input. This
>>> allows paths to be connected/disconnected, providing a more ASoC style
>>> approach to managing the power.
>>
>> Humm, maybe my analysis was too naive but the SDCA PDE seemed
>> like a DAPM power supply to me. When a path becomes active,
>> DAPM turns on the power for you, and power is turned off some time
>> after the path becomes inactive.
> 
> Correct, the PDEs are modeled as supply widgets and those are
> powered up when the path is active as normal. The problem
> alluded to in this paragraph is there a couple times where
> SDCA topologies just have a permanently connected path so
> things would always power up.

Ah yes those loops would indeed be problematic, but no more than in existing non-SDCA topologies where we used pin switches to disable such loops. All existing TDM-based solutions used pin switches, I was assuming we'd use them as well for SDCA.

>> Why would we need to have a control to force the power to be turned on?
> 
> We are not having a control to force the power on, that is me
> describing the SDCA view of the world, not the Linux ASoC view
> of the world.
> 
>> And there are quite a few topologies without any Mixer Units so can
>> we depend on a solution that's not applicable across all topologies?
> 
> Most of the time things are fine because most topologies
> generally go between a dataport terminal and a physical
> terminal, so the dataport can trigger power up. There are
> only two cases I am currently aware of where this happens
> at the moment, one is the side-tone path in the UAJ topology,
> which is primarily the one I was concerned about for now. The
> other is the sense path in the SmartAmp topology, which I am
> slightly less concerned about for now.
> 
> The sense path case can't be solved with the mixer switches.
> So eventually I think we will also need to add pin switches
> on the non-dataport terminals as well, but this is thinking I
> was hoping to delay for later.
> 
> However, we should really consider the wider mechanisms defined
> by the specification rather than just the given topologies.
> User-defined topologies are allowed and people will likely make
> small uncompliant diversions from the standard topologies as well.
> 
> My opinion is that even if we end up adding the pin switches as
> well it still makes sense to allow connecting and disconnecting
> the inputs of a Mixer Unit.  These are typically where two
> audio streams come together and having the ability from the
> host side to say if you want that connection or not seems very
> valuable to me. As in SDCA land you basically make that choice by
> directly flipping the PDE.

I have no objection if there are both pin switches and MU switches. 

I view pin switches as a more generic mechanism that userpace has to set to use a specific endpoint. 

The MU switches seem like debug capabilities to isolate which path has a problem. My experience fixing Baytrail issues is that you want a default mixer switch to be on, otherwise you'll get warnings on unconnected items or 'there is no sound' bug reports. In other words, the MU switches are a nice-to-have mechanism to disable default paths, so even if userspace doesn't touch those controls sound can be heard on endpoints.
 
>> And last PDEs are typically related to terminals, while Mixer
>> Units are usually for host-generated streams.
> 
> From the view of DAPM I don't think we need to be too careful
> about that distinction. Its all just connections between widgets.
> 
>> It would also help to define which power levels you wanted to
>> control for PDEs. For me, only PS0 and PS3 can currently be modeled,
>> I have no idea how PS1 with its degraded quality would be used,
>> and PS2 depends on firmware.
> 
> Indeed, currently the code only deals with PS0 and PS3. I will
> update the commit message to call that out as well.

Sounds good.