Re: [alsa-devel] [PATCH 2/3] ASoC: xlnx: add pcm formatter platform driver

On Wed, Dec 19, 2018 at 10:49:08PM +0530, Maruthi Srinivas Bayyavarapu wrote:

This looks mostly good, a few small things but all pretty trivial:

• switch (bits_per_sample) {
• case 8:

• val |= (BIT_DEPTH_8 << AUD_CTRL_DATA_WIDTH_SHIFT);
  

• break;
  

• case 16:

• val |= (BIT_DEPTH_16 << AUD_CTRL_DATA_WIDTH_SHIFT);
  

• break;
  

• case 20:

• val |= (BIT_DEPTH_20 << AUD_CTRL_DATA_WIDTH_SHIFT);
  

• break;
  

• case 24:

• val |= (BIT_DEPTH_24 << AUD_CTRL_DATA_WIDTH_SHIFT);
  

• break;
  

• case 32:

• val |= (BIT_DEPTH_32 << AUD_CTRL_DATA_WIDTH_SHIFT);
  

• break;
  

• }

It'd be better to have a default case returning an error for unsupported sample sizes.

• ret = devm_request_irq(dev, aud_drv_data->mm2s_irq,
  

• 		       xlnx_mm2s_irq_handler, 0,
  

• 		       "xlnx_formatter_pcm_mm2s_irq", dev);
  

• if (ret) {
  

• 	dev_err(dev, "xlnx audio mm2s irq request failed\n");
  

• 	goto clk_err;
  

• }
  

• ret = xlnx_formatter_pcm_reset(aud_drv_data->mmio +
  

• 			       XLNX_MM2S_OFFSET);
  

• if (ret) {
  

• 	dev_err(dev, "audio formatter reset failed\n");
  

• 	goto clk_err;
  

• }
  

• xlnx_formatter_disable_irqs(aud_drv_data->mmio +
  

• 			    XLNX_MM2S_OFFSET,
  

• 			    SNDRV_PCM_STREAM_PLAYBACK);
  

This requests the interrupt before we've reset and quiesced the hardware meaning the interrupt could fire immediately on request with the hardware in an unknown state; it'd be better to reset the hardware first then request the interrupt to minimize the risk that something goes wrong with the handler.

• dev_info(dev, "pcm platform device registered\n");
• return 0;

This is just adding noise to the log.