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

[Mark Brown]

On Tue, Nov 14, 2017 at 12:34:08PM -0600, Steven Eckhoff wrote:

> Currently there is no support for the TSCS42xx audio CODEC.

This driver is doing a lot of weird, non-standard stuff without any
explanation - quite a bit of this needs to be refactored to use standard
interfaces.  The code is mostly clean but if it doesn't fit in well with
the rest of the system that's an issue.

Please submit patches using subject lines reflecting the style for the
subsystem.  This makes it easier for people to identify relevant
patches.  Look at what existing commits in the area you're changing are
doing and make sure your subject lines visually resemble what they're
doing.

> +What:		/sys/bus/i2c/devices/.../dsp/eq[n]_ch[n]_[ab][n]
> +Date:		November 2017
> +KernelVersion:	4.14
> +Contact:	Steven Eckhoff <steven.eckhoff.opensource@xxxxxxxxx>
> +Description:	Sets the equalizer biquad coefficient

Don't add sysfs files for configuring DSP coefficients, use binary
controls like other drivers do so that all the audio configuration can
be done via one interface.

> +What:		/sys/bus/i2c/devices/.../controls/limit_en
> +Date:		November 2017
> +KernelVersion:	4.14
> +Contact:	Steven Eckhoff <steven.eckhoff.opensource@xxxxxxxxx>
> +Description:	Enables the limiter

This isn't even a DSP coefficeint, this is just a standard on/off enable
control.  Why are you adding all this stuff that makes no attempt to use
standard interfaces?

> index c367d11079bc..34e911c1e082 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -217,6 +217,7 @@ config SND_SOC_ALL_CODECS
>  	select SND_SOC_WM9705 if SND_SOC_AC97_BUS
>  	select SND_SOC_WM9712 if SND_SOC_AC97_BUS
>  	select SND_SOC_WM9713 if SND_SOC_AC97_BUS
> +	select SND_SOC_TSCS42XX if I2C
>          help

Keep these lists sorted please.

> +			/* Write LSB first due to auto increment on MSB */
> +			ret = snd_soc_write(codec, R_DACCRWRL + j,
> +					fw->data[i + NUM_DACCR_BYTES - 1 - j]);
> +			if (ret < 0) {

Can't the device handle writing all three bytes at once (the regmap said
it could)?  If so that'd speed things up a lot.

> +	dev_info(codec->dev, "Loaded tscs42xx_daccram.dfw\n");

dev_dbg() at most.

> +#define NUM_CONTROL_BYTES 2
> +static int load_control_regs(struct snd_soc_codec *codec)
> +{
> +	const struct firmware *fw = NULL;
> +	int ret;
> +	int i;
> +
> +	ret = request_firmware_direct(&fw, "tscs42xx_controls.dfw", codec->dev);
> +	if (ret) {
> +		dev_info(codec->dev,
> +			"No tscs42xx_controls.dfw file found (%d)\n", ret);
> +		return 0;
> +	}

What exactly are these controls?

> +	for (count = 0; count < PLL_LOCK_TIME_MAX; count++) {
> +		reg = snd_soc_read(codec, R_PLLCTL0);
> +		if (reg < 0) {
> +			dev_err(codec->dev,
> +				"Failed to read PLL lock status (%d)\n", ret);
> +			break;
> +		} else if (reg > 0) {
> +			ret = true;
> +			break;
> +		}
> +		mdelay(1);

A mdelay() is going to spin the CPU, is that really required or would an
msleep() be fine?

> +static int tscs42xx_hw_params(struct snd_pcm_substream *substream,
> +		struct snd_pcm_hw_params *params,
> +		struct snd_soc_dai *codec_dai)
> +{
> +	struct snd_soc_codec *codec = codec_dai->codec;
> +	struct tscs42xx_priv *tscs42xx = snd_soc_codec_get_drvdata(codec);
> +	int ret;
> +
> +	mutex_lock(&tscs42xx->lock);

Why is there locking here?  In general what is this lock for?

> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		ret = -EINVAL;
> +		dev_err(codec->dev, "tscs42xx slave mode not supported (%d)\n",
> +			ret);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		dev_err(codec->dev, "Unsupported format (%d)\n", ret);
> +		break;
> +	}

Just remove the _CBS_CFS case, it's redundant.

> +	switch (ratio) {
> +	case 32:
> +	value = RV_DACSR_DBCM_32;
> +		break;
> +	case 40:

Please follow the kernel coding style.

> +static int tscs42xx_i2c_read(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte(i2c, reg);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "I2C write failed (%d)\n", ret);
> +		return ret;
> +	}

Why not just use the regmap?

> +	ret = of_property_read_string(np, "mclk-src", &mclk_src);
> +	if (ret) {
> +		dev_err(&i2c->dev, "mclk-src is needed (%d)\n", ret);
> +		return ret;
> +	}

If this needs to be in the DT rather than determined by the machine
driver the mux should be a standard clock driver using the clock
bindings.

> +	if (!strncmp(mclk_src, "mclk", 4)) {
> +		tscs42xx->mclk = devm_clk_get(&i2c->dev, NULL);
> +		if (IS_ERR(tscs42xx->mclk)) {
> +			dev_info(&i2c->dev, "mclk not present trying again\n");
> +			return -EPROBE_DEFER;
> +		}

This is broken, it's just ignoring the error code and deferring even if
there is a definitive error from the clock API.

> +		tscs42xx->pll_src_clk = PLL_SRC_CLK_MCLK2;

What happened to MCLK1?

> +static int __init tscs42xx_modinit(void)
> +{
> +	int ret = 0;

module_i2c_driver().

Attachment: signature.asc
Description: PGP signature