Re: [PATCH v11 2/4] ASoC: tac5xx2-sdw: add soundwire based codec driver

[Mark Brown <broonie@kernel.org>]

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

On Mon, Apr 27, 2026 at 01:53:59PM +0530, Niranjan H Y wrote:
> Add codec driver for tac5xx2 family of devices.

> +	switch (dai->id) {
> +	case TAC5XX2_DMIC:
> +		pde_entity = TAC_SDCA_ENT_PDE11;
> +		function_id = TAC_FUNCTION_ID_SM;
> +		break;
> +	case TAC5XX2_UAJ:
> +		function_id = TAC_FUNCTION_ID_UAJ;
> +		pde_entity = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
> +				TAC_SDCA_ENT_PDE47 : TAC_SDCA_ENT_PDE34;
> +		break;
> +	default:
> +		function_id = TAC_FUNCTION_ID_SA;
> +		pde_entity = TAC_SDCA_ENT_PDE23;
> +		break;
> +	}

Shouldn't the default case be to print a warning or something and have
an explicit case for what's there now?  It seems weird that we just
silently handle one ID by mapping any old number that gets passed in to
it.

> +static int tac5xx2_set_jack(struct snd_soc_component *component,
> +			    struct snd_soc_jack *hs_jack, void *data)
> +{

> +	tac_dev->hs_jack = hs_jack;
> +	if (!tac_dev->hw_init) {
> +		dev_err(tac_dev->dev, "jack init failed, hw not initialized");
> +		return 0;
> +	}

I'm not seeing anything that does the jack setup when the hw_init
happens?

> +static int tac_update_status(struct sdw_slave *slave,
> +			     enum sdw_slave_status status)
> +{

> +	if (!tac_dev->first_hw_init) {
> +		pm_runtime_set_autosuspend_delay(tac_dev->dev, 3000);
> +		pm_runtime_use_autosuspend(tac_dev->dev);
> +		pm_runtime_mark_last_busy(tac_dev->dev);
> +		pm_runtime_set_active(tac_dev->dev);
> +		pm_runtime_enable(tac_dev->dev);
> +		tac_dev->first_hw_init = true;
> +		first = true;
> +	}

Does anything ever actually undo this pm_runtime_enable()?

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