public inbox for alsa-devel@alsa-project.org
 help / color / mirror / Atom feed
* [PATCH 3/5] ASoC: dwc: Iterate over all channels
@ 2014-12-03 16:39 Andrew Jackson
  2014-12-03 17:29 ` Mark Brown
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Jackson @ 2014-12-03 16:39 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai
  Cc: alsa-devel@alsa-project.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Rajeev Kumar, Liam Girdwood,
	Mark Brown, Liviu Dudau

On the Designware core, the channels are independent and not combined
in higher registers.  So as more channels are added, more registers need
to be updated.

Signed-off-by: Andrew Jackson <Andrew.Jackson@arm.com>
---
 sound/soc/dwc/designware_i2s.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
index f8946bd..c497ada 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s.c
@@ -227,19 +227,25 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
 
 	i2s_disable_channels(dev, substream->stream);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		i2s_write_reg(dev->i2s_base, TCR(ch_reg), xfer_resolution);
-		i2s_write_reg(dev->i2s_base, TFCR(ch_reg), 0x02);
-		irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
-		i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x30);
-		i2s_write_reg(dev->i2s_base, TER(ch_reg), 1);
-	} else {
-		i2s_write_reg(dev->i2s_base, RCR(ch_reg), xfer_resolution);
-		i2s_write_reg(dev->i2s_base, RFCR(ch_reg), 0x07);
-		irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
-		i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x03);
-		i2s_write_reg(dev->i2s_base, RER(ch_reg), 1);
-	}
+	/* Iterate over set of channels - independently controlled.
+	 */
+	do {
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			i2s_write_reg(dev->i2s_base, TCR(ch_reg),
+				      xfer_resolution);
+			i2s_write_reg(dev->i2s_base, TFCR(ch_reg), 0x02);
+			irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
+			i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x30);
+			i2s_write_reg(dev->i2s_base, TER(ch_reg), 1);
+		} else {
+			i2s_write_reg(dev->i2s_base, RCR(ch_reg),
+				      xfer_resolution);
+			i2s_write_reg(dev->i2s_base, RFCR(ch_reg), 0x07);
+			irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
+			i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x03);
+			i2s_write_reg(dev->i2s_base, RER(ch_reg), 1);
+		}
+	} while (ch_reg-- > 0);
 
 	i2s_write_reg(dev->i2s_base, CCR, ccr);
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/5] ASoC: dwc: Iterate over all channels
  2014-12-03 16:39 [PATCH 3/5] ASoC: dwc: Iterate over all channels Andrew Jackson
@ 2014-12-03 17:29 ` Mark Brown
  2014-12-04  6:55   ` rajeev kumar
  2014-12-04  9:03   ` Andrew Jackson
  0 siblings, 2 replies; 5+ messages in thread
From: Mark Brown @ 2014-12-03 17:29 UTC (permalink / raw)
  To: Andrew Jackson
  Cc: Jaroslav Kysela, Takashi Iwai, alsa-devel@alsa-project.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Rajeev Kumar, Liam Girdwood,
	Liviu Dudau

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

On Wed, Dec 03, 2014 at 04:39:01PM +0000, Andrew Jackson wrote:

> +	/* Iterate over set of channels - independently controlled.
> +	 */
> +	do {
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +			i2s_write_reg(dev->i2s_base, TCR(ch_reg),
> +				      xfer_resolution);
> +			i2s_write_reg(dev->i2s_base, TFCR(ch_reg), 0x02);
> +			irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
> +			i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x30);
> +			i2s_write_reg(dev->i2s_base, TER(ch_reg), 1);
> +		} else {
> +			i2s_write_reg(dev->i2s_base, RCR(ch_reg),
> +				      xfer_resolution);
> +			i2s_write_reg(dev->i2s_base, RFCR(ch_reg), 0x07);
> +			irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
> +			i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x03);
> +			i2s_write_reg(dev->i2s_base, RER(ch_reg), 1);
> +		}
> +	} while (ch_reg-- > 0);

The normal way to write an iteration would be with a for loop - why are
we not doing that?

Also I see that you've not sent these as a single thread - please use
--thread.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/5] ASoC: dwc: Iterate over all channels
  2014-12-03 17:29 ` Mark Brown
@ 2014-12-04  6:55   ` rajeev kumar
  2014-12-04  9:13     ` Andrew Jackson
  2014-12-04  9:03   ` Andrew Jackson
  1 sibling, 1 reply; 5+ messages in thread
From: rajeev kumar @ 2014-12-04  6:55 UTC (permalink / raw)
  To: Mark Brown
  Cc: Andrew Jackson, Jaroslav Kysela, Takashi Iwai,
	alsa-devel@alsa-project.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Liam Girdwood, Liviu Dudau

On Wed, Dec 3, 2014 at 10:59 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Dec 03, 2014 at 04:39:01PM +0000, Andrew Jackson wrote:
>
>> +     /* Iterate over set of channels - independently controlled.
>> +      */
>> +     do {
>> +             if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> +                     i2s_write_reg(dev->i2s_base, TCR(ch_reg),
>> +                                   xfer_resolution);
>> +                     i2s_write_reg(dev->i2s_base, TFCR(ch_reg), 0x02);
>> +                     irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
>> +                     i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x30);
>> +                     i2s_write_reg(dev->i2s_base, TER(ch_reg), 1);
>> +             } else {
>> +                     i2s_write_reg(dev->i2s_base, RCR(ch_reg),
>> +                                   xfer_resolution);
>> +                     i2s_write_reg(dev->i2s_base, RFCR(ch_reg), 0x07);
>> +                     irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
>> +                     i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x03);
>> +                     i2s_write_reg(dev->i2s_base, RER(ch_reg), 1);
>> +             }
>> +     } while (ch_reg-- > 0);
>
> The normal way to write an iteration would be with a for loop - why are
> we not doing that?
>
> Also I see that you've not sent these as a single thread - please use
> --thread.

designware i2s has individual register for channel support. It is like
TCR(0), TCR(1) and so on.. So a macro is defined to capture these
please check below #define

#define TCR(x)      (0x40 * x + 0x034)

and the same is true for capture also. So there is no need for
iteration. Only writing to the particular register will do the work.

~Rajeev

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/5] ASoC: dwc: Iterate over all channels
  2014-12-03 17:29 ` Mark Brown
  2014-12-04  6:55   ` rajeev kumar
@ 2014-12-04  9:03   ` Andrew Jackson
  1 sibling, 0 replies; 5+ messages in thread
From: Andrew Jackson @ 2014-12-04  9:03 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel@alsa-project.org, Liam Girdwood, Takashi Iwai,
	Liviu Dudau, linux-kernel@vger.kernel.org, Rajeev Kumar,
	linux-arm-kernel@lists.infradead.org

On 12/03/14 17:29, Mark Brown wrote:
> On Wed, Dec 03, 2014 at 04:39:01PM +0000, Andrew Jackson wrote:
> 
>> +	/* Iterate over set of channels - independently controlled.
>> +	 */
>> +	do {
>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> +			i2s_write_reg(dev->i2s_base, TCR(ch_reg),
>> +				      xfer_resolution);
>> +			i2s_write_reg(dev->i2s_base, TFCR(ch_reg), 0x02);
>> +			irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
>> +			i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x30);
>> +			i2s_write_reg(dev->i2s_base, TER(ch_reg), 1);
>> +		} else {
>> +			i2s_write_reg(dev->i2s_base, RCR(ch_reg),
>> +				      xfer_resolution);
>> +			i2s_write_reg(dev->i2s_base, RFCR(ch_reg), 0x07);
>> +			irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
>> +			i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x03);
>> +			i2s_write_reg(dev->i2s_base, RER(ch_reg), 1);
>> +		}
>> +	} while (ch_reg-- > 0);
> 
> The normal way to write an iteration would be with a for loop - why are
> we not doing that?

The intention was to minimise the changes, excluding whitespace, between this version and the original.  Also, it is a perfectly valid looping construct.  I'm happy to rework it into a for loop.

> Also I see that you've not sent these as a single thread - please use
> --thread.
> 

	Andrew

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/5] ASoC: dwc: Iterate over all channels
  2014-12-04  6:55   ` rajeev kumar
@ 2014-12-04  9:13     ` Andrew Jackson
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Jackson @ 2014-12-04  9:13 UTC (permalink / raw)
  To: rajeev kumar, Mark Brown
  Cc: alsa-devel@alsa-project.org, Takashi Iwai, Liviu Dudau,
	Liam Girdwood, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org

On 12/04/14 06:55, rajeev kumar wrote:
> On Wed, Dec 3, 2014 at 10:59 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Wed, Dec 03, 2014 at 04:39:01PM +0000, Andrew Jackson wrote:
>>
>>> +     /* Iterate over set of channels - independently controlled.
>>> +      */
>>> +     do {
>>> +             if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>>> +                     i2s_write_reg(dev->i2s_base, TCR(ch_reg),
>>> +                                   xfer_resolution);
>>> +                     i2s_write_reg(dev->i2s_base, TFCR(ch_reg), 0x02);
>>> +                     irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
>>> +                     i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x30);
>>> +                     i2s_write_reg(dev->i2s_base, TER(ch_reg), 1);
>>> +             } else {
>>> +                     i2s_write_reg(dev->i2s_base, RCR(ch_reg),
>>> +                                   xfer_resolution);
>>> +                     i2s_write_reg(dev->i2s_base, RFCR(ch_reg), 0x07);
>>> +                     irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
>>> +                     i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x03);
>>> +                     i2s_write_reg(dev->i2s_base, RER(ch_reg), 1);
>>> +             }
>>> +     } while (ch_reg-- > 0);
>>
>> The normal way to write an iteration would be with a for loop - why are
>> we not doing that?
>>
>> Also I see that you've not sent these as a single thread - please use
>> --thread.
> 
> designware i2s has individual register for channel support. It is like
> TCR(0), TCR(1) and so on.. So a macro is defined to capture these
> please check below #define
> 
> #define TCR(x)      (0x40 * x + 0x034)
> 
> and the same is true for capture also. So there is no need for
> iteration. Only writing to the particular register will do the work.

However params_channels returns the number of channels that the device needs to configure.  So, for example, when there are four channels two sets of channel registers need to be programmed.


	Andrew
> 
> ~Rajeev
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2014-12-04  9:13 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-03 16:39 [PATCH 3/5] ASoC: dwc: Iterate over all channels Andrew Jackson
2014-12-03 17:29 ` Mark Brown
2014-12-04  6:55   ` rajeev kumar
2014-12-04  9:13     ` Andrew Jackson
2014-12-04  9:03   ` Andrew Jackson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox