lkml.org 
[lkml]   [2018]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2018-12-20 16:08    [W:0.041 / U:27.948 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site