Re: [PATCH] ASoC: rt766: add RT766/RT767 SDCA driver

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

On Mon, Jul 20, 2026 at 05:06:13PM +0800, shumingf@realtek.com wrote:
> From: Shuming Fan <shumingf@realtek.com>
> 
> This patch adds the initial SDCA multi-function codec driver for the RT766 and RT767.
> 
> Signed-off-by: Shuming Fan <shumingf@realtek.com>
> ---
> +static struct sdca_entity *rt766_find_entity_by_label(struct sdca_function_data *func,
> +	const char *label)
> +{
> +	struct sdca_entity *entity = NULL;
> +	int idx;
> +
> +	for (idx = 0; idx < func->num_entities; idx++) {
> +		entity = &func->entities[idx];
> +
> +		if (!strcmp(entity->label, label))
> +			return entity;
> +	}
> +
> +	return NULL;
> +}

Probably better to export find_sdca_entity_by_label() if you need
this from the driver.

> +static int rt766_sdca_irq_ctl(struct rt766_sdca_priv *rt766,
> +							  struct sdca_function_data *function,
> +							  struct snd_soc_component *component,
> +							  struct sdca_interrupt_info *info,
> +							  bool enabled)
> +{
> +	struct device *dev = &rt766->slave->dev;
> +	struct sdca_interrupt *interrupt;
> +	struct sdca_control *control;
> +	struct sdca_entity *entity;
> +	irq_handler_t handler;
> +	int i, j, irq, ret;
> +
> +	for (i = 0; i < function->num_entities; i++) {
> +		entity = &function->entities[i];
> +
> +		for (j = 0; j < entity->num_controls; j++) {
> +			control = &entity->controls[j];
> +			irq = control->interrupt_position;
> +
> +			switch (SDCA_CTL_TYPE(entity->type, control->sel)) {
> +			case SDCA_CTL_TYPE_S(GE, DETECTED_MODE):
> +				handler = rt766_sdca_irq_jd_handler;
> +				break;
> +			case SDCA_CTL_TYPE_S(HIDE, HIDTX_CURRENTOWNER):
> +				handler = rt766_sdca_irq_btn_handler;
> +				break;
> +			default:
> +				continue;
> +			}
> +
> +			interrupt = &info->irqs[irq];
> +
> +			if (enabled) {
> +				ret = sdca_irq_data_populate(dev, rt766->regmap, component,
> +								function, entity, control,
> +								interrupt);
> +				if (ret)
> +					return ret;
> +
> +				interrupt->priv = rt766;
> +				ret = sdca_irq_request(dev, info, irq, interrupt->name,
> +								handler, interrupt);
> +				if (ret) {
> +					dev_err(dev, "failed to request irq %s: %d\n",
> +						interrupt->name, ret);
> +					return ret;
> +				}
> +				dev_dbg(dev, "Requesting IRQ %d InterruptName=%s\n", irq, interrupt->name);
> +			} else {
> +				sdca_irq_free(dev, info, irq, interrupt->name, interrupt);
> +				dev_dbg(dev, "Freeing IRQ %d\n", irq);
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}

Hmm... yeah this is moving in a slightly different direction from
where I was going with the core code. Let me think about that and
update tomorrow.

> +static int rt766_sdca_pde47_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +	struct rt766_sdca_priv *rt766 = snd_soc_component_get_drvdata(component);
> +	const struct sdca_entity *entity = NULL;
> +	unsigned char ps0 = 0x0, ps3 = 0x3;
> +	int from_ps, to_ps;
> +	int ret;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		regmap_write(rt766->regmap, RT766_PDE_REQ_REG(UAJ, PDE47), ps0);
> +		from_ps = ps3;
> +		to_ps = ps0;
> +		break;
> +	case SND_SOC_DAPM_PRE_PMD:
> +		regmap_write(rt766->regmap, RT766_PDE_REQ_REG(UAJ, PDE47), ps3);
> +		from_ps = ps0;
> +		to_ps = ps3;
> +		break;
> +	}
> +
> +	entity = rt766_find_entity_by_label(rt766->uaj_func_data, "PDE 47");
> +	ret = sdca_asoc_pde_poll_actual_ps(component->dev, rt766->regmap,
> +				   RT766_FUNC_NUM_UAJ,
> +				   RT766_SDCA_ENT_PDE47,
> +				   from_ps, to_ps,
> +				   entity ? entity->pde.max_delay : NULL,
> +				   entity ? entity->pde.num_max_delay : 0);
> +	if (ret)
> +		dev_err(component->dev, "%s: PDE transition %x -> %x failed, err=%d\n",
> +			__func__, from_ps, to_ps, ret);

Does this actually run in cases where the entity doesn't exist? I
would be inclined to error out if you didn't find the entity.

> +static int rt766_sdca_pde11_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +	struct rt766_sdca_priv *rt766 = snd_soc_component_get_drvdata(component);
> +	const struct sdca_entity *entity = NULL;
> +	unsigned char ps0 = 0x0, ps3 = 0x3;
> +	int from_ps, to_ps;
> +	int ret;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		regmap_write(rt766->regmap, RT766_PDE_REQ_REG(MIC, PDE11), ps0);
> +		from_ps = ps3;
> +		to_ps = ps0;
> +		break;
> +	case SND_SOC_DAPM_PRE_PMD:
> +		regmap_write(rt766->regmap, RT766_PDE_REQ_REG(MIC, PDE11), ps3);
> +		from_ps = ps0;
> +		to_ps = ps3;
> +		break;
> +	}
> +
> +	entity = rt766_find_entity_by_label(rt766->sm_func_data, "PDE 11");
> +	ret = sdca_asoc_pde_poll_actual_ps(component->dev, rt766->regmap,
> +				   RT766_FUNC_NUM_MIC,
> +				   RT766_SDCA_ENT_PDE11,
> +				   from_ps, to_ps,
> +				   entity ? entity->pde.max_delay : NULL,
> +				   entity ? entity->pde.num_max_delay : 0);
> +	if (ret)
> +		dev_err(component->dev, "%s: PDE transition %x -> %x failed, err=%d\n",
> +			__func__, from_ps, to_ps, ret);
> +
> +	return ret;
> +}

I feel like lot of these put/get and event helpers would benefit
from a parameterised helper. You have loads of functions doing
the same thing, would it be nicer to have a helper that takes
the required registers etc.

> +static int rt766_sdca_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 rt766_sdca_priv *rt766 = snd_soc_component_get_drvdata(component);
> +	struct sdw_stream_config stream_config;
> +	struct sdw_port_config port_config;
> +	enum sdw_data_direction direction;
> +	struct sdw_stream_runtime *sdw_stream;
> +	int retval, port, num_channels;
> +	unsigned int sampling_rate;
> +
> +	dev_dbg(dai->dev, "%s %s id %d", __func__, dai->name, dai->id);
> +	sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> +
> +	if (!sdw_stream)
> +		return -EINVAL;
> +
> +	if (!rt766->slave)
> +		return -EINVAL;
> +
> +	/* SoundWire specific configuration */
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		direction = SDW_DATA_DIR_RX;
> +		if (dai->id == RT766_AIF1)
> +			port = 3;
> +		else if (dai->id == RT766_AIF2)
> +			port = 1;
> +		else
> +			return -EINVAL;
> +	} else {
> +		direction = SDW_DATA_DIR_TX;
> +		if (dai->id == RT766_AIF1)
> +			port = 12;
> +		else if (dai->id == RT766_AIF3)
> +			port = 8;
> +		else
> +			return -EINVAL;
> +	}
> +
> +	stream_config.frame_rate = params_rate(params);
> +	stream_config.ch_count = params_channels(params);
> +	stream_config.bps = snd_pcm_format_width(params_format(params));
> +	stream_config.direction = direction;
> +
> +	num_channels = params_channels(params);
> +	port_config.ch_mask = GENMASK(num_channels - 1, 0);
> +	port_config.num = port;

Can you use snd_sdw_params_to_config here?

> +static int rt766_parse_rates(struct device *dev,
> +				struct sdca_function_data *function,
> +				struct sdca_entity *entity,
> +				unsigned int *out_rates)
> +{
> +	struct sdca_control_range *range;
> +	unsigned int sample_rate;
> +	unsigned int clock_rates = 0;
> +	unsigned int rates = 0;
> +	int sel, i;
> +
> +	switch (entity->type) {
> +	case SDCA_ENTITY_TYPE_IT:
> +		sel = SDCA_CTL_IT_USAGE;
> +		break;
> +	case SDCA_ENTITY_TYPE_OT:
> +		sel = SDCA_CTL_OT_USAGE;
> +		break;
> +	default:
> +		dev_err(dev, "%s: entity type has no usage control\n",
> +			entity->label);
> +		return -EINVAL;
> +	}
> +
> +	if (entity->iot.clock) {
> +		range = sdca_selector_find_range(dev, entity->iot.clock,
> +						 SDCA_CTL_CS_SAMPLERATEINDEX,
> +						 SDCA_SAMPLERATEINDEX_NCOLS, 0);
> +		if (!range)
> +			return -EINVAL;
> +
> +		for (i = 0; i < range->rows; i++) {
> +			sample_rate = sdca_range(range, SDCA_SAMPLERATEINDEX_RATE, i);
> +			clock_rates |= rate_find_mask(dev, sample_rate);
> +		}
> +	} else {
> +		clock_rates = UINT_MAX;
> +	}
> +
> +	range = sdca_selector_find_range(dev, entity, sel, SDCA_USAGE_NCOLS, 0);
> +	if (!range)
> +		return -EINVAL;
> +
> +	for (i = 0; i < range->rows; i++) {
> +		sample_rate = sdca_range(range, SDCA_USAGE_SAMPLE_RATE, i);
> +		sample_rate = rate_find_mask(dev, sample_rate);
> +
> +		if (sample_rate & clock_rates)
> +			rates |= sample_rate;
> +	}
> +
> +	*out_rates = rates;
> +
> +	dev_dbg(dev, "%s: entity %s supports rates 0x%08x, clock_rates=0x%08x\n",
> +		__func__, entity->label, rates, clock_rates);
> +
> +	return 0;
> +}

This is mostly just populate_rate_format(), the values end up in
the snd_soc_pcm_stream, can we export that and use it here?

Thanks,
Charles