[U-Boot] [PATCH v3 5/5] New board support: Nokia RX-51 aka N900

[Marek Vasut <marex@denx.de>]

Dear Pali Roh?r,

[...]

> > > Reason? ++i and i++ are same (for this situation).
> > 
> > On arm, yes, on intel, no in certain cases. Also, to keep the
> > code consistent, go with postdecrement.
> 
> What is different on intel (without assigning value of i)?

IIRC the prefetch behaves differently. Ccing Graeme, but this might be a 
question to the GCC guys.

> > > > > +	/* append omap atag only if env setup_omap_atag is set
> > > > > to
> > > 
> > > 1
> > > 
> > > > > */ +	str = getenv("setup_omap_atag");
> > > > > +	if (!str || strcmp(str, "1") != 0)
> > > > 
> > > > str[0] == '1' ? But still, you only want to check if it's
> > > > defined, no?
> > > 
> > > Hm, I'm checking if setup_omap_atag is 1. But is it problem?
> > 
> > Then why not str[0] == '1' ?
> 
> Ok.
> 
> > > > > +/*
> > > > > + * Routine: twl4030_regulator_set_mode
> > > > > + * Description: Set twl4030 regulator mode over i2c
> > > > > powerbus.
> > > > > + */
> > > > > +static void twl4030_regulator_set_mode(u8 id, u8 mode)
> > > > > +{
> > > > > +	u16 msg = MSG_SINGULAR(DEV_GRP_P1, id, mode);
> > > > > +	twl4030_i2c_write_u8(TWL4030_CHIP_PM_MASTER, msg >> 8,
> > > > > +			TWL4030_PM_MASTER_PB_WORD_MSB);
> > > > > +	twl4030_i2c_write_u8(TWL4030_CHIP_PM_MASTER, msg &
> > > > > 0xff,
> > > > > +			TWL4030_PM_MASTER_PB_WORD_LSB);
> > > > 
> > > > Uh, is this somehow special that you can't do longer
> > > > transfer?
> > > 
> > > I do not know how, because registers are different (MSB and
> > > LSB).
> > 
> > Investigate ;-)
> 
> I think it is not possible. Registers are in bad order. (So on
> some high endian system it could be possible)

High endian :-)

> Also in linux twl regulator driver is used two calls see:
> https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=driv
> ers/regulator/twl- regulator.c;hb=HEAD#l327

ok

> > > > > +
> > > > > +static unsigned long int twl_wd_time; /* last time of
> > > > > watchdog reset */ +static unsigned long int twl_i2c_lock;
> > > > 
> > > > Are you sure you want to use global vars for these? These
> > > > won't work before reloc!
> > > 
> > > Why it does not work before reloc? U-Boot is on n900 always
> > > started from RAM.
> > 
> > The BSS isn't cleared though.
> 
> So how to fix it? I need to share these vars in more functions.

Make a struct and pass it around? Or if it's used only _after_ relocation, these 
will work.

> > > > > +
> > > > > +static u8 keys[8];
> > > > > +static u8 old_keys[8] = {0, 0, 0, 0, 0, 0, 0, 0};
> > > > > +#define KEYBUF_SIZE 32
> > > > > +static u8 keybuf[KEYBUF_SIZE];
> > > > > +static u8 keybuf_head;
> > > > > +static u8 keybuf_tail;
> > > > 
> > > > How much of this can be made const ?
> > > 
> > > Nothing. All is for keyboard and is changed by keyboard
> > > functions (tstc, getc)
> > > 
> > > > This magic needs at least _some_ documentation.
> > > > 
> > > > > +	if (!(mods & 2) && (k == 18 || k == 31 || k == 33 || k
> > > > > ==
> > > > > 34)) { +		/* cursor keys, without fn */
> > > > > +		keybuf[keybuf_tail++] = '\e';
> > > 
> > > there is "cursor keys without fn" (fn is "meta" key on n900).
> > > So this condition check if some cursor key (without fn) was
> > > pressed.
> > 
> > Good, document it.
> 
> Ok.