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.
Attachment:
signature.asc
Description: PGP signature
