Re: [PATCH v3 RESEND 14/19] ASoC: SDCA: Add FDL library for XU entities

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

On Mon, Oct 27, 2025 at 03:39:40PM +0100, Pierre-Louis Bossart wrote:
> > +config SND_SOC_SDCA_FDL
> > +	bool "SDCA FDL (File DownLoad) support"
> > +	depends on SND_SOC_SDCA
> > +	default y
> > +	help
> > +	  This option enables support for the File Download using UMP,
> > +	  typically used for downloading firmware to devices.
> 
> nit-pick: shouldn't this option be selected by device drivers
> who need this library?

These bits are all bools that add additional stuff into the main
SDCA module, so they do nothing if SDCA isn't selected. So the
device driver would just select SDCA generally speaking and I
think it makes sense if you enable SDCA that you get all the
functionality. If a certain config wants less they can disable
but that feels like its just for people really trying to size
optimise their kernel.

> There should be guardrails to prevent the fallback helpers from
> being used silently.

A top level driver has to bind to the device and it still
basically controls what happens, it has to register the stuff for
FDL to happen before it will happen.

> > +int sdca_reset_function(struct device *dev, struct sdca_function_data *function,
> > +			struct regmap *regmap)
> > +{
> > +	unsigned int reg = SDW_SDCA_CTL(function->desc->adr,
> > +					SDCA_ENTITY_TYPE_ENTITY_0,
> > +					SDCA_CTL_ENTITY_0_FUNCTION_ACTION, 0);
> > +	unsigned int val, poll_us;
> > +	int ret;
> > +
> > +	ret = regmap_write(regmap, reg, SDCA_CTL_ENTITY_0_RESET_FUNCTION_NOW);
> > +	if (ret) // Allowed for function reset to not be implemented
> > +		return 0;
> 
> nit-pick: maybe document why this is allowed. This doesn't seem
> very good in terms of design.

It's what the spec says, as often is the case I tend to agree with
you its not the best design decision. I guess perhaps the wording
could be tweaked but in general where the comments say this weird
thing is allowed/required they are referring to the spec.

> > +	if (!function->reset_max_delay) {
> > +		dev_err(dev, "No reset delay specified in DisCo\n");
> > +		return -EINVAL;
> > +	}
> 
> nit-pick: or maybe fallback to a reasonable default? Making the
> entire download dependent on Disco correctness isn't going to work
> long-term.

I don't have any objections to a default here, although it should
also have a comment to say why, since the spec requires this
property if function reset is implemented.

On a wider point I am not sure I totally agree with the DisCo
correctness point. I guess we have to cross those bridges as we find
them but in general an awful lot of this class driver relies on DisCo
correctness. We build the whole function topology from DisCo if that
is incorrect there is a fairly good chance the part won't work.
Although for the most part my thinking has been if someone stuffs
up the DisCo too badly they will have to implement a custom
driver to handle it rather than relying on the class driver, but
for cases like this I think we can be a little flexible.

> > +
> > +	poll_us = umin(function->reset_max_delay >> 4, 1000);
> 
> add comment on what this >> 4 means.

Yeah that could use a comment I will add one.

> > +	switch (response) {
> > +	case SDCA_CTL_XU_FDLH_RESET_ACK:
> > +		dev_dbg(dev, "FDL request reset\n");
> > +
> > +		switch (xu->reset_mechanism) {
> > +		default:
> > +			dev_warn(dev, "Requested reset mechanism not implemented\n");
> > +			fallthrough;
> 
> this fallthrough looks odd since it forces a check on the reset a second time ...
> 
> > +		case SDCA_XU_RESET_FUNCTION:
> > +			goto reset_function;
> > +		}
> > +	default:
> > +		return 0;
> > +	}
> > +
> > +reset_function:
> 
> ... inside this function call
> 
> > +	sdca_reset_function(dev, interrupt->function, interrupt->function_regmap);

Not sure I totally follow this one, there isn't a second check of
reset_mechanism inside sdca_reset_function(). Perhaps this was an
earlier iteration of the code.

Thanks,
Charles