Date: Tue, 16 Jan 2024 08:00:26 +0000
From: "Ding, Shenghao" <shenghao-ding@...com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
CC: "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "perex@...ex.cz"
	<perex@...ex.cz>,
        "13916275206@....com" <13916275206@....com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "liam.r.girdwood@...el.com" <liam.r.girdwood@...el.com>,
        "mengdong.lin@...el.com" <mengdong.lin@...el.com>,
        "yung-chuan.liao@...ux.intel.com" <yung-chuan.liao@...ux.intel.com>,
        "Xu,
 Baojun" <baojun.xu@...com>, "Lu, Kevin" <kevin-lu@...com>,
        "Gupta, Peeyush"
	<peeyush@...com>,
        "Navada Kanyana, Mukund" <navada@...com>,
        "tiwai@...e.de"
	<tiwai@...e.de>,
        "broonie@...nel.org" <broonie@...nel.org>
Subject: RE: [EXTERNAL] Re: [PATCH v5] ASoc: tas2783: Add tas2783 codec driver

Thanks for your review comment.

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> Sent: Monday, January 15, 2024 5:34 PM
> To: Ding, Shenghao <shenghao-ding@...com>; broonie@...nel.org
> Cc: andriy.shevchenko@...ux.intel.com; lgirdwood@...il.com;
> perex@...ex.cz; 13916275206@....com; alsa-devel@...a-project.org;
> linux-kernel@...r.kernel.org; liam.r.girdwood@...el.com;
> mengdong.lin@...el.com; yung-chuan.liao@...ux.intel.com; Xu, Baojun
> <baojun.xu@...com>; Lu, Kevin <kevin-lu@...com>; Gupta, Peeyush
> <peeyush@...com>; Navada Kanyana, Mukund <navada@...com>;
> tiwai@...e.de
> Subject: [EXTERNAL] Re: [PATCH v5] ASoc: tas2783: Add tas2783 codec
> driver
> 
> This version is much better than previous ones, but can be improved further,
> specifically on the identification of firmware based on the unique_id and
> pm_runtime. See additional nit-picks and suggestions below.
> 
> 
> > ---
> > Change in v5:
> >  - simplify tasdevice_set_sdw_stream
> >  - fixed some Linux coding style
> >  - fixed the spelling mistakes
> >  - Select left/right channel based on unique id
> >  - a longer description has been added
> >  - remove unused crc8 in KCONFIG
> 
> Don't you need a 'select CRC32' though? ...
> 
> Maybe it's already set by other Kconfigs.
> 
> 
> > +/* Update the calibrate data, including speaker impedance, f0, etc, into
> algo.
> > + * Calibrate data is done by manufacturer in the factory. These data
> > +are used
> > + * by Algo for calucating the speaker temperature, speaker membrance
> > +excursion
> > + * and f0 in real time during playback.
> > + * In case of no or valid calibrated data, dsp will still works with
> > +default
> > + * calibrated data inside algo.
> 
> suggested edits:
> 
> Load the calibration data, including speaker impedance, f0, etc.
> Calibration is done by the manufacturer in the factory. The calibration data
> are used by the algorithm  for calculating the speaker temperature, speaker
> membrance excursion and f0 in real time during playback.
> The DSP will work with default data values if calibrated data are missing or
> are invalid.
> 
> 
> > +static int tasdevice_sdw_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 tasdevice_priv *tas_priv =
> > +		snd_soc_component_get_drvdata(component);
> > +	struct sdw_stream_config stream_config = {0};
> > +	struct sdw_port_config port_config = {0};
> > +	struct sdw_stream_runtime *sdw_stream;
> > +	int ret;
> > +
> > +	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 (!tas_priv->sdw_peripheral)
> > +		return -EINVAL;
> > +
> > +	/* SoundWire specific configuration */
> > +	snd_sdw_params_to_config(substream, params,
> > +		&stream_config, &port_config);
> > +
> > +	/* port 1 for playback */
> > +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> > +		port_config.num = 1;
> > +	else
> > +		port_config.num = 2;
> > +
> > +	ret = sdw_stream_add_slave(tas_priv->sdw_peripheral,
> > +		&stream_config, &port_config, 1, sdw_stream);
> > +	if (ret) {
> > +		dev_err(dai->dev, "Unable to configure port\n");
> > +		return ret;
> > +	}
> > +
> > +	dev_dbg(dai->dev, "%s fomrat: %i rate: %i\n", __func__,
> 
> typo: format
> 
> > +		params_format(params), params_rate(params));
> > +
> > +	return 0;
> > +}
> > +
> 
> > +static int tasdevice_mute(struct snd_soc_dai *dai, int mute,
> > +	int direction)
> > +{
> > +	struct snd_soc_component *component = dai->component;
> > +	struct tasdevice_priv *tas_dev =
> > +		snd_soc_component_get_drvdata(component);
> > +	struct regmap *map = tas_dev->regmap;
> > +	int ret;
> > +
> > +	dev_dbg(tas_dev->dev, "Mute or unmute %d.\n", mute);
> > +
> > +	if (mute) {
> > +		/* Echo channel can't be shutdown while tas2783 must keep
> > +		 * working state while playback is on.
> 
> Consider rewording that comment, two 'while' in the same sentence make it
> hard to parse.
> 
> > +		 */
> > +		if (direction == SNDRV_PCM_STREAM_CAPTURE
> > +			&& tas_dev->pstream == true)
> > +			return 0;
> > +		/* FU23 mute (0x40400108) */
> > +		ret = regmap_write(map,
> > +			SDW_SDCA_CTL(TAS2783_FUNC_TYPE_SMART_AMP,
> > +			TAS2783_SDCA_ENT_FU23,
> TAS2783_SDCA_CTL_FU_MUTE, 0),
> > +			1);
> > +		ret |= regmap_write(map, TASDEVICE_REG(0, 0, 0x02), 0x1a);
> 
> ORin error codecs is not quite right
> 
> > +		tas_dev->pstream = false;
> > +	} else {
> > +		/* FU23 Unmute, 0x40400108. */
> > +		ret = regmap_write(map,
> > +			SDW_SDCA_CTL(TAS2783_FUNC_TYPE_SMART_AMP,
> > +			TAS2783_SDCA_ENT_FU23,
> TAS2783_SDCA_CTL_FU_MUTE, 0),
> > +			0);
> > +		ret |= regmap_write(map, TASDEVICE_REG(0, 0, 0x02), 0x0);
> > +		if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> > +			tas_dev->pstream = true;
> > +	}
> > +
> > +	if (ret)
> > +		dev_err(tas_dev->dev, "Mute or unmute %d failed %d.\n",
> > +			mute, ret);
> > +
> > +	return ret;
> > +}
> > +
> 
> > +static int tasdevice_init(struct tasdevice_priv *tas_dev) {
> > +	int ret;
> > +
> > +	dev_set_drvdata(tas_dev->dev, tas_dev);
> > +
> > +	mutex_init(&tas_dev->codec_lock);
> > +	ret = devm_snd_soc_register_component(tas_dev->dev,
> > +		&soc_codec_driver_tasdevice,
> > +		tasdevice_dai_driver, ARRAY_SIZE(tasdevice_dai_driver));
> > +	if (ret) {
> > +		dev_err(tas_dev->dev, "%s: codec register error:%d.\n",
> > +			__func__, ret);
> > +	}
> > +
> > +	tas2783_reset(tas_dev);
> > +	/* tas2783-8[9,...,f].bin was copied into /lib/firmware/ */
> 
> You need to add a comment to explain what those files contain, it's not clear
> at all if they are just 'firmware' or if they contain tables.
> 
> > +	scnprintf(tas_dev->rca_binaryname, 64, "tas2783-%01x.bin",
> > +		tas_dev->sdw_peripheral->id.unique_id);
> 
> And more specifically the use of the unique_id is problematic since it's only
> relevant in the context of one link. If you have two amps on separate links,
> the unique_id is irrelevant.
> 
> > +
> > +	ret = request_firmware_nowait(THIS_MODULE,
> FW_ACTION_UEVENT,
> > +		tas_dev->rca_binaryname, tas_dev->dev, GFP_KERNEL,
> > +		tas_dev, tasdevice_rca_ready);
> > +	if (ret) {
> > +		dev_dbg(tas_dev->dev,
> > +		"%s: request_firmware %x open status: %d.\n",
> > +		__func__, tas_dev->sdw_peripheral->id.unique_id, ret);
> > +	}
> > +
> > +	/* set autosuspend parameters */
> > +	pm_runtime_set_autosuspend_delay(tas_dev->dev, 3000);
> > +	pm_runtime_use_autosuspend(tas_dev->dev);
> > +
> > +	/* make sure the device does not suspend immediately */
> > +	pm_runtime_mark_last_busy(tas_dev->dev);
> > +
> > +	pm_runtime_enable(tas_dev->dev);
> 
> You are handling pm_runtime here...
> 
> > +
> > +	dev_dbg(tas_dev->dev, "%s was called for TAS2783.\n",  __func__);
> > +
> > +	return ret;
> > +}
> > +
> > +static int tasdevice_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(2); /* BITMAP: 00000100 */
> > +	prop->sink_ports = BIT(1); /* BITMAP:  00000010 */
> > +
> > +	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 = 20;
> > +
> > +	return 0;
> > +}
> > +
> > +static int tasdevice_io_init(struct device *dev, struct sdw_slave
> > +*slave) {
> > +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> > +
> > +	if (tas_priv->hw_init)
> > +		return 0;
> > +
> > +	if (tas_priv->first_hw_init) {
> > +		regcache_cache_only(tas_priv->regmap, false);
> > +		regcache_cache_bypass(tas_priv->regmap, true);
> > +	} else {
> > +		/*
> > +		 * PM runtime is only enabled when a Slave reports as
> Attached
> > +		 */
> > +
> > +		/* set autosuspend parameters */
> > +		pm_runtime_set_autosuspend_delay(&slave->dev, 3000);
> > +		pm_runtime_use_autosuspend(&slave->dev);
> > +
> > +		/* update count of parent 'active' children */
> > +		pm_runtime_set_active(&slave->dev);
> > +
> > +		/* make sure the device does not suspend immediately */
> > +		pm_runtime_mark_last_busy(&slave->dev);
> > +
> > +		pm_runtime_enable(&slave->dev);
> 
> ... and here.
> 
> This isn't quite right, you should only use set_active() here and move all other
> parts in the probe.
> 
> > +	}
> > +
> > +	pm_runtime_get_noresume(&slave->dev);
> > +
> > +	/* sw reset */
> > +	regmap_write(tas_priv->regmap, 0x8001, 0x01);
> > +
> > +	if (tas_priv->first_hw_init) {
> > +		regcache_cache_bypass(tas_priv->regmap, false);
> > +		regcache_mark_dirty(tas_priv->regmap);
> > +	} else
> > +		tas_priv->first_hw_init = true;
> > +	/* Mark Slave initialization complete */
> > +	tas_priv->hw_init = true;
> 
> if you do a get_noresume() you've got to do a put_noidle() here for
> symmetry.
> 
> > +
> > +	return 0;
> > +}
> > +

Powered by blists - more mailing lists