Re: [PATCH v12] ASoc: tas2783: Add tas2783 codec driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]

On Mon, Apr 01, 2024 at 08:50:24AM -0500, Pierre-Louis Bossart wrote:

> > +static int tasdevice_comp_probe(struct snd_soc_component *comp)
> > +{

> > +	ret = request_firmware(&fw_entry, dspfw_binaryname, tas_dev->dev);
> > +	if (ret) {
> > +		dev_err(tas_dev->dev,
> > +			"%s: request_firmware %x open status: %d.\n", __func__,
> > +			tas_dev->sdw_peripheral->id.unique_id, ret);
> > +		goto out;
> > +	}
> > +
> > +	tasdevice_dspfw_ready(fw_entry, tas_dev);

> Question: is there a specific reason why all this functionality needs to
> be done in a component probe instead of when the device reports as ATTACHED?

> Since you have an interaction with userspace for the firmware, and
> firmware download takes time, you would want to do this as early as
> possible.

> Usually the component probe is part of the card creation, so you could
> add card-related or control related things. Downloading firmware does
> not strike me as a card-related activity?

This does have the effect of ensuring that the card won't instantiate
without firmware.  Given how utterly dependent on firmware this device
seems to be I can see a case for blocking card registration until the
firmware is ready, there's a usability thing there but it feels like
there's a usability issue with any error handling for missing firmware
on these devices and not registering the card does seem like a valid
choice.  That said it would still be nicer to initiate the firmware
process earlier in order to minimise latency and then either defer
registration of the component until we've managed to load firmware or
have a check at component probe to make sure that the firmware is ready.

> Another question is whether the firmware needs to be downloaded again
> during system/pm_runtime resume? That may depend on how power is managed
> at the hardware level, but in theory an SDCA device should report to the
> host whether the firmware is still valid or needs to be downloaded.

That does seem like a concern too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]