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

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

On Mon, Oct 27, 2025 at 04:24:05PM +0100, Pierre-Louis Bossart wrote:
> > +	snd_sdw_params_to_config(substream, params, &sconfig, &pconfig);
> > +
> > +	ret = sdca_asoc_get_port(drv->dev, drv->regmap, drv->function, dai);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pconfig.num = ret;
> 
> You should probably document the assumption that each function
> has access to separate ports.
> 
> IIRC there could be cases where ports are *shared* between
> functions due to hardware restrictions. I remember telling the
> SDCA committee that either functions were independent or they were
> not, and the answer is that they are independent except when they
> aren't. It's probably ok to assume a simple case first to make
> progress, but with a clear documentation of the known limits.
> 
> Oh and for some functions multiple ports are required to make use
> of different lanes, e.g. for the digital links in the NDAI functions.

Yeah I will add a comment to clarify we are currently only
supporting a single port, although it does somewhat duplicate the
FIXME already in sdca_asoc_get_port for that.

But overal I think these are definitely bridges we should cross
when we get there. It is more important to get something that
functions rather than it supporting every single feature in the
first iteration (especially given how close we now our to laptops
shipping with parts we were hoping to support through this class
driver).

> > +	pm_runtime_set_autosuspend_delay(dev, 200);
> 
> why 200?

That is a totally made up figure specially selected to be
slightly shorted that the totally made up figure I picked for the
primary driver. Happy to take feedback on these lengths or add a
comment to say they are arbitary although these tend to be
arbitary on all drivers so I didn't feel the need to add the
comment.

> > +	ret = devm_snd_soc_register_component(dev, cmp_drv, dais, num_dais);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to register component\n");
> > +
> > +	dev_err(dev, "%s: %pfwP: probe completed\n", __func__, auxdev->dev.fwnode);
> 
> why is this an error? the error case is handled earlier.

Yeah that is just debug that hasn't been nuked yet the whole
print should be deleted.

> > +static int class_function_codec_runtime_suspend(struct device *dev)
> > +{
> > +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
> > +
> > +	/*
> > +	 * Whilst the driver doesn't power the chip down here, going into
> > +	 * runtime suspend lets the SoundWire bus power down, which means
> > +	 * the driver can't communicate with the device any more.
> > +	 */
> 
> actually this is not completely correct. The function driver can
> communicate IF the parent is active. This is copy-paste from the
> device level but it's not how the hardware works.

I mean it does say "lets the SoundWire bus power down" not forces
the bus to power down. But I will try to reword to make it more
clear this is covering the fact the bus might power down rather
than will power down.

> > +	regcache_cache_only(drv->regmap, true);
> 
> I would also add a comment that all the power management for
> PE/GEs is handled by DAPM when streams become active/suspended. So
> there isn't a need to deal with PE/GEs at this level.

Can do.

> > +static int class_function_codec_runtime_resume(struct device *dev)
> > +{
> > +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
> > +	int ret;
> > +
> > +	regcache_cache_only(drv->regmap, false);
> > +
> > +	regcache_mark_dirty(drv->regmap);
> 
> does the order matter?

I don't believe it does, happy to reverse them.

Thanks,
Charles