Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1239578

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

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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 :/

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH v2 0/2] ASoC: add driver for Atmel I2S controller Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2015-09-29 16:10 +0200
  [PATCH v2 1/2] ASoC: atmel-i2s: add DT bindings for I2S controller Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2015-09-29 16:10 +0200
  [PATCH v2 2/2] ASoC: atmel-i2s: add driver for the new Atmel I2S controller Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2015-09-29 16:20 +0200
    Re: [PATCH v2 2/2] ASoC: atmel-i2s: add driver for the new Atmel I2S  controller Mark Brown <broonie@kernel.org> - 2015-10-05 16:00 +0200

csiph-web