Re: [alsa-devel] [PATCH v3 2/2] ASoC: atmel-i2s: add driver for the new Atmel I2S controller

On Wed, Jul 13, 2016 at 12:25:39PM +0200, Cyrille Pitchen wrote:

A few small things but otherwise this looks good:

• switch (params_channels(params)) {
• case 1:

• mr |= is_playback ? ATMEL_I2SC_MR_TXMONO : ATMEL_I2SC_MR_RXMONO;
  

Please write normal if statements, it makes things easier to read.

• break;
  

• case 2:

• break;
  

• default:

• dev_err(dev->dev, "unsupported number of audio channels\n");
  

• break;
  

• }

Why don't we return an error if we don't support the requested number of channels?

• dev->gclk = devm_clk_get(&pdev->dev, "gclk");
• if (IS_ERR(dev->aclk) && IS_ERR(dev->gclk)) {

• /* Master Mode not supported */
  

• dev->aclk = NULL;
  

• dev->gclk = NULL;
  

This won't handle probe deferral properly, it'll just ignore the clocks if we get -EPROBE_DEFER. The driver should special case that at least.