[alsa-devel] [PATCH] ALSA: AT73C213: Rectify misleading comments.
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;
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>
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
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 :)
participants (4)
-
Hans-Christian Egtvedt -
Mark Brown -
Peter Rosin -
Peter Rosin