Hi Sjoerd, On 28 February 2015 at 07:12, Sjoerd Simons <[email protected]> wrote: > On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote: >> Hi Sjoerd, >> >> On 19 February 2015 at 15:41, Sjoerd Simons >> <[email protected]> wrote: >> > Properly map memory through map_sysmem so that pxe can be used from the >> > sandbox. >> > >> > Signed-off-by: Sjoerd Simons <[email protected]> >> >> Please run your patches through patman as you seem to have style >> violations. Also can you add some notes about how you have tested this >> on real hardware? > > Will do for the next round together with addressing your comments. One > confused me tough, see below. > >> > --- >> > common/cmd_pxe.c | 80 >> > ++++++++++++++++++++++++++++++++------------------------ >> > 1 file changed, 46 insertions(+), 34 deletions(-) > >> > @@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu >> > *cfg) >> > * >> > * Returns 1 on success, < 0 on error. >> > */ >> > -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu >> > *cfg, int nest_level) >> > +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b, >> > + struct pxe_menu *cfg, int nest_level) >> >> s/b/base/ or something more meaningful (fix above/below also) >> > { >> > struct token t; >> > - char *s, *b, *label_name; >> > + char *s, *label_name, *base; >> > int err; >> > >> > - b = p; >> > + base = p; >> >> This worries me - assigning a pointer to a ulong. > > > base is a pointer here though. So this comment confuses me.
Actually it confused me. I don't think it's good to use base as a pointer or a ulong in the same file. Maybe use base for the ulong and base_ptr for the pointer. Or base_addr and base. But code is harder to read if different functions in the file have different conventions. Regards, Simon _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
