Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

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

On Mon, Oct 27, 2025 at 04:02:54PM +0100, Pierre-Louis Bossart wrote:
> > diff --git a/sound/soc/sdca/Kconfig b/sound/soc/sdca/Kconfig
> > index e7f36d668f159..56e3ff8448762 100644
> > --- a/sound/soc/sdca/Kconfig
> > +++ b/sound/soc/sdca/Kconfig
> > @@ -37,4 +37,14 @@ config SND_SOC_SDCA_FDL
> >  config SND_SOC_SDCA_OPTIONAL
> >  	def_tristate SND_SOC_SDCA || !SND_SOC_SDCA
> >  
> > +config SND_SOC_SDCA_CLASS
> > +	tristate "SDCA Class Driver"
> > +	select SND_SOC_SDCA
> > +	select SND_SOC_SDCA_FDL
> 
> which builds on my other comment that the default y is probably not required.
> config SND_SOC_SDCA_FDL
> 	bool "SDCA FDL (File DownLoad) support"
> 	depends on SND_SOC_SDCA
> 	default y

Yeah ok I see what you are saying here, I would be reticent to
remove these selects because the driver depends on those
features, but then if all drivers think like that the default
does become redundant.

> > +	select SND_SOC_SDCA_HID
> > +	select SND_SOC_SDCA_IRQ
> > +	help
> > +	  This option enables support for the SDCA Class driver which should
> > +	  support any class compliant SDCA part.
> > +
> 
> > +#define CLASS_SDW_ATTACH_TIMEOUT_MS	5000
> 
> maybe add a comment on what the 'ATTACH' refers to.

Is this really ambiguous attach has a pretty defined meaning in a
soundwire context and it is SDW_ATTACH?

> > +
> > +static int class_read_prop(struct sdw_slave *sdw)
> > +{
> > +	struct sdw_slave_prop *prop = &sdw->prop;
> > +
> > +	sdw_slave_read_prop(sdw);
> > +
> > +	prop->use_domain_irq = true;
> > +	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY |
> > +			      SDW_SCP_INT1_IMPL_DEF;
> 
> Oh now I understand this 'class' driver is really a SoundWire
> device driver. You should clarify that this isn't meant to deal
> with function devices but it's one level up.

Yeah the commit message in general needs some beefing up.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int class_sdw_update_status(struct sdw_slave *sdw, enum sdw_slave_status status)
> > +{
> > +	struct sdca_class_drv *drv = dev_get_drvdata(&sdw->dev);
> > +
> > +	switch (status) {
> > +	case SDW_SLAVE_ATTACHED:
> > +		dev_info(drv->dev, "device attach\n");
> 
> probably too verbose, dev_dbg() would make more sense IMHO.

Agree yeah makes sense.

> > +	pm_runtime_set_autosuspend_delay(dev, 250);
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_get_noresume(dev);
> 
> This seems odd, I don't recall that we do something similar in existing codec drivers.
> It'd be worth explaining the whole pm_runtime flow.

I suspect that means those codec drivers are broken :-) at least
if they actively use pm_runtime. The first two enable
autosuspend, that is suspending the device after a short period
of inactivity. set_active tells the PM runtime the device is
already powered up, as it is. Finally, get_noresume takes a
reference, this will stop it powering down immediately once the
pm_runtime is enabled, it will waiting until we put the reference
we are holding.

> > +static int class_runtime_resume(struct device *dev)
> > +{
> > +	struct sdca_class_drv *drv = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = class_wait_for_attach(drv);
> 
> You probably need to explain the model here. In the general
> case a bus could just enable clock_stop and the attach should be
> immediate on restarting the clock. The wait only makes sense for
> cases where the bus is powered-down. Not all solutions will choose
> the last option.
> 

If the device never dettached wait_for_attach will just return
immediately. I am not sure exactly what the problem is here, the
soundwire framework in Linux does not guarantee the device is
attached before the runtime_resume callback is called so we need
to ensure that is the case. Now personally I would probably argue
it would be nice if the framework did guarantee that but this
isn't an SDCA thing, that is an existing property of the
soundwire stuff so I don't think blocking the SDCA on changing it
makes sense. But it is definitely something that people finding
time to work on would be awesome.

> > +static const struct sdw_device_id class_sdw_id[] = {
> > +	SDW_SLAVE_ENTRY(0x01FA, 0x4245, 0),
> 
> It seems odd to put specific ACPI identifiers here.

This is likely not the full complete solution but probably is a
reasonable place to start for now.

> It also begs the question on how extensions could be managed.
> For a first-start it's probably ok but we've got to have alignment on
> how we identify 
> - devices for which the default class is 'good-enough'

Here we identify those by which identifiers are added to the
class driver.

> - devices where an extension on top of the default class is required
> - devices where a custom driver is needed

Currently the philosophy we are working on is that there are no
extensions which I thought we had discussed in the past.  All the
SDCA stuff has been implemented as a library of helpers, that
means that if your part needs custom handling you should
implement a custom driver but for the parts where you are spec
compliant you can just call the library helpers so you don't have
to reimplement those bits. The class driver here is just some
code that calls the superset of helpers.

> IIRC when I looked at class drivers a long time ago, we don't
> have the plumbing in the SoundWire bus to automagically load a class
> driver if no custom driver is found - and adding such plumbing was
> way above my skills/knowledge.

Yeah long term something like check for driver match then if not
check the SDCA bit and try the class driver if it is set. But I
don't think those are problems we should be trying to solve here.
Nothing here really forces any choices on these decisions later.

Thanks,
Charles