Received-SPF: pass (google.com: domain of linux-kernel+bounces-56793-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <b864e6d7-1e37-4901-b934-1e074d3c0b6c@linux.intel.com>
Date: Wed, 7 Feb 2024 10:45:51 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7] ASoc: tas2783: Add tas2783 codec driver
Content-Language: en-US
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, mengdong.lin@intel.com,
 yung-chuan.liao@linux.intel.com, baojun.xu@ti.com, kevin-lu@ti.com,
 navada@ti.com, tiwai@suse.de, soyer@irl.hu
References: <20240207054743.1504-1-shenghao-ding@ti.com>
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20240207054743.1504-1-shenghao-ding@ti.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Much better than previous versions but still questions on pm_runtime and
a big undocumented part on the unique_id use. see comments below
-Pierre


> +config SND_SOC_TAS2783
> +	tristate "Texas Instruments TAS2783 speaker amplifier (sdw)"
> +	depends on SOUNDWIRE
> +	depends on EFI
> +	select REGMAP
> +	select REGMAP_SOUNDWIRE

nit-pick: should have 'select CRC32'...

> +	select CRC32_SARWATE

before selecting the CRC32 implementation. It's also not clear if this
is needed, the help says

 "Only choose this option if you know what you are doing."

> +static void tas2783_apply_calib(struct tasdevice_priv *tas_dev,
> +	unsigned int *cali_data)
> +{
> +	struct regmap *map = tas_dev->regmap;
> +	u8 *reg_start;
> +	int ret;
> +
> +	if (!tas_dev->sdw_peripheral) {
> +		dev_err(tas_dev->dev, "%s, slaver doesn't exist.\n",

"peripheral does not exist"

> +			__func__);
> +		return;
> +	}
> +	if ((tas_dev->sdw_peripheral->id.unique_id < TAS2783_ID_MIN) ||
> +		(tas_dev->sdw_peripheral->id.unique_id > TAS2783_ID_MAX)) {

Where does this unique_id requirement come from?

I see this in the header files, and that means only half of the bits are
supported?

+/* Unique id start */
+#define TAS2783_ID_MIN			0x08
+/* Unique id end */
+#define TAS2783_ID_MAX			0x0f

the unique_id is only meant to allow identical devices to work
concurrently on the same link, specifically it enables the enumeration
of identical devices with the hardware arbitration. The device with the
highest unique_id is enumerated first in case of conflicts.

The unique_id is usually set at the board level. I don't know how the
codec driver can enforce a specific value.

This needs more explanations....

> +		dev_err(tas_dev->dev, "%s, error unique_id.\n",
> +			__func__);
> +		return;
> +	}
> +
> +	reg_start = (u8 *)(cali_data + (tas_dev->sdw_peripheral->id.unique_id
> +		- TAS2783_ID_MIN) * sizeof(tas2783_cali_reg));
> +	for (int i = 0; i < ARRAY_SIZE(tas2783_cali_reg); i++) {
> +		ret = regmap_bulk_write(map, tas2783_cali_reg[i],
> +			&reg_start[4 * i], 4);
> +		if (ret) {
> +			dev_err(tas_dev->dev, "Cali failed %x:%d\n",
> +				tas2783_cali_reg[i], ret);
> +			break;
> +		}
> +	}
> +}

> +static void tasdevice_rca_ready(const struct firmware *fmw,
> +	void *context)
> +{
> +	struct tasdevice_priv *tas_dev =
> +		(struct tasdevice_priv *) context;
> +	struct tas2783_firmware_node *p;
> +	struct regmap *map = tas_dev->regmap;
> +	unsigned char *buf = NULL;
> +	int offset = 0, img_sz;
> +	int ret, value_sdw;
> +
> +	if (!fmw || !fmw->data) {
> +		/* No firmware binary, devices will work in ROM mode. */
> +		dev_err(tas_dev->dev,
> +			"Failed to read %s, no side-effect on driver\n",
> +			tas_dev->rca_binaryname);
> +		ret = -EINVAL;

If this is not an error, it should be dev_info or dev_warn?

> +		goto out;
> +	}
> +	buf = (unsigned char *)fmw->data;
> +
> +	img_sz = le32_to_cpup((__le32 *)&buf[offset]);
> +	offset  += sizeof(img_sz);
> +	if (img_sz != fmw->size) {
> +		dev_err(tas_dev->dev, "Size not matching, %d %u",
> +			(int)fmw->size, img_sz);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	while (offset < img_sz) {
> +		p = (struct tas2783_firmware_node *)(buf + offset);
> +		if (p->length > 1) {
> +			ret = regmap_bulk_write(map, p->download_addr,
> +			buf + offset + sizeof(unsigned int)*5, p->length);
> +		} else
> +			ret = regmap_write(map, p->download_addr,
> +				*(buf + offset + sizeof(unsigned int) * 5));
> +
> +		if (ret != 0) {
> +			dev_dbg(tas_dev->dev, "Load FW fail: %d.\n", ret);
> +			goto out;
> +		}
> +		offset += sizeof(unsigned int)*5 + p->length;
> +	}
> +	/* Select left/right channel based on unique id. */
> +	value_sdw = 0x1a;
> +	value_sdw |= ((tas_dev->sdw_peripheral->dev_num & 1) << 4);

This is a very odd sequence, please add commments on what those bits
mean. It looks like this is confusing unique id and device number. Not
the same thing at all! The unique_id is set at the board level and used
during enumeration only, the dev_num is used as a logical value for
command/control. The dev_num is assigned in drivers/soundwire/bus.c and
depends on multiple things (order of attachment, allocation policy on
this host, etc). The codec driver cannot assume any specific value for
dev_num.

> +	dev_dbg(tas_dev->dev, "%s dev_num = %u", __func__,
> +		tas_dev->sdw_peripheral->dev_num);
> +	regmap_write(map, TAS2783_REG_TDM_RX_CFG, value_sdw);
> +
> +	tas2783_calibration(tas_dev);
> +
> +out:
> +	if (fmw)
> +		release_firmware(fmw);
> +}

> +static int tasdevice_init(struct tasdevice_priv *tas_dev)
> +{
> +	int ret;
> +
> +	dev_set_drvdata(tas_dev->dev, tas_dev);
> +
> +	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);
> +		goto out;
> +	}
> +
> +	/* tas2783-link_id[0,1,...,N]-unique_id[8,9,...,f].bin stores the dsp
> +	 * firmware including speaker protection algorithm, audio acoustic
> +	 * algorithm, speaker characters and algorithm params, it must be
> +	 * copied into firmware folder. Each tas2783 has its own bin file.
> +	 */
> +	scnprintf(tas_dev->rca_binaryname, 64, "tas2783-%d-%x.bin",
> +		tas_dev->sdw_peripheral->bus->link_id,
> +		tas_dev->sdw_peripheral->id.unique_id);

Goodness, again this unique_id usage.

This is really problematic, how would this work for a Linux
distribution? Fetching the firmware ONLY on the basis of a unique_id
means possible collisions between platformA from OEM_X and platformB
from OEM_Y.

> +
> +	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_err(tas_dev->dev,
> +			"%s: request_firmware %x open status: %d.\n",
> +			__func__, tas_dev->sdw_peripheral->id.unique_id, ret);
> +		goto out;
> +	}
> +
> +	/* 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_get_noresume(tas_dev->dev);

why are you increasing the refcount here? No other SoundWire codec
driver does this.

> +	pm_runtime_enable(tas_dev->dev);
> +
> +out:
> +	return ret;
> +}

> +static int tasdevice_sdw_remove(struct sdw_slave *peripheral)
> +{
> +	struct tasdevice_priv *tas_dev = dev_get_drvdata(&peripheral->dev);
> +
> +	if (tas_dev->first_hw_init)
> +		pm_runtime_disable(tas_dev->dev);
> +
> +	pm_runtime_put_noidle(tas_dev->dev);

that should be removed as well, this and the previous get_no_resume()
guarantee that pm_runtime suspend *NEVER* happens...

> +	return 0;
> +}

> +/* Unique id start */
> +#define TAS2783_ID_MIN			0x08
> +/* Unique id end */
> +#define TAS2783_ID_MAX			0x0f

this needs a lot more documentation, there's really nothing in the
SoundWire spec that allows for the unique_id to be restricted...