Received-SPF: pass (google.com: domain of linux-kernel+bounces-126692-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <df80758b-f3d6-4456-8600-1a19fab75407@linux.intel.com>
Date: Mon, 1 Apr 2024 08:50:24 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v12] ASoc: tas2783: Add tas2783 codec driver
To: Shenghao Ding <shenghao-ding@ti.com>, broonie@kernel.org
Cc: andriy.shevchenko@linux.intel.com, lgirdwood@gmail.com, perex@perex.cz,
 13916275206@139.com, alsa-devel@alsa-project.org,
 linux-kernel@vger.kernel.org, liam.r.girdwood@intel.com,
 bard.liao@intel.com, yung-chuan.liao@linux.intel.com, kevin-lu@ti.com,
 tiwai@suse.de, baojun.xu@ti.com, soyer@irl.hu, Baojun.Xu@fpt.com,
 navada@ti.com, v-po@ti.com
References: <20240331021458.1418-1-shenghao-ding@ti.com>
Content-Language: en-US
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20240331021458.1418-1-shenghao-ding@ti.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Thank you for the update, I only have a couple of nit-picks and a set of
questions on when firmware is supposed to be downloaded. See below.
Regards,
-Pierre

> +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: dai_name %s", __func__, dai->name);

nit-pick: do you really need this sort of logs?

> +	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, "%s: Unable to configure port\n", __func__);
> +		return ret;
> +	}
> +
> +	dev_dbg(dai->dev, "%s: format: %i rate: %i\n", __func__,
> +		params_format(params), params_rate(params));
> +
> +	return 0;
> +}

> +static int tasdevice_playback(struct tasdevice_priv *tas_dev, int mute)
> +{
> +	struct regmap *map = tas_dev->regmap;
> +	int ret;
> +
> +	if (mute) {
> +		/* 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);
> +		if (ret) {
> +			dev_err(tas_dev->dev, "%s: FU23 mute failed.\n",
> +				__func__);
> +			goto out;
> +		}
> +
> +		ret = regmap_update_bits(map, TAS2873_REG_PWR_CTRL,
> +			TAS2783_REG_PWR_MODE_MASK |
> +			TAS2783_REG_AEF_MASK,
> +			TAS2783_REG_PWR_MODE_ACTIVE |
> +			TAS2783_REG_PWR_MODE_SW_PWD);
> +		if (ret) {
> +			dev_err(tas_dev->dev, "%s: shutdown failed.\n",

nit-pick: is this really a shutdown or a "PWR Mute" for symmetry with...

> +				__func__);
> +		}
> +	} 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);
> +		if (ret) {
> +			dev_err(tas_dev->dev,
> +				"%s: FU23 Unmute failed.\n", __func__);
> +			goto out;
> +		}
> +		ret = regmap_update_bits(map, TAS2873_REG_PWR_CTRL,
> +			TAS2783_REG_PWR_MODE_MASK,
> +			TAS2783_REG_PWR_MODE_ACTIVE);
> +		if (ret) {
> +			dev_err(tas_dev->dev,
> +				"%s: PWR Unmute failed.\n", __func__);

.. this log?

> +		}
> +	}
> +out:
> +	return ret;
> +}

> +static int tasdevice_comp_probe(struct snd_soc_component *comp)
> +{
> +	struct tasdevice_priv *tas_dev = snd_soc_component_get_drvdata(comp);
> +	unsigned char dspfw_binaryname[TAS2783_DSPFW_FILENAME_LEN];
> +	acpi_handle handle = ACPI_HANDLE(tas_dev->dev);
> +	const struct firmware *fw_entry = NULL;
> +	const char *sub = NULL;
> +	int ret;
> +
> +	if (handle) {
> +		sub = acpi_get_subsystem_id(handle);
> +		if (IS_ERR(sub))
> +			sub = NULL;
> +	}
> +
> +	/*
> +	 * Each tas2783 in the system has its own dspfw.
> +	 */
> +	if (comp->name_prefix) {
> +		/*
> +		 * name_prefix.bin stores the dsp firmware including speaker
> +		 * protection algorithm, audio acoustic algorithm, speaker
> +		 * characters and algorithm params, it must be copied into
> +		 * firmware folder.
> +		 */
> +		scnprintf(dspfw_binaryname, TAS2783_DSPFW_FILENAME_LEN,
> +			"%s-tas2783.bin", comp->name_prefix);
> +	} else {
> +		/* Compatible with the previous naming rule */
> +		if (sub) {
> +			/*
> +			 * subsys_id-link_id[0,1,...,N]-unique_id
> +			 * [0x8,...,0xf].bin stores the dsp firmware including
> +			 * speaker protection algorithm, audio acoustic
> +			 * algorithm, speaker characters and algorithm params,
> +			 * it must be copied into firmware folder.
> +			 */
> +			scnprintf(dspfw_binaryname, TAS2783_DSPFW_FILENAME_LEN,
> +				"%s-%d-0x%x.bin", sub,
> +				tas_dev->sdw_peripheral->bus->link_id,
> +				tas_dev->sdw_peripheral->id.unique_id);
> +		} else {
> +			/*
> +			 * tas2783-link_id[0,1,...,N]-unique_id
> +			 * [0x8,...,0xf].bin stores the dsp firmware including
> +			 * speaker protection algorithm, audio acoustic
> +			 * algorithm, speaker characters and algorithm params,
> +			 * it must be copied into firmware folder.
> +			 */
> +			scnprintf(dspfw_binaryname, TAS2783_DSPFW_FILENAME_LEN,
> +				"tas2783-%d-0x%x.bin",
> +				tas_dev->sdw_peripheral->bus->link_id,
> +				tas_dev->sdw_peripheral->id.unique_id);
> +		}
> +	}
> +
> +	ret = request_firmware(&fw_entry, dspfw_binaryname, tas_dev->dev);
> +	if (ret) {
> +		dev_err(tas_dev->dev,
> +			"%s: request_firmware %x open status: %d.\n", __func__,
> +			tas_dev->sdw_peripheral->id.unique_id, ret);
> +		goto out;
> +	}
> +
> +	tasdevice_dspfw_ready(fw_entry, tas_dev);

Question: is there a specific reason why all this functionality needs to
be done in a component probe instead of when the device reports as ATTACHED?

Since you have an interaction with userspace for the firmware, and
firmware download takes time, you would want to do this as early as
possible.

Usually the component probe is part of the card creation, so you could
add card-related or control related things. Downloading firmware does
not strike me as a card-related activity?

Another question is whether the firmware needs to be downloaded again
during system/pm_runtime resume? That may depend on how power is managed
at the hardware level, but in theory an SDCA device should report to the
host whether the firmware is still valid or needs to be downloaded.


> +
> +out:
> +	if (fw_entry)
> +		release_firmware(fw_entry);
> +	return 0;
> +}