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

[Mark Brown]

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.

Attachment: signature.asc
Description: PGP signature