> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Wednesday, April 19, 2023 10:24 PM
> To: Jack Yu <jack.yu@xxxxxxxxxxx>; broonie@xxxxxxxxxx;
> lgirdwood@xxxxxxxxx
> Cc: alsa-devel@xxxxxxxxxxxxxxxx; lars@xxxxxxxxxx; Flove(HsinFu)
> <flove@xxxxxxxxxxx>; Oder Chiou <oder_chiou@xxxxxxxxxxx>; Shuming [范書
> 銘] <shumingf@xxxxxxxxxxx>; Derek [方德義] <derek.fang@xxxxxxxxxxx>;
> Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> > ---
>
> ... 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.
