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

[Jack Yu <jack.yu@realtek.com>]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Wednesday, April 19, 2023 10:24 PM
> To: Jack Yu <jack.yu@realtek.com>; broonie@kernel.org;
> lgirdwood@gmail.com
> Cc: alsa-devel@alsa-project.org; lars@metafoo.de; Flove(HsinFu)
> <flove@realtek.com>; Oder Chiou <oder_chiou@realtek.com>; Shuming [范書
> 銘] <shumingf@realtek.com>; Derek [方德義] <derek.fang@realtek.com>;
> Bard Liao <yung-chuan.liao@linux.intel.com>
> Subject: Re: [PATCH v2] ASoC: rt722-sdca: Add RT722 SDCA driver
> 
> 
> External mail.
> 
> 
> 
> 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 ...
> >

Ok, I'll fix it.

> > 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?

I'll add some information regarding to port configuration.

> 
> > +
> > +     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.
>

I can separate these two functions (dmic and speaker) into two DAIs like below

static struct snd_soc_dai_driver rt722_sdca_dai[] = {
	...
	{
		.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,
		},
		.ops = &rt722_sdca_ops,
	},
	{
		.name = "rt722-sdca-aif3",
		.id = RT722_AIF3,
		.capture = {
			.stream_name = "DP6 DMic Capture",
			.channels_min = 1,
			.channels_max = 2,
			.rates = RT722_STEREO_RATES,
			.formats = RT722_FORMATS,
		},
		.ops = &rt722_sdca_ops,
	}
...
};

> 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?

I'll separate the DAIs for dmic/speaker and hw_param settings will be independent as well.

> 
> > +     }
> > +
> > +     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,
> > +     }
> > +};
> 
> 
> ------Please consider the environment before printing this e-mail.