Message-ID: <35849737-dbff-4655-9062-5d5d0fd861df@sirena.org.uk>
Date:   Wed, 2 Aug 2023 14:01:17 +0100
From:   Mark Brown <broonie@...nel.org>
To:     "Baojun.Xu" <jim_monkey@....com>
Cc:     lgirdwood@...il.com, perex@...ex.cz, tiwai@...e.com,
        shenghao-ding@...com, kevin-lu@...com,
        krzysztof.kozlowski@...aro.org, rf@...nsource.cirrus.com,
        shumingf@...ltek.com, herve.codina@...tlin.com,
        povik+lin@...ebit.org, ryans.lee@...log.com,
        ckeepax@...nsource.cirrus.com, sebastian.reichel@...labora.com,
        fido_max@...ox.ru, wangweidong.a@...nic.com,
        linux-kernel@...r.kernel.org, alsa-devel@...a-project.org,
        peeyush@...com, navada@...com, tiwai@...e.de,
        mengdong.lin@...el.com, Baojun Xu <baojun.xu@...com>
Subject: Re: [PATCH v1 1/2] ASoC: tas2783: Add source files for tas2783
 soundwire driver

On Tue, Aug 01, 2023 at 10:18:57PM +0800, Baojun.Xu wrote:

> +       while (val_size) {
> +               /* to end of page */
> +               bytes = SDW_REG_NO_PAGE - (reg & SDW_REGADDR);

regmap has paging support, can't the driver use that?

> +static const struct regmap_config tasdevice_regmap = {
> +	.reg_bits = 32,
> +	.val_bits = 8,
> +	.readable_reg = tas2783_readable_register,
> +	.volatile_reg = tas2783_volatile_register,
> +	.max_register = 0x41008000 + TASDEVICE_REG(0xa1, 0x60, 0x7f),
> +	.reg_defaults = tas2783_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(tas2783_reg_defaults),
> +	.cache_type = REGCACHE_RBTREE,

Please use _MAPLE for new devices, it's more modern than _RBTREE.  It
should make little if any practical difference.

> +	.use_single_read = true,
> +	.use_single_write = true,
> +};

> +/*
> + * Registers are big-endian on I2C and SPI but little-endian on SoundWire.
> + * Exported firmware controls are big-endian on I2C/SPI but little-endian
> + * on SoundWire.

Are you sure this isn't due to running on different host architecture?

> + * Firmware files are always big-endian and are opaque blobs.
> + * Present a big-endian regmap and hide the endianness swap,
> + * so that the ALSA byte controls always have the same byte order,
> + * and firmware file blobs can be written verbatim.
> + */
> +static const struct regmap_bus tas2783_regmap_bus_sdw = {
> +	.read = tas2783_sdw_read,
> +	.write = tas2783_sdw_write,
> +	.gather_write = tas2783_sdw_gather_write,
> +	.reg_format_endian_default = REGMAP_ENDIAN_LITTLE,
> +	.val_format_endian_default = REGMAP_ENDIAN_BIG,
> +};

None of the other SoundWire devices use a custom bus, this all feels
suspicous especially since there's a bunch of bypassing of the bus in
places and calling functions directly.  I would expect everything
outside the regmap code should be able to use the regmap, possibly
excluding firmware download, and that regmap should be able to
encapsulate any differences in endianness between the different buses.
At the minute the regmap is reported as having 8 bit registers which
should mean there are no endianness issues.

> +static int tas2783_digital_getvol(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *codec
> +		= snd_soc_kcontrol_component(kcontrol);
> +	struct tasdevice_priv *tas_dev =
> +		snd_soc_component_get_drvdata(codec);
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +	int val;
> +	unsigned int invert = mc->invert;
> +	int max = mc->max;
> +	int ret = 0;
> +
> +	/* Read the primary device as the whole */
> +	ret = tasdevice_dev_read(tas_dev, &mc->reg, (unsigned int *)&val);

Why is this a custom control?

> +	val = ucontrol->value.integer.value[0];
> +	if (val > max)
> +		val = max;
> +	if (invert)
> +		val = max - val;
> +	if (val < 0)
> +		val = 0;

This is valid but it's preferable to report an error for out of range
values.

> +static int tas2783_calibration(struct tasdevice_priv *tas_priv)
> +{
> +	efi_guid_t efi_guid = EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc,
> +			0x09, 0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92);
> +	static efi_char16_t efi_name[] = L"CALI_DATA";
> +	struct tm *tm = &tas_priv->tm;
> +	unsigned int attr, crc;
> +	unsigned int *tmp_val;
> +	efi_status_t status;
> +
> +	tas_priv->cali_data.total_sz = 128;
> +	/* Get real size of UEFI variable */
> +	status = efi.get_variable(efi_name, &efi_guid, &attr,
> +		&tas_priv->cali_data.total_sz, tas_priv->cali_data.data);
> +	dev_dbg(tas_priv->dev, "cali get %lx bytes with result : %ld\n",
> +			tas_priv->cali_data.total_sz, status);
> +	if (status == EFI_BUFFER_TOO_SMALL) {
> +		status = efi.get_variable(efi_name, &efi_guid, &attr,
> +			&tas_priv->cali_data.total_sz,
> +			tas_priv->cali_data.data);
> +		dev_dbg(tas_priv->dev, "cali get %lx bytes result:%ld\n",
> +			tas_priv->cali_data.total_sz, status);
> +	}

What if there is nothing in EFI?  What if the system is not using EFI at
all?

> +	while ((offset < img_sz) && (num_files < MAX_NO_FILES)) {
> +		p = (struct MYSWFTFile *)(buf + offset);
> +		tas_dev->m_swftFile[num_files].m_vendor_id =
> +			p->m_vendor_id;
> +		tas_dev->m_swftFile[num_files].m_file_id = p->m_file_id;
> +		tas_dev->m_swftFile[num_files].m_version = p->m_version;
> +		tas_dev->m_swftFile[num_files].m_Length = p->m_Length;
> +		tas_dev->m_swftFile[num_files].m_downloadAddr =
> +			p->m_downloadAddr;
> +		tas_dev->m_swftFile[num_files].m_startAdress =
> +			((char *)p) + sizeof(unsigned int)*5;

The driver should validate that there's enough space left in the buffer
to contain the struct.

> +static int tasdevice_dapm_event(struct snd_soc_dapm_widget *w,
> +			struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_component *codec =
> +		snd_soc_dapm_to_component(w->dapm);
> +	struct tasdevice_priv *tas_dev =
> +		snd_soc_component_get_drvdata(codec);
> +	int state = 1;
> +
> +	/* Codec Lock Hold */
> +	mutex_lock(&tas_dev->codec_lock);

I'm not clear what this lock is protecting?

> +static int tasdevice_startup(struct snd_pcm_substream *substream,
> +	struct snd_soc_dai *dai)
> +{
> +	int ret = 0;
> +
> +	return ret;
> +}

Remove empty functions.

> +static int tasdevice_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> +	int clk_id, unsigned int freq, int dir)
> +{
> +	struct tasdevice_priv *tas_dev =
> +		snd_soc_dai_get_drvdata(codec_dai);
> +
> +	tas_dev->sysclk = freq;
> +
> +	return 0;
> +}

I can't see anything using the sysclk so you could just remove the
set_sysclk() operation and the driver data for it I think?

> +static int tasdevice_mute(struct snd_soc_dai *dai, int mute,
> +	int direction)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct tasdevice_priv *tasdevice =
> +		snd_soc_component_get_drvdata(component);
> +	int ret = 0;
> +	unsigned char value_sdw = 0;
> +
> +	dev_dbg(tasdevice->dev, "%s: mute %d, %d\n",
> +		__func__, mute, direction);
> +	if (mute == 0) {// Unmute.
> +		// FU21 Unmute
> +		value_sdw = 0;
> +		ret = sdw_nwrite_no_pm(tasdevice->sdw_peripheral,
> +			SDW_SDCA_CTL(1, 1, 1, 0), 1, &value_sdw);
> +		dev_dbg(tasdevice->dev,  "Unmuted %lx %x\n",
> +			SDW_SDCA_CTL(1, 1, 1, 0), ret);
> +		value_sdw = 0;
> +		ret = sdw_nwrite_no_pm(tasdevice->sdw_peripheral,
> +			0x40400090, 1, &value_sdw);
> +		dev_dbg(tasdevice->dev,  "Unmuted %x\n", ret);
> +		ret = sdw_nwrite_no_pm(tasdevice->sdw_peripheral,
> +			0x40402090, 1, &value_sdw);
> +		dev_dbg(tasdevice->dev,  "Unmuted %x\n", ret);

This feels very big/complicated for a mute/unmute operation - should
this just be a user controllable thing?  I'm a bit surprised a SoundWire
device would need a mute control, usually that's for stopping an I2S
controller outputting junk while it's starting up or shutting down but
I'd not expect that this is an issue with SoundWire.  Only one Cirrus
device has a control like that.

> +static int tasdevice_codec_probe(struct snd_soc_component *codec)
> +{
> +	struct tasdevice_priv *tas_dev =
> +		snd_soc_component_get_drvdata(codec);
> +	int ret = 0;
> +
> +	dev_dbg(tas_dev->dev, "%s called for TAS2783 start.\n",
> +		__func__);
> +	/* Codec Lock Hold */
> +	mutex_lock(&tas_dev->codec_lock);
> +
> +	crc8_populate_msb(tas_dev->crc8_lkp_tbl,
> +		TASDEVICE_CRC8_POLYNOMIAL);

I don't see anywhere where we reference this table after it is
initialised - what's it for?  Also could we do this on normal device
probe rather than waiting for the sound card to probe?

> +	tas_dev->codec = codec;
> +
> +	scnprintf(tas_dev->rca_binaryname, 64, "MY_SWFT_x%01x.bin",
> +		tas_dev->sdw_peripheral->id.unique_id);
> +
> +	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
> +		tas_dev->rca_binaryname, tas_dev->dev, GFP_KERNEL,
> +		tas_dev, tasdevice_rca_ready);
> +	dev_dbg(tas_dev->dev,
> +		"%s: request_firmware %x open status: 0x%08x\n",
> +		__func__, tas_dev->sdw_peripheral->id.unique_id, ret);

Why wait until component probe to request the firmware - it seems like
something that could be requested in normal device probe?  

> +	if (tasdevice->first_hw_init) {
> +		regcache_cache_only(tasdevice->regmap, false);
> +		regcache_cache_bypass(tasdevice->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);
> +	}

This looks like it might've been templated from some of the other
drivers which had recent bug fixes in this area - check there's no
updates needed.

> +#define CH_L	1
> +#define CH_R	2
> +


> +#define MAX_NO_FILES			100
> +#define MAX_CALIBRATION_DATA_SIZE	252

Some of these constants look like they need namespacing.

> +#define SMS_HTONL(a, b, c, d)		((((a)&0x000000FF)<<24) | \
> +					(((b)&0x000000FF)<<16) | \
> +					(((c)&0x000000FF)<<8) | \
> +					((d)&0x000000FF))

The kernel has endianness conversion macros which you should use, apart
from anything else they work for all architectures.

> +enum channel {
> +	top_left_Chn,
> +	top_right_chn,
> +	bottom_left_Chn,
> +	bottom_right_chn,
> +	max_chn,
> +};

This should be namespaced, but it's not used so could just be deleted.

> +struct MYSWFTFile {
> +	unsigned int m_vendor_id;
> +	unsigned int m_file_id;
> +	unsigned int m_version;
> +	unsigned int m_Length;
> +	unsigned int m_downloadAddr;
> +	unsigned char *m_startAdress;
> +};

Nothing about this struct fits the kernel naming style, 

> +/*
> + * This item is used to store the generic i2c address of
> + * all the tas2781 devices for I2C broadcast during the multi-device
> + *	writes, useless in mono case.
> + */
> +struct global_addr {
> +	unsigned char cur_book;
> +	unsigned int dev_addr;
> +	int ref_cnt;
> +};

It looks like this is just the SoundWire side - perhaps just delete the
I2C references for now?  Though if the I2C/SPI support is likely to be
upstreamed it'd be good to split the SoundWire specific bits into a
separate file like other drivers with multiple buses do, this makes it
much easier to handle build dependencies on the different buses.

> +struct tasdevice_irqinfo {
> +	int irq_gpio;
> +	int irq;
> +};

There was no interrupt support in the driver, just remove this until you
need it.

> +struct tasdevice_priv {
> +	struct device *dev;
> +	struct snd_soc_component *component;
> +	struct sdw_slave *sdw_slave;
> +	enum sdw_slave_status status;
> +	struct sdw_bus_params params;
> +	struct regmap *regmap;
> +	struct mutex codec_lock;
> +	struct mutex dev_lock;
> +	int rst_gpio;
> +	struct tasdevice tasdevice[max_chn];

Why is there both a struct tasdevice and tasdevice_priv?  It looks like
there's some support for trying to group devices but it's not fully
implemented, probably best to just delete all that until you add the
multiple device support - it makes everything simpler not to have to
worry about it.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists