[PATCH] ALSA: AT73C213: Rectify misleading comments.

[PATCH] ALSA: AT73C213: Rectify misleading comments.

[Peter Rosin]

The Atmel SSC can divide by even numbers, not only powers of two and
the given values are binary, not hexadecimal.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 sound/spi/at73c213.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

I'm working on a driver based on AT73C213 and have had to look up
if the below nits were code problems or comment problems.  The Atmel
specs tells me that the comments are wrong and this patch fixes
those comments.

Cheers and thanks,
Peter

diff --git a/sound/spi/at73c213.c b/sound/spi/at73c213.c
index 1bc56b2..548e17a 100644
--- a/sound/spi/at73c213.c
+++ b/sound/spi/at73c213.c
@@ -155,7 +155,7 @@ static int snd_at73c213_set_bitrate(struct snd_at73c213 *chip)
 	if (max_tries < 1)
 		max_tries = 1;
 
-	/* ssc_div must be a power of 2. */
+	/* ssc_div must be even. */
 	ssc_div = (ssc_div + 1) & ~1UL;
 
 	if ((ssc_rate / (ssc_div * 2 * 16)) < BITRATE_MIN) {
@@ -621,7 +621,7 @@ static int snd_at73c213_line_capture_volume_info(
 {
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
 	uinfo->count = 2;
-	/* When inverted will give values 0x10001 => 0. */
+	/* When inverted will give values 10001b => 0. */
 	uinfo->value.integer.min = 14;
 	uinfo->value.integer.max = 31;
 
@@ -634,7 +634,7 @@ static int snd_at73c213_aux_capture_volume_info(
 {
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
 	uinfo->count = 1;
-	/* When inverted will give values 0x10001 => 0. */
+	/* When inverted will give values 10001b => 0. */
 	uinfo->value.integer.min = 14;
 	uinfo->value.integer.max = 31;
 
-- 
1.7.2.3

Re: [PATCH] ALSA: AT73C213: Rectify misleading comments.

[Hans-Christian Egtvedt]

On Mon, 2010-11-15 at 12:29 +0100, Peter Rosin wrote: 
> The Atmel SSC can divide by even numbers, not only powers of two and
> the given values are binary, not hexadecimal.

IIRC the divide by power of two is more related to the generic clocks in
the device thatn the SSC module itself.

> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  sound/spi/at73c213.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> I'm working on a driver based on AT73C213 and have had to look up
> if the below nits were code problems or comment problems.  The Atmel
> specs tells me that the comments are wrong and this patch fixes
> those comments.
> 
> Cheers and thanks,
> Peter
> 
> diff --git a/sound/spi/at73c213.c b/sound/spi/at73c213.c
> index 1bc56b2..548e17a 100644
> --- a/sound/spi/at73c213.c
> +++ b/sound/spi/at73c213.c
> @@ -155,7 +155,7 @@ static int snd_at73c213_set_bitrate(struct snd_at73c213 *chip)
>  	if (max_tries < 1)
>  		max_tries = 1;
>  
> -	/* ssc_div must be a power of 2. */
> +	/* ssc_div must be even. */

IIRC the bitrate is controlled by a generic clock, and it has a power of
two divider possibility. Hence the comment about power of two.

<snipp other two changes that looked fine>

-- 
Hans-Christian Egtvedt

Re: [PATCH] ALSA: AT73C213: Rectify misleading comments.

[Peter Rosin]

Den 2010-11-15 15:50 skrev Hans-Christian Egtvedt:
> On Mon, 2010-11-15 at 12:29 +0100, Peter Rosin wrote: 
>> The Atmel SSC can divide by even numbers, not only powers of two and
>> the given values are binary, not hexadecimal.
> 
> IIRC the divide by power of two is more related to the generic clocks in
> the device thatn the SSC module itself.
> 
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  sound/spi/at73c213.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> I'm working on a driver based on AT73C213 and have had to look up
>> if the below nits were code problems or comment problems.  The Atmel
>> specs tells me that the comments are wrong and this patch fixes
>> those comments.
>>
>> Cheers and thanks,
>> Peter
>>
>> diff --git a/sound/spi/at73c213.c b/sound/spi/at73c213.c
>> index 1bc56b2..548e17a 100644
>> --- a/sound/spi/at73c213.c
>> +++ b/sound/spi/at73c213.c
>> @@ -155,7 +155,7 @@ static int snd_at73c213_set_bitrate(struct snd_at73c213 *chip)
>>  	if (max_tries < 1)
>>  		max_tries = 1;
>>  
>> -	/* ssc_div must be a power of 2. */
>> +	/* ssc_div must be even. */
> 
> IIRC the bitrate is controlled by a generic clock, and it has a power of
> two divider possibility. Hence the comment about power of two.

The way I read it is that the driver looks for a divider for the SSC that
is compatible with a power-of-two divider for chip->board->dac_clk.
But the divider for the SSC need not be a power of two, it just needs to
be even.  On the AT91SAM9261-EK board the SSC clock is inherited from
the same clock as dac_clk, and in that case the SSC clock must have a
power of two divider as it has to be 8x the dac_clk which in turn has
to be a power of two divider.  This is apparent from this snippet

		/* 256 / (2 * 16) = 8 */
		dac_rate_new = 8 * (ssc_rate / ssc_div);

in the do-while loop.  So in that case it ssc_div must indeed be a power
of two.  It is not unlikely that ssc_div is always a power of two for
all real boards featuring an at73c213.

However, from the driver point of view, there is nothing that requires
the SSC clock and the dac_clk to be based on the same clock, and if they
are unrelated it is entirely possible that ssc_div does not end up a
power of two.  The driver code is prepped for this case and happily runs
along with any even ssc_div that fits the bill.

My point is that the "ssc_div must be a power of 2" comment is
misleading in that this is an external (to the driver) constraint that
has nothing to do with the driver code.  This constraint is also not
universally true, and the comment just makes it harder to follow what
the driver code is doing.

Heck, the comment is right before a statement that makes ssc_div even
using some bit manipulations.  IMHO, the comment *should* describe what
the bit manipulations are doing, and not some external constraint.

The current comment just makes you wonder if the code or the comment is
wrong.

Cheers,
Peter

Re: [PATCH] ALSA: AT73C213: Rectify misleading comments.

[Mark Brown]

On Mon, Nov 15, 2010 at 07:52:31PM +0100, Peter Rosin wrote:
> Den 2010-11-15 15:50 skrev Hans-Christian Egtvedt:

> > IIRC the bitrate is controlled by a generic clock, and it has a power of
> > two divider possibility. Hence the comment about power of two.

> My point is that the "ssc_div must be a power of 2" comment is
> misleading in that this is an external (to the driver) constraint that
> has nothing to do with the driver code.  This constraint is also not
> universally true, and the comment just makes it harder to follow what
> the driver code is doing.

This code probably ought to be converted to ASoC anyway :)