RE: [PATCH v2 3/3] ASoC: rt766: add RT766/RT767 SDCA driver

> > +config SND_SOC_RT766_SDCA_SDW
> > +	tristate "Realtek RT766 SDCA Codec - SDW"
> > +	depends on SOUNDWIRE
> > +	depends on SND_SOC_SDCA
> > +	select REGMAP_SOUNDWIRE
> > +	select REGMAP_SOUNDWIRE_MBQ
> > +
> 
> Does this need SND_SOC_SDCA_IRQ and SND_SOC_SDCA_HID?

Yes, will select this two configs.

> > +static int rt766_sdca_mbq_size(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case RT766_VOLUME_REG(UAJ, USER_FU41, 1):
> > +	case RT766_VOLUME_REG(UAJ, USER_FU41, 2):
> > +	case RT766_VOLUME_REG(UAJ, USER_FU36, 1):
> > +	case RT766_VOLUME_REG(UAJ, USER_FU36, 2):
> > +	case RT766_VOLUME_REG(MIC, USER_FU113, 1):
> > +	case RT766_VOLUME_REG(MIC, USER_FU113, 2):
> > +	case RT766_VOLUME_REG(MIC, USER_FU113, 3):
> > +	case RT766_VOLUME_REG(MIC, USER_FU113, 4):
> > +	case RT766_VOLUME_REG(AMP, USER_FU21, 1):
> > +	case RT766_VOLUME_REG(AMP, USER_FU21, 2):
> > +		return 2;
> 
> Are the FU33 gain registers also 2 bytes?

Yes, will add FU33 gain registers here.

> > +static void rt766_sdca_sdw_remove(struct sdw_slave *slave) {
> > +	struct rt766_sdca_priv *rt766 = dev_get_drvdata(&slave->dev);
> > +
> > +	pm_runtime_disable(&slave->dev);
> > +
> > +	mutex_destroy(&rt766->disable_irq_lock);
> > +}
> 
> The mutex is used by the interrupt code, probably better to allocate it using
> devm so it's ordered with everything else during teardown.

will fix. Thanks.

> > +static int rt766_sdca_pcm_hw_params(struct snd_pcm_substream
> *substream,
> > +				struct snd_pcm_hw_params *params,
> > +				struct snd_soc_dai *dai)
> > +{
> 
> > +
> > +	/* set sampling frequency */
> > +	switch (dai->id) {
> > +	case RT766_AIF1:
> > +		regmap_write(rt766->regmap,
> > +			RT766_SDCA_CTL(UAJ, CS41,
> SDCA_CTL_CS_SAMPLERATEINDEX),
> > +			sampling_rate);
> > +		regmap_write(rt766->regmap,
> > +			RT766_SDCA_CTL(UAJ, CS36,
> SDCA_CTL_CS_SAMPLERATEINDEX),
> > +			sampling_rate);
> > +		break;
> 
> Are these separate playback/capture clocks?  If so should we only be
> programming the one that's active, or if they have to be in sync should the DAI
> be flagged as needing symmetric rates?

Will add symmetric_rate = 1 for this DAI.

> > +int rt766_sdca_init(struct device *dev, struct regmap *regmap, struct
> > +sdw_slave *slave) {
> 
> > +			rates = rt766_find_dt_rates(dev, func_data_ptr, "IT 41");
> > +			if (rates)
> > +				rt766_sdca_dai[RT766_DAI_UAJ].playback.rates = rates;
> > +
> > +			rates = rt766_find_dt_rates(dev, func_data_ptr, "OT 36");
> > +			if (rates)
> > +				rt766_sdca_dai[RT766_DAI_UAJ].capture.rates = rates;
> 
> This should modify a local copy rather than the global in case there's multiple
> devices with different configs.

Will fix.