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

On Tue, Sep 29, 2015 at 04:09:20PM +0200, Cyrille Pitchen wrote:

• if (pending & ATMEL_I2SC_INT_RXOR) {

• mask = ATMEL_I2SC_SR_RXOR;
  

• for (ch = 0; ch < ATMEL_I2SC_MAX_TDM_CHANNELS; ++ch)
  

• 	if (sr & ATMEL_I2SC_SR_RXORCH(ch)) {
  

• 		mask |= ATMEL_I2SC_SR_RXORCH(ch);
  

• 		dev_err(dev->dev,
  

• 			"RX overrun on channel %d\n", ch);
  

• 	}
  

• regmap_write(dev->regmap, ATMEL_I2SC_SCR, mask);
  

• }

Coding style - the for loop needs { } for legibility.

• if (pending & ATMEL_I2SC_INT_TXUR) {

• mask = ATMEL_I2SC_SR_TXUR;
  

• for (ch = 0; ch < ATMEL_I2SC_MAX_TDM_CHANNELS; ++ch)
  

• 	if (sr & ATMEL_I2SC_SR_TXURCH(ch)) {
  

• 		mask |= ATMEL_I2SC_SR_TXURCH(ch);
  

• 		dev_err(dev->dev,
  

• 			"TX underrun on channel %d\n", ch);
  

• 	}
  

• regmap_write(dev->regmap, ATMEL_I2SC_SCR, mask);
  

• }
• return IRQ_HANDLED;

This IRQ_HANDLED should be generated only if one of the interrupts we know about got handled - there was a check to see if any of the unmasked bits is set earlier on in the function but that's not quite the same check.

+static int atmel_i2s_prepare(struct snd_pcm_substream *substream,

• 	     struct snd_soc_dai *dai)
  

+{

• struct atmel_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
• bool is_playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
• unsigned int rhr, sr = 0;
• if (is_playback) {

• regmap_read(dev->regmap, ATMEL_I2SC_SR, &sr);
  

• if (sr & ATMEL_I2SC_SR_RXRDY) {
  

• 	dev_dbg(dev->dev, "RXRDY is set\n");
  

• 	regmap_read(dev->regmap, ATMEL_I2SC_RHR, &rhr);
  

• }
  

• }

What's this doing? It just seems to do two reads and issue a debug message...

+static int atmel_i2s_sama5d2_mck_init(struct atmel_i2s_dev *dev,

• 		      struct device_node *np)
  

+{

• #define SFR_I2SCLKSEL 0x90U
• struct regmap *sfr;
• int id;
• id = of_alias_get_id(np, "i2s");
• if (id < 0) {

• dev_err(dev->dev, "failed to get alias ID\n");
  

• return id;
  

• }
• if (id > 1) {

• dev_err(dev->dev, "invalid I2S controller ID: %d\n", id);
  

• return -EINVAL;
  

• }

This didn't appear in the DT binding and looks pretty funky - what's going on here?

• sfr = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
• if (IS_ERR(sfr)) {

• dev_err(dev->dev, "failed to get SFR syscon\n");
  

• return PTR_ERR(sfr);
  

• }

This didn't appear in the binding either.

• /* Get hardware capabilities. */
• match = of_match_node(atmel_i2s_dt_ids, np);
• dev->caps = match ? match->data : NULL;

Please don't abuse the ternery operator like this, just write a normal if statement :/