Dear Pali Rohár,
[...]
>
> Nothing. This code must be in assembler because stack is not
> initialized. Also we must be sure that U-Boot will not overwrite
> attached kernel (with can be in U-Boot malloc/monitor area...)
Don't we have something that can reserve a memory area?
> > > +static char *boot_reason_ptr;
> > > +static char *hw_build_ptr;
> > > +static char *nolo_version_ptr;
> > > +static char *boot_mode_ptr;
> > > +
> > > +/*
> > > + * Routine: init_omap_tags
> > > + * Description: Initialize pointers to values in tag_omap
> > > + */
> > > +static void init_omap_tags(void)
> > > +{
> > > + char *component;
> > > + char *version;
> > > + int i = 0;
> > > + while (omap[i].hdr.tag) {
> > > + switch (omap[i].hdr.tag) {
> > > + case OMAP_TAG_BOOT_REASON:
> > > + boot_reason_ptr =
>
> omap[i].u.boot_reason.reason_str;
>
> > > + break;
> > > + case OMAP_TAG_VERSION_STR:
> > > + component = omap[i].u.version.component;
> > > + version = omap[i].u.version.version;
> > > + if (strcmp(component, "hw-build") == 0)
> > > + hw_build_ptr = version;
> > > + else if (strcmp(component, "nolo") == 0)
> > > + nolo_version_ptr = version;
> > > + else if (strcmp(component, "boot-mode") == 0)
> > > + boot_mode_ptr = version;
> > > + break;
> >
> > default: missing.
>
> Is really needed? (if yes, I can add default: break;)
It's a good practice.
> > > + }
> > > + ++i;
> >
> > i++;
>
> 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.
> > > + /* 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' ?
> > > +/*
> > > + * 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 ;-)
> > > + /* set env variable attkernaddr for relocated kernel */
> > > + sprintf(buf, "%#x", KERNEL_ADDRESS);
> > > + setenv("attkernaddr", buf);
> >
> > Uhhh ? This definitelly isn't right! What are you trying to
> > achieve here?
>
> I think it is right. I want to store address of kernel (in hex
> with leading 0x) to env attkernaddr. And %#x is doing it.
Ah, now that you mentioned that you don't want kernel to be rewritten, I see
the
point of this variable.
> > > +
> > > +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.
> > > +
> > > +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.
> > > +int rx51_kp_tstc(void)
> > > +{
> > > + u8 c, r, dk, i;
> > > + u8 intr;
> > > + u8 mods;
> > > +
> > > + /* localy lock twl4030 i2c bus */
> > > + if (test_and_set_bit(0, &twl_i2c_lock))
> > > + return 0;
> > > +
> > > + /* twl4030 remembers up to 2 events */
> > > + for (i = 0; i < 2; i++) {
> > > +
> > > + /* check interrupt register for events */
> > > + twl4030_i2c_read_u8(TWL4030_CHIP_KEYPAD, &intr,
> > > + TWL4030_KEYPAD_KEYP_ISR1+(2*i));
> > > +
> > > + if (intr&1) { /* got an event */
> >
> > I will let you think about how to optimize the indent depth
> > here ...
>
> Ok, I will optimize indentation. This code (keyboard support) was
> written by Alistair Buxton and I reused it with minimal changes.
That doesn't justify it ;-)
> > > +#define tostring(s) #s
> > > +#define stringify(s) tostring(s)
> >
> > We do have __stringify(), use that!
>
> Ok, in April (when I wrote this part of code) __stringify was not
> in U-Boot (I checked it).
Yep, time changed.
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
