[U-Boot] [PATCH] sh: i2c: Add support I2C controller of SH7734

[Mike Frysinger <vapier@gentoo.org>]

On Wednesday 22 February 2012 08:53:50 Marek Vasut wrote:
> > On Tuesday 21 February 2012 20:13:47 Nobuhiro Iwamatsu wrote:
> > > --- /dev/null
> > > +++ b/drivers/i2c/sh_sh7734_i2c.c
> > > 
> > > +#if DEBUG
> > > +static void sh_i2c_dump_reg(struct sh_i2c *base)
> > > +{
> > > +	printf("iccr1 : %02X\n", readb(&base->iccr1));
> > > +	printf("iccr2 : %02X\n", readb(&base->iccr2));
> > > +	printf("icmr  : %02X\n", readb(&base->icmr));
> > > +	printf("icier : %02X\n", readb(&base->icier));
> > > +	printf("icsr  : %02X\n", readb(&base->icsr));
> > > +	printf("sar   : %02X\n", readb(&base->sar));
> > > +	printf("icdrt : %02X\n", readb(&base->icdrt));
> > > +	printf("icdrr : %02X\n", readb(&base->icdrr));
> > > +	printf("nf2cyc: %02X\n", readb(&base->nf2cyc));
> > > +}
> > > +#endif
> > 
> > if you used debug(), you wouldn't need the DEBUG check
> 
> But this puts lower amount of strain on the CPP. It doesn't have to check
> if DEBUG is defined 10 times, but only once ;-) Putting this into one big
> debug() call is also possible btw., though I don't like it.

assuming you're not joking, i'm fairly certain that's now how the CPP engine 
works.  we want to minimize ifdef's and let the compiler to code checking as 
much as possible and do DCE on unused stuff.

> > > +int i2c_set_bus_num(unsigned int bus)
> > > +{
> > > +	if ((bus < 0) || (bus >= CONFIG_SYS_MAX_I2C_BUS)) {
> > > +		printf("Bad bus: %d\n", bus);
> > > +		return -1;
> > > +	}
> > > +
> > > +	switch (bus) {
> > > +	case 0:
> > > +		base = (void *)CONFIG_SH_I2C_BASE0;
> > > +		break;
> > > +	case 1:
> > > +		base = (void *)CONFIG_SH_I2C_BASE1;
> > > +		break;
> > > +	default:
> > > +		return -1;
> > > +	}
> > 
> > do you need the if() check if you have default here ?
> 
> CONFIG_SYS_MAX_I2C_BUS may be different in some obscure case?

unlikely, but you're right.  the code could still be unified though:
	if (bus >= CONFIG_SYS_MAX_I2C_BUS)
		goto case_bad_bus;
	switch (bus) {
	case 0:
		...
	case 1:
		...
	case_bad_bus:
	default:
		printf("...warning...");
		return -1;
	}
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120222/c09d76af/attachment.pgp>