On Tue, Jun 22, 2010 at 09:43:14AM +1000, Stuart Longland wrote:
On Sat, Jun 19, 2010 at 02:12:21AM +0100, Mark Brown wrote:
On Sat, Jun 19, 2010 at 08:24:36AM +1000, Stuart Longland wrote:
- /* Page 1 */
- if (page == 1) {
if (reg <= 4)return 1;
I can't help but think that this'd be more legible with switch () statements (GCC has an extension for ranges in switch statements which you could use).
One is to go on a page-by-page basis, which is how I do it using the if statements. Here; I define my ranges so that I start from the very end... anything beyond page 70 is invalid ... voila, I eliminate those early on. A number of pages have a similar register pattern, and so I make use of nested if statements to explain this. The if block for pages following always use the block before to define the upper, non-inclusive bound.
It's not so much the outer ifs that were bothering me, it's the inner ones where you're doing the final register ranges as just a sequence of if statements (not even if ... else) which really bothered me here. The code just doesn't look like what it's trying to do.
This is a function largely intended for debugging, in fact, I'm thinking I should probably wrap it in #ifdef CONFIG_DEBUG_FS, since the function isn't called unless debugfs is enabled. So I'm not certain that performance is worth chasing here given the intended purpose -- it's not something that's called all the time, nor something that will be used in a production environment.
Oh, I thought you were using it to filter the CODEC register displays?
That's my thoughts on the issue, perhaps naïve, but I'm not sure there's any real gain in refactoring this.
It's fairly hard to read at the minute -