Re: [PATCH 5/7] ASoC: imx-pcm-rpmsg: Add platform driver for audio base on rpmsg

On Fri, Feb 05, 2021 at 02:57:28PM +0800, Shengjiu Wang wrote:

• if (params_format(params) == SNDRV_PCM_FORMAT_S16_LE)

• msg->s_msg.param.format   = RPMSG_S16_LE;
  

• else if (params_format(params) == SNDRV_PCM_FORMAT_S24_LE)

Again this should be a switch statement.

• if (params_channels(params) == 1)

• msg->s_msg.param.channels = RPMSG_CH_LEFT;
  

• else

• msg->s_msg.param.channels = RPMSG_CH_STEREO;
  

Shouldn't this be reporting an error if the number of channels is more than 2?

• /*
  

•  * if the data in the buffer is less than one period
  

•  * send message immediately.
  

•  * if there is more than one period data, delay one
  

•  * period (timer) to send the message.
  

•  */
  

• if ((avail - writen_num * period_size) <= period_size) {
  

• 	imx_rpmsg_insert_workqueue(substream, msg, info);
  

• } else if (rpmsg->force_lpa && !timer_pending(timer)) {
  

• 	int time_msec;
  

• 	time_msec = (int)(runtime->period_size * 1000 / runtime->rate);
  

• 	mod_timer(timer, jiffies + msecs_to_jiffies(time_msec));
  

• }
  

The comment here is at least confusing - why would we not send a full buffer immediately if we have one? This sounds like it's the opposite way round to what we'd do if we were trying to cut down the number of messages. It might help to say which buffer and where?

• /**

• * Every work in the work queue, first we check if there
  

/** comments are only for kerneldoc.