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

[Simon Ho <simon.ho.cnxt@gmail.com>]

On Wed, Apr 05, 2017 at 04:17:04PM +0100, Charles Keepax wrote:
> 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.
> 

Thank you for your useful comments, I will fix them.

> > +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