Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18

["Randy.Dunlap" <rdunlap@xenotime.net>]

On Sat, 23 Sep 2006 02:18:36 +0200 Arnd Bergmann wrote:

> I've done a brief review. Overall, it looks fine, but you have lots of
> code duplication between your specific machines. It would be good
> to generalize that further.

Yes, I noticed that too.


> It would be good if you can get your code to build cleanly when using
> C=1 with sparse, right now that clearly won't work.

See Documentation/SubmitChecklist.

> Detailed comments follow
> 
> > +/*#define IRQ_DEBUG*/
> > +#undef IRQ_DEBUG
> > +
> > +#ifdef IRQ_DEBUG
> > +#define IRQ_DPRINTK(x...)	printk(x)
> > +#else
> > +#define IRQ_DPRINTK(x...)	do { } while (0)
> > +#endif
> 
> Use pr_debug here instead of making up your own macros

I saw 3 versions/variants of debug printk macros.  :(
(maybe I missed a few :)

[lots of other good comments snipped :]


> > +static char *cplb_print_entry(char *buf, int type)
> > +{
> > +	unsigned long *p_addr = dpdt_table;
> > +	unsigned long *p_data = dpdt_table + 1;
> > +	unsigned long *p_icount = dpdt_swapcount_table;
> > +	unsigned long *p_ocount = dpdt_swapcount_table + 1;
> > +	unsigned long *cplb_addr = (unsigned long *)DCPLB_ADDR0;
> > +	unsigned long *cplb_data = (unsigned long *)DCPLB_DATA0;
> > +	int entry, used_cplb = 0;
> > +
> > +	if (type == CPLB_I) {
> > +		buf += sprintf(buf, "Instrction CPLB entry:\n");
> > +		p_addr = ipdt_table;
> > +		p_data = ipdt_table + 1;
> > +		p_icount = ipdt_swapcount_table;
> > +		p_ocount = ipdt_swapcount_table + 1;
> > +		cplb_addr = (unsigned long *)ICPLB_ADDR0;
> > +		cplb_data = (unsigned long *)ICPLB_DATA0;
> > +	} else
> > +		buf += sprintf(buf, "Data CPLB entry:\n");
> > +
> > +	buf += sprintf(buf, "Address\t\tData\tSize\tValid\tLocked\tSwapin\
> > +\tiCount\toCount\n");
> > +
> > +	while (*p_addr != 0xffffffff) {
> > +		entry = cplb_find_entry(cplb_addr, cplb_data, *p_addr);
> > +		if (entry >= 0 && *p_data == cplb_data[entry])
> > +			used_cplb |= 1 << entry;
> > +
> > +		buf +=
> > +		    sprintf(buf,
> > +			    "0x%08lx\t0x%05lx\t%s\t%c\t%c\t%2d\t%ld\t%ld\n",
> > +			    *p_addr, *p_data,
> > +			    page_size_string_table[(*p_data & 0x30000) >> 16],
> > +			    (*p_data & CPLB_VALID) ? 'Y' : 'N',
> > +			    (*p_data & CPLB_LOCK) ? 'Y' : 'N', entry, *p_icount,
> > +			    *p_ocount);
> > +
> > +		p_addr += 2;
> > +		p_data += 2;
> > +		p_icount += 2;
> > +		p_ocount += 2;
> > +	}
> > +
> > +	if (used_cplb != 0xffff) {
> > +		buf += sprintf(buf, "Unused/mismatched CPLBs:\n");
> > +
> > +		for (entry = 0; entry < 16; entry++)
> > +			if (0 == ((1 << entry) & used_cplb)) {
> > +				int flags = cplb_data[entry];
> > +				buf +=
> > +				    sprintf(buf,
> > +					    "%2d: 0x%08lx\t0x%05x\t%s\t%c\t%c\n",
> > +					    entry, cplb_addr[entry], flags,
> > +					    page_size_string_table[(flags &
> > +								    0x30000) >>
> > +								   16],
> > +					    (flags & CPLB_VALID) ? 'Y' : 'N',
> > +					    (flags & CPLB_LOCK) ? 'Y' : 'N');
> > +			}
> > +	}
> > +
> > +	buf += sprintf(buf, "\n");
> > +
> > +	return buf;
> > +}
> 
> Another one of those files that just don't belong in procfs. Find some other
> interface for this.

examples of acceptable interfaces?

---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/