Re: [PATCH v4 2/2] ASoC: Add support for Conexant CX2092X DSP

[Charles Keepax <ckeepax@opensource.wolfsonmicro.com>]

On Wed, Apr 05, 2017 at 06:27:13PM +0800, simon.ho.cnxt@gmail.com wrote:
> From: Simon Ho <simon.ho@conexant.com>
> 
> Initial commit of Conexant CX20921/CX20924 I2S Audio DSP driver
> 
> The CX2092X devices are designed for virtual assisant application need to
> be always open, listening for users to summon it. There is no any power
> saving mode support on this device. The processed voice data will be sent
> to automatic speech recognition (ASR) application for further processing.
> 
> Signed-off-by: Simon Ho <simon.ho@conexant.com>
> ---

Just a couple of very minor points.

> +static int cx2092x_sendcmd(struct snd_soc_codec *codec,
> +			   struct cx2092x_cmd *cmd)
> +{
> +	struct cx2092x_priv *cx2092x = snd_soc_codec_get_drvdata(codec);
> +	int ret = 0;
> +	int num_32b_words = cmd->num_32b_words;
> +	unsigned long time_out;
> +	u32 *i2c_data = (u32 *)cmd;
> +	int size = num_32b_words + 2;
> +
> +	/* calculate how many WORD that will be wrote to device*/
> +	cmd->num_32b_words = cmd->command_id & CX2092X_CMD_GET(0) ?
> +			     CX2092X_CMD_SIZE : num_32b_words;
> +
> +	/* write all command data except fo frist 4 bytes*/
> +	ret = regmap_bulk_write(cx2092x->regmap, 4, &i2c_data[1], size - 1);
> +	if (ret < 0) {
> +		dev_err(cx2092x->dev, "Failed to write command data\n");
> +		goto LEAVE;

All caps for a label is a bit unusual probably would be more
normal kernel style to make this lower case.

> +	}
> +
> +	/* write first 4 bytes command data*/
> +	ret = regmap_bulk_write(cx2092x->regmap, 0, i2c_data, 1);
> +	if (ret < 0) {
> +		dev_err(cx2092x->dev, "Failed to write command\n");
> +		goto LEAVE;
> +	}
> +
> +	/* continuously read the first bytes data from device until
> +	 * either timeout or the flag 'reply' is set.
> +	 */
> +	time_out = msecs_to_jiffies(2000);
> +	time_out += jiffies;
> +	do {
> +		regmap_bulk_read(cx2092x->regmap, 0, &i2c_data[0], 1);
> +		if (cmd->reply == 1)
> +			break;
> +		mdelay(10);

Do you really want to use mdelay's, they are busy wait loops
so thrash the processor, a usleep_range or an msleep might be a
more friendly.

Thanks,
Charles