linux-kernel - Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's TSCS42xx audio CODEC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171212163254.GL16323@sirena.org.uk>
Date:   Tue, 12 Dec 2017 16:32:54 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Steven Eckhoff <steven.eckhoff.opensource@...il.com>
Cc:     alsa-devel@...a-project.org, Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] ASoC: TSCS42xx: Add support for Tempo Semiconductor's
 TSCS42xx audio CODEC

On Mon, Dec 11, 2017 at 01:54:25PM -0600, Steven Eckhoff wrote:
> Currently there is no support for the TSCS42xx audio CODEC.
> 
> Add support for it.
> 
> v5 attempts to address all issues raised in the previous reviews.
> 
> Thank you to everyone who has invested their time reviewing these
> patches.

Please add inter-version changelogs and commentary like this after --- 
as covered in SubmittingPatches - this lets tools know they don't need
to end up in git.

> +  - compatible : "tscs:tscs42xx"

Compatible strings should be for specific devices, they may all be
treated identically by software now but may not be in future.

> +		do {
> +			ret = snd_soc_read(codec, R_DACCRSTAT);
> +			if (ret < 0) {
> +				dev_err(codec->dev,
> +					"Failed to read stat (%d)\n", ret);
> +				return ret;
> +			}
> +		} while (ret);

There should be some sort of upper bound on how many times we'll try to
wait for this in case the hardware fails somehow.

> +static int tscs42xx_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	int ret;
> +
> +	if (mute)
> +		if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			ret = dac_mute(codec);
> +		else
> +			ret = adc_mute(codec);
> +	else
> +		if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> +			ret = dac_unmute(codec);
> +		else
> +			ret = adc_unmute(codec);
> +
> +	return ret;
> +}

All these mute functions also shut down the PLLs which since...

> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +		ret = snd_soc_update_bits(codec, R_AIC1, RM_AIC1_MS,
> +				RV_AIC1_MS_MASTER);
> +		if (ret < 0)
> +			dev_err(codec->dev,
> +				"Failed to set codec DAI master (%d)\n", ret);
> +		else
> +			ret = 0;
> +		break;
> +	default:

...we only support the CODEC being the clock master seems like it might
mean we stop clocking the DAI?  If that's the case it's better to just
not have the mute control and allow the user to just control these as
normal mutes.

> +static int tscs42xx_probe(struct snd_soc_codec *codec)
> +{
> +	int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(r_inits); ++i) {
> +		ret = snd_soc_write(codec, r_inits[i].reg, r_inits[i].def);
> +		if (ret < 0) {
> +			dev_err(codec->dev,
> +				"Failed to write codec defaults (%d)\n", ret);
> +			return ret;
> +		}
> +	}

I'd expect the driver to just reset the CODEC (it appears to have that
feature) and the regmap.

> +static int tscs42xx_remove(struct snd_soc_codec *codec)
> +{
> +	return 0;
> +}

Just remove empty functions.

> +static const struct i2c_device_id tscs42xx_i2c_id[] = {
> +	{ "tscs42xx", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tscs42xx_i2c_id);

I2C IDs are better as explicit part numbers too like compatible
strings.

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