Re: [PATCH v10 3/5] ASoC: es9356-sdca: Add ES9356 SDCA driver

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

On 4/24/26 07:49, Zhang Yi wrote:
> This is the codec driver for es9356-sdca.

Starting to look quite good, additional comments below

> +struct  es9356_sdw_priv {
> +	struct sdw_slave *slave;
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct snd_soc_component *component;
> +	struct snd_soc_jack *hs_jack;
> +
> +	struct mutex disable_irq_lock;
> +	struct mutex pde_lock;

if you run checkpatch.pl ---strict it'll flag that the expectation is a comment where mutexes are declared.

> +	bool hw_init;
> +	bool first_hw_init;
> +	int jack_type;
> +	bool disable_irq;
> +
> +	struct delayed_work interrupt_handle_work;
> +	struct delayed_work button_detect_work;
> +	unsigned int sdca_status;

> +static int es9356_sdca_button_detect(struct es9356_sdw_priv *es9356)
> +{
> +	unsigned int btn_type = 0, offset, idx, val, owner;
> +	int ret;
> +	unsigned int button[3];

nit-pick: swap last two lines for reverse x-mas style.

> +
> +	ret = regmap_read(es9356->regmap,
> +		SDW_SDCA_CTL(FUNC_NUM_HID, ES9356_SDCA_ENT_HID01, ES9356_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), &owner);
> +	if (ret < 0 || owner == 0x01)
> +		return 0;
> +
> +	ret = regmap_read(es9356->regmap, ES9356_BUF_ADDR_HID, &offset);
> +	if (ret < 0)
> +		goto button_det_end;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(button); idx++) {
> +		ret = regmap_read(es9356->regmap, ES9356_BUF_ADDR_HID + offset + idx, &val);
> +		if (ret < 0)
> +			goto button_det_end;
> +		button[idx] = val;
> +	}
> +
> +	btn_type = es9356_sdca_button(&button[0]);
> +
> +button_det_end:
> +	if (owner == 0x00)
> +		regmap_write(es9356->regmap,
> +			SDW_SDCA_CTL(FUNC_NUM_HID, ES9356_SDCA_ENT_HID01, ES9356_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), 0x01);
> +
> +	return btn_type;
> +}

> +static int es9356_sdw_pcm_hw_params(struct snd_pcm_substream *substream,
> +				    struct snd_pcm_hw_params *params,
> +				    struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct es9356_sdw_priv *es9356 = snd_soc_component_get_drvdata(component);
> +	struct sdw_stream_config stream_config = {0};
> +	struct sdw_port_config port_config = {0};
> +	struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> +	int ret;
> +	unsigned char func, pde_entity, cs_entity;
> +	unsigned char ps0 = 0x0, ps3 = 0x3;
> +	unsigned int rate;
> +
> +	if (!sdw_stream)
> +		return -EINVAL;
> +
> +	if (!es9356->slave)
> +		return -EINVAL;
> +
> +	/* SoundWire specific configuration */
> +	snd_sdw_params_to_config(substream, params, &stream_config, &port_config);
> +
> +	port_config.num = dai->id;
> +
> +	ret = sdw_stream_add_slave(es9356->slave, &stream_config,
> +				   &port_config, 1, sdw_stream);
> +	if (ret) {
> +		dev_err(dai->dev, "Unable to configure port\n");
> +		return -EINVAL;
> +	}
> +
> +	switch (params_rate(params)) {
> +	case 16000:
> +		rate = ES9356_SDCA_RATE_16000HZ;
> +		break;
> +	case 44100:
> +		rate = ES9356_SDCA_RATE_44100HZ;
> +		break;
> +	case 48000:
> +		rate = ES9356_SDCA_RATE_48000HZ;
> +		break;
> +	case 96000:
> +		rate = ES9356_SDCA_RATE_96000HZ;
> +		break;
> +	default:
> +		dev_err(component->dev, "%s: Rate %d is not supported\n",
> +			__func__, params_rate(params));
> +		return -EINVAL;
> +	}
> +
> +	switch (dai->id) {
> +	case ES9356_DMIC:
> +		func = FUNC_NUM_MIC;
> +		cs_entity = ES9356_SDCA_ENT_CS113;
> +		pde_entity = ES9356_SDCA_ENT_PDE11;
> +		break;
> +	case ES9356_AMP:
> +		func = FUNC_NUM_AMP;
> +		cs_entity = ES9356_SDCA_ENT_CS21;
> +		pde_entity = ES9356_SDCA_ENT_PDE23;
> +		break;
> +	case ES9356_JACK_IN:
> +		func = FUNC_NUM_UAJ;
> +		cs_entity = ES9356_SDCA_ENT_CS36;
> +		pde_entity = ES9356_SDCA_ENT_PDE34;
> +		break;
> +	case ES9356_JACK_OUT:
> +		func = FUNC_NUM_UAJ;
> +		cs_entity = ES9356_SDCA_ENT_CS41;
> +		pde_entity = ES9356_SDCA_ENT_PDE47;
> +		break;
> +	default:
> +		dev_err(component->dev, "%s: Invalid dai id: %d\n",
> +			__func__, dai->id);
> +		return -EINVAL;
> +	}

can this switch + the power sequence below be factored in a helper used by hw_params and hw_free

> +
> +	regmap_write(es9356->regmap,
> +			SDW_SDCA_CTL(func, cs_entity, ES9356_SDCA_CTL_SAMPLE_FREQ_INDEX, 0), rate);

does this need to be done before the power sequence, if not can this be moved before the switch or after the power sequence to help create a helper?
> +
> +	/* power state changes are not independent across functions */
> +	mutex_lock(&es9356->pde_lock);
> +	ret = es9356_pde_transition_delay(es9356, func, pde_entity, ps3);

hah this is interesting, do you need to turn off before turning on?

Actually, since you have a pde_lock, is there really a case where another thread could turn off the power but not wait for the power transition to be complete?

Wondering how useful this is...

> +	if (ret) {
> +		regmap_write(es9356->regmap,
> +			SDW_SDCA_CTL(func, pde_entity, ES9356_SDCA_CTL_REQ_POWER_STATE, 0), ps0);
> +		ret = es9356_pde_transition_delay(es9356, func, pde_entity, ps0);
> +	} else
> +		dev_dbg(component->dev, "%s PDE is already PS0", __func__);
> +
> +	mutex_unlock(&es9356->pde_lock);
> +
> +	return 0;
> +}
> +
> +static int es9356_sdw_pcm_hw_free(struct snd_pcm_substream *substream,
> +				  struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct es9356_sdw_priv *es9356 = snd_soc_component_get_drvdata(component);
> +	struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> +	int ret;
> +	unsigned char func, pde_entity;
> +	unsigned char ps0 = 0x0, ps3 = 0x3;
> +
> +	if (!es9356->slave)
> +		return -EINVAL;
> +
> +	sdw_stream_remove_slave(es9356->slave, sdw_stream);
> +
> +	switch (dai->id) {
> +	case ES9356_DMIC:
> +		func = FUNC_NUM_MIC;
> +		pde_entity = ES9356_SDCA_ENT_PDE11;
> +		break;
> +	case ES9356_AMP:
> +		func = FUNC_NUM_AMP;
> +		pde_entity = ES9356_SDCA_ENT_PDE23;
> +		break;
> +	case ES9356_JACK_IN:
> +		func = FUNC_NUM_UAJ;
> +		pde_entity = ES9356_SDCA_ENT_PDE34;
> +		break;
> +	case ES9356_JACK_OUT:
> +		func = FUNC_NUM_UAJ;
> +		pde_entity = ES9356_SDCA_ENT_PDE47;
> +		break;
> +	default:
> +		dev_err(component->dev, "%s: Invalid dai id: %d\n",
> +			__func__, dai->id);
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&es9356->pde_lock);
> +	ret = es9356_pde_transition_delay(es9356, func, pde_entity, ps0);

same here, do you need to turn on before turning off?
in which cases would the power state NOT be PS0?

> +	if (ret) {
> +		regmap_write(es9356->regmap,
> +			SDW_SDCA_CTL(func, pde_entity, ES9356_SDCA_CTL_REQ_POWER_STATE, 0), ps3);
> +		ret = es9356_pde_transition_delay(es9356, func, pde_entity, ps3);
> +	} else
> +		dev_dbg(component->dev, "%s PDE is already PS0", __func__);
> +
> +	mutex_unlock(&es9356->pde_lock);
> +
> +	return 0;