Re: [PATCH v2] ASoC: rt722-sdca: Add RT722 SDCA driver

On 4/19/23 05:15, Jack Yu wrote:

This is the initial codec driver for rt722-sdca.

Patch v2 is to fix warning message from kernel test robot.

this version information should go below the --- line ...

Signed-off-by: Jack Yu jack.yu@realtek.com

... here

+static int rt722_sdca_read_prop(struct sdw_slave *slave) +{

• struct sdw_slave_prop *prop = &slave->prop;
• int nval;
• int i, j;
• u32 bit;
• unsigned long addr;
• struct sdw_dpn_prop *dpn;
• prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
• prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
• prop->paging_support = true;
• /* first we need to allocate memory for set bits in port lists */
• prop->source_ports = BIT(6) | BIT(2); /* BITMAP: 01000100 */
• prop->sink_ports = BIT(3) | BIT(1); /* BITMAP: 00001010 */

which port is used for what?

• nval = hweight32(prop->source_ports);
• prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,

• sizeof(*prop->src_dpn_prop), GFP_KERNEL);
  

• if (!prop->src_dpn_prop)

• return -ENOMEM;
  

• i = 0;
• dpn = prop->src_dpn_prop;
• addr = prop->source_ports;
• for_each_set_bit(bit, &addr, 32) {

• dpn[i].num = bit;
  

• dpn[i].type = SDW_DPN_FULL;
  

• dpn[i].simple_ch_prep_sm = true;
  

• dpn[i].ch_prep_timeout = 10;
  

• i++;
  

• }
• /* do this again for sink now */
• nval = hweight32(prop->sink_ports);
• prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,

• sizeof(*prop->sink_dpn_prop), GFP_KERNEL);
  

• if (!prop->sink_dpn_prop)

• return -ENOMEM;
  

• j = 0;
• dpn = prop->sink_dpn_prop;
• addr = prop->sink_ports;
• for_each_set_bit(bit, &addr, 32) {

• dpn[j].num = bit;
  

• dpn[j].type = SDW_DPN_FULL;
  

• dpn[j].simple_ch_prep_sm = true;
  

• dpn[j].ch_prep_timeout = 10;
  

• j++;
  

• }
• /* set the timeout values */
• prop->clk_stop_timeout = 200;
• /* wake-up event */
• prop->wake_capable = 1;
• return 0;

+}

+static int rt722_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 rt722_sdca_priv *rt722 = 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", __func__, dai->name);
• sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
• if (!sdw_stream)

• return -EINVAL;
  

• if (!rt722->slave)

• return -EINVAL;
  

• /*

• * RT722_AIF1 with port = 1 for headphone playback
  

• * RT722_AIF1 with port = 2 for headset-mic capture
  

• * RT722_AIF2 with port = 3 for speaker playback
  

• * RT722_AIF2 with port = 6 for digital-mic capture
  

• */
  

I guess the answer is here...

It wouldn't hurt to have the information above as well.

It's also an interesting partition because in theory the amplifier and mic 'functions' are supposed to be independent, yet they are on the same DAI.

Bard, would this work for the machine driver integration?

• if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {

• direction = SDW_DATA_DIR_RX;
  

• if (dai->id == RT722_AIF1)
  

• 	port = 1;
  

• else if (dai->id == RT722_AIF2)
  

• 	port = 3;
  

• else
  

• 	return -EINVAL;
  

• } else {

• direction = SDW_DATA_DIR_TX;
  

• if (dai->id == RT722_AIF1)
  

• 	port = 2;
  

• else if (dai->id == RT722_AIF2)
  

• 	port = 6;
  

• 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;
• retval = sdw_stream_add_slave(rt722->slave, &stream_config,

• 			&port_config, 1, sdw_stream);
  

• if (retval) {

• dev_err(dai->dev, "Unable to configure port\n");
  

• return retval;
  

• }
• if (params_channels(params) > 16) {

• dev_err(component->dev, "Unsupported channels %d\n",
  

• 	params_channels(params));
  

• return -EINVAL;
  

• }
• /* sampling rate configuration */
• switch (params_rate(params)) {
• case 44100:

• sampling_rate = RT722_SDCA_RATE_44100HZ;
  

• break;
  

• case 48000:

• sampling_rate = RT722_SDCA_RATE_48000HZ;
  

• break;
  

• case 96000:

• sampling_rate = RT722_SDCA_RATE_96000HZ;
  

• break;
  

• case 192000:

• sampling_rate = RT722_SDCA_RATE_192000HZ;
  

• break;
  

• default:

• dev_err(component->dev, "Rate %d is not supported\n",
  

• 	params_rate(params));
  

• return -EINVAL;
  

• }
• /* set sampling frequency */
• if (dai->id == RT722_AIF1) {

• regmap_write(rt722->regmap,
  

• 	SDW_SDCA_CTL(FUNC_NUM_JACK_CODEC, RT722_SDCA_ENT_CS01,
  

• 		RT722_SDCA_CTL_SAMPLE_FREQ_INDEX, 0), sampling_rate);
  

• regmap_write(rt722->regmap,
  

• 	SDW_SDCA_CTL(FUNC_NUM_JACK_CODEC, RT722_SDCA_ENT_CS11,
  

• 		RT722_SDCA_CTL_SAMPLE_FREQ_INDEX, 0), sampling_rate);
  

• }
• if (dai->id == RT722_AIF2) {

• regmap_write(rt722->regmap,
  

• 	SDW_SDCA_CTL(FUNC_NUM_MIC_ARRAY, RT722_SDCA_ENT_CS1F,
  

• 		RT722_SDCA_CTL_SAMPLE_FREQ_INDEX, 0), sampling_rate);
  

• regmap_write(rt722->regmap,
  

• 	SDW_SDCA_CTL(FUNC_NUM_AMP, RT722_SDCA_ENT_CS31,
  

• 		RT722_SDCA_CTL_SAMPLE_FREQ_INDEX, 0), sampling_rate);
  

and that's precisely the sort of problems I had in mind earlier. Why would the sample-rate be aligned for both amplifier and dmic?

I don't think this follows the intent of the SDCA spec. The functions are supposed to be independent, so when we set hw_params for e.g. amplifiers we can't touch the microphone function.

I would recommend splitting the DAIs here to have self-contained operations that preserve the independence between functions - if the hardware can deal with independent functions we have no reason to rejoin these functions at the driver level, do we?

• }
• return 0;

+}

+static int rt722_sdca_pcm_hw_free(struct snd_pcm_substream *substream,

• 		struct snd_soc_dai *dai)
  

+{

• struct snd_soc_component *component = dai->component;
• struct rt722_sdca_priv *rt722 = snd_soc_component_get_drvdata(component);
• struct sdw_stream_runtime *sdw_stream =

• snd_soc_dai_get_dma_data(dai, substream);
  

• if (!rt722->slave)

• return -EINVAL;
  

• sdw_stream_remove_slave(rt722->slave, sdw_stream);
• return 0;

+}

+#define RT722_STEREO_RATES (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | \

• 	SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_192000)
  

+#define RT722_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \

• 	SNDRV_PCM_FMTBIT_S24_LE)
  

+static const struct snd_soc_dai_ops rt722_sdca_ops = {

• .hw_params = rt722_sdca_pcm_hw_params,
• .hw_free = rt722_sdca_pcm_hw_free,
• .set_stream = rt722_sdca_set_sdw_stream,
• .shutdown = rt722_sdca_shutdown,

+};

+static struct snd_soc_dai_driver rt722_sdca_dai[] = {

• {

• .name = "rt722-sdca-aif1",
  

• .id = RT722_AIF1,
  

• .playback = {
  

• 	.stream_name = "DP1 Headphone Playback",
  

• 	.channels_min = 1,
  

• 	.channels_max = 2,
  

• 	.rates = RT722_STEREO_RATES,
  

• 	.formats = RT722_FORMATS,
  

• },
  

• .capture = {
  

• 	.stream_name = "DP2 Headset Capture",
  

• 	.channels_min = 1,
  

• 	.channels_max = 2,
  

• 	.rates = RT722_STEREO_RATES,
  

• 	.formats = RT722_FORMATS,
  

• },
  

• .ops = &rt722_sdca_ops,
  

• },
• {

• .name = "rt722-sdca-aif2",
  

• .id = RT722_AIF2,
  

• .playback = {
  

• 	.stream_name = "DP3 Speaker Playback",
  

• 	.channels_min = 1,
  

• 	.channels_max = 2,
  

• 	.rates = RT722_STEREO_RATES,
  

• 	.formats = RT722_FORMATS,
  

• },
  

• .capture = {
  

• 	.stream_name = "DP6 DMic Capture",
  

• 	.channels_min = 1,
  

• 	.channels_max = 2,
  

• 	.rates = RT722_STEREO_RATES,
  

• 	.formats = RT722_FORMATS,
  

• },
  

• .ops = &rt722_sdca_ops,
  

• }

+};