[PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Peter Ujfalusi]

In order to avoid confusing the applications with msbits bigger
than the selected sample size apply the msbits constraint only
to sample seze bigger than the requested msbits.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---

Hi Mark,

Converted to use while loop.
We run the loop as long as the sample size is bigger than the requested
msbits.
Most of the drivers require 24/32 configuration, so not point of looping
for smaller sample sizes.

Regards,
Peter

 sound/soc/soc-pcm.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 8bb1793..ef41a39 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -68,13 +68,14 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream,
  * like the DAC/ADC resolution to use but there isn't right now.
  */
 static int sample_sizes[] = {
-	8, 16, 24, 32,
+	32, 24, 16, 8,
 };
 
 static void soc_pcm_apply_msb(struct snd_pcm_substream *substream,
 			      struct snd_soc_dai *dai)
 {
-	int ret, i, bits;
+	int ret, bits;
+	int i = 0;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		bits = dai->driver->playback.sig_bits;
@@ -84,14 +85,14 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream,
 	if (!bits)
 		return;
 
-	for (i = 0; i < ARRAY_SIZE(sample_sizes); i++) {
-		ret = snd_pcm_hw_constraint_msbits(substream->runtime,
-						   0, sample_sizes[i],
-						   bits);
+	/* Apply constraint only for sample size bigger than requested msbits */
+	while (sample_sizes[i] > bits && i < ARRAY_SIZE(sample_sizes)) {
+		ret = snd_pcm_hw_constraint_msbits(substream->runtime, 0,
+						   sample_sizes[i], bits);
 		if (ret != 0)
-			dev_warn(dai->dev,
-				 "Failed to set MSB %d/%d: %d\n",
+			dev_warn(dai->dev, "Failed to set MSB %d/%d: %d\n",
 				 bits, sample_sizes[i], ret);
+		i++;
 	}
 }
 
-- 
1.7.8.3

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Mark Brown]

On Wed, Jan 18, 2012 at 04:05:14PM +0100, Peter Ujfalusi wrote:

> Converted to use while loop.
> We run the loop as long as the sample size is bigger than the requested
> msbits.

I'm not sure this is actually a legibility improvement, if anything it's
probably less clear than the original as now the setup of the loop is
spread even further around the function.

> Most of the drivers require 24/32 configuration, so not point of looping
> for smaller sample sizes.

Performance isn't really a concern in this path unless we do something
totally insane.  Thinking time on the part of the reader needs to be
considered too...

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Clemens Ladisch]

Peter Ujfalusi wrote:
> We run the loop as long as the sample size is bigger than the requested
> msbits.

> +	while (sample_sizes[i] > bits && i < ARRAY_SIZE(sample_sizes)) {

This will overrun sample_sizes[] if bits < 8.


Regards,
Clemens

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Peter Ujfalusi]

On 01/18/2012 04:35 PM, Clemens Ladisch wrote:
> Peter Ujfalusi wrote:
>> We run the loop as long as the sample size is bigger than the requested
>> msbits.
> 
>> +	while (sample_sizes[i] > bits && i < ARRAY_SIZE(sample_sizes)) {
> 
> This will overrun sample_sizes[] if bits < 8.

which is prevented by the: && i < ARRAY_SIZE(sample_sizes)

-- 
Péter

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Peter Ujfalusi]

On 01/18/2012 04:43 PM, Peter Ujfalusi wrote:
> On 01/18/2012 04:35 PM, Clemens Ladisch wrote:
>> Peter Ujfalusi wrote:
>>> We run the loop as long as the sample size is bigger than the requested
>>> msbits.
>>
>>> +	while (sample_sizes[i] > bits && i < ARRAY_SIZE(sample_sizes)) {
>>
>> This will overrun sample_sizes[] if bits < 8.
> 
> which is prevented by the: && i < ARRAY_SIZE(sample_sizes)

I take it back. True, we overrun. These need to be swapped.

-- 
Péter

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Peter Ujfalusi]

On 01/18/2012 04:29 PM, Mark Brown wrote:
> I'm not sure this is actually a legibility improvement, if anything it's
> probably less clear than the original as now the setup of the loop is
> spread even further around the function.

Would it make it clearer if I set i to 0 right before the while?

>> Most of the drivers require 24/32 configuration, so not point of looping
>> for smaller sample sizes.
> 
> Performance isn't really a concern in this path unless we do something
> totally insane.  Thinking time on the part of the reader needs to be
> considered too...

Sure it is not a concern. These small 'Performance isn't really a
concern in this path' at the end ads up that we need faster CPUs to have
the same perceived perfomrance.

-- 
Péter

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Mark Brown]

On Wed, Jan 18, 2012 at 05:43:28PM +0100, Peter Ujfalusi wrote:
> On 01/18/2012 04:29 PM, Mark Brown wrote:
> > I'm not sure this is actually a legibility improvement, if anything it's
> > probably less clear than the original as now the setup of the loop is
> > spread even further around the function.

> Would it make it clearer if I set i to 0 right before the while?

That'd help a bit.  Though I'd just go with a for loop, the while
clearly doesn't look any better - I was just suggesting it without
actually having tried writing it out.

> > Performance isn't really a concern in this path unless we do something
> > totally insane.  Thinking time on the part of the reader needs to be
> > considered too...

> Sure it is not a concern. These small 'Performance isn't really a
> concern in this path' at the end ads up that we need faster CPUs to have
> the same perceived perfomrance.

Yeah, but if that percieved performance is already instantaneous there's
no need to worry :)  My first thought would've been to just continue on
sample rates we don't like rather than trying to break out of an
iteration of 4 steps early.

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Peter Ujfalusi]

On 01/18/2012 06:46 PM, Mark Brown wrote:
> That'd help a bit.  Though I'd just go with a for loop, the while
> clearly doesn't look any better - I was just suggesting it without
> actually having tried writing it out.

Are you going to take the for version of patch, or should I resend it?
I can add a comment to explain that we bail out, and not checking the
sample sizes lower than the requested msbits, if it helps.

-- 
Péter

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Mark Brown]

On Thu, Jan 19, 2012 at 09:27:33AM +0100, Peter Ujfalusi wrote:

> Are you going to take the for version of patch, or should I resend it?
> I can add a comment to explain that we bail out, and not checking the
> sample sizes lower than the requested msbits, if it helps.

I'd just replace it with a continue on sizes we don't want, much
simpler.

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Peter Ujfalusi]

On 01/19/2012 11:48 AM, Mark Brown wrote:
> On Thu, Jan 19, 2012 at 09:27:33AM +0100, Peter Ujfalusi wrote:
> 
>> Are you going to take the for version of patch, or should I resend it?
>> I can add a comment to explain that we bail out, and not checking the
>> sample sizes lower than the requested msbits, if it helps.
> 
> I'd just replace it with a continue on sizes we don't want, much
> simpler.

If I can do this (bits == 24):
i = 0
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] <= bits)
 apply constraint
i++
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] <= bits)
 go out

--------------------------------------
I will not do this (bits == 24):
static int sample_sizes[] = {
	8, 16, 24, 32,
};

i = 0
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] > bits)
i++
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] > bits)
i++
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] > bits)
i++
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] > bits)
 apply constraint
i++
check (i < ARRAY_SIZE(sample_sizes))
 go out

--------------------------------------
nor this (bits == 24):
static int sample_sizes[] = {
	32, 24, 16, 8,
};

i = 0
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] <= bits)
 apply constraint
i++
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] <= bits)
i++
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] <= bits)
i++
check (i < ARRAY_SIZE(sample_sizes))
check (sample_sizes[i] <= bits)
i++
check (i < ARRAY_SIZE(sample_sizes))
 go out

-- 
Péter

Re: [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits

[Mark Brown]

On Thu, Jan 19, 2012 at 01:40:35PM +0100, Peter Ujfalusi wrote:
> On 01/19/2012 11:48 AM, Mark Brown wrote:

> > I'd just replace it with a continue on sizes we don't want, much
> > simpler.

> If I can do this (bits == 24):

This mail took me a little while to parse but I think you're trying to
say you insist on the microoptimisation.  Unless you can come up with a
microoptimisation which doesn't take effort to read please write
something simple.  The reason I wasn't happy with the original patch was
that what should be a trivial bit of code took far too long to check.
Maintainability is vastly more important than microoptimising a slow
path, having to read things again later was the whole reason I didn't do
any filtering in the first place.