[U-Boot] [PATCH 04/11 v2] drivers/net/vsc9953: Refractor the parser for VSC9953 commands

[Codrin Constantin Ciubotariu]

Hi Joe,

> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
> Sent: Friday, June 26, 2015 1:31 AM
> To: Ciubotariu Codrin Constantin-B43658
> Cc: u-boot; Joe Hershberger; Sun York-R58495
> Subject: Re: [U-Boot] [PATCH 04/11 v2] drivers/net/vsc9953: Refractor the parser
> for VSC9953 commands
> 
> >  static struct vsc9953_info vsc9953_l2sw = { @@ -575,6 +576,10 @@ void
> > vsc9953_init(bd_t *bis)  }
> >
> >  #ifdef CONFIG_VSC9953_CMD
> 
> I'd like to see this moved to its own file in common... maybe
> "common/cmd_ethsw.c". I'd also like to see this #define change to something like
> "CONFIG_CMD_ETHSW".
> 
> These changes don't necessarily need to be part of this series, since it already
> got in as is, but if you feel motivated, I would recommend you add a patch
> before this one that moves it.

I could move this parser in common/do_ethsw.c and rename the define. I guess this would imply that upcoming drivers for Ethernet L2 Switches could use the same commands while calling their specific functions.

> > -/* function to interpret commands starting with "ethsw " */ -static
> > int do_ethsw(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > argv[])
> > +struct command_def {
> > +       int cmd_to_keywords[VSC9953_MAX_CMD_PARAMS];
> > +       int cmd_keywords_nr;
> > +       int port;
> > +       int err;
> 
> Remove this. Just use a return value.

Ok, I will remove "err" and all the other references to it.

> > +#define VSC9953_PORT_CONF_HELP "[port <port_no>] { enable | disable |
> > +show } " \
> > +"- enable/disable a port; show shows a port's configuration"
> 
> Probably better to define this down by the use.

Ok.

> >                 return -1;
> 
> Please use "return CMD_RET_USAGE;" from include/command.h.

Ok, I wasn't aware of these macros. I will make use them in this patch set when it's appropriate.

> > +/* match optional keywords */
> > +static void cmd_keywords_opt_check(struct command_def *parsed_cmd,
> > +                                  int *argc_val) {
> > +       int                     i, keyw_opt_id, argc_val_max;
> 
> Use a single space. Put each var on a separate line.

Ok, I will make sure this applies throughout the whole patch set.

> 
> > +
> > +       /* remember the best match */
> > +       argc_val_max = *argc_val;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(cmd_opt_def); i++) {
> > +               keyw_opt_id = 0;
> > +               while (keyw_opt_id + *argc_val <
> > +                               parsed_cmd->cmd_keywords_nr &&
> > +                      cmd_opt_def[i].cmd_keyword[keyw_opt_id] != id_key_end
> &&
> > +                      parsed_cmd->cmd_to_keywords[keyw_opt_id + *argc_val] ==
> > +                      cmd_opt_def[i].cmd_keyword[keyw_opt_id])
> > +                       keyw_opt_id++;
> 
> It might help to break this up a bit and use some intermediate variables to make
> the code more readable.

Ok.

> 
> > +               if (keyw_opt_id && keyw_opt_id + *argc_val <=
> > +                                  parsed_cmd->cmd_keywords_nr &&
> > +                   cmd_opt_def[i].cmd_keyword[keyw_opt_id] == id_key_end &&
> > +                   (*argc_val + keyw_opt_id > argc_val_max))
> 
> This could benefit from a comment describing what you expect to be verified.

Ok.

> > +
> > +       for (i = 0; i < ARRAY_SIZE(cmd_def); i++) {
> > +               keyword_id = 0;
> > +               while (keyword_id + *argc_val < parsed_cmd->cmd_keywords_nr &&
> > +                      cmd_def[i].cmd_keyword[keyword_id] != id_key_end &&
> > +                      parsed_cmd->cmd_to_keywords[keyword_id + *argc_val] ==
> > +                                     cmd_def[i].cmd_keyword[keyword_id])
> > +                       keyword_id++;
> 
> It might help to break this up a bit and use some intermediate variables to make
> the code more readable.

Ok.

> 
> > +               if (keyword_id && keyword_id + *argc_val ==
> > +                                 parsed_cmd->cmd_keywords_nr &&
> > +                   cmd_def[i].cmd_keyword[keyword_id] == id_key_end)
> > + {
> 
> This could benefit from a comment describing what you expect to be verified.

Ok.

> > +/* find all the keywords in the command */ static void
> > +keywords_find(int argc, char * const argv[],
> 
> Make this function return an int.

Ok. I will also make the changes along this patch to check it’s return code.

> > +static void command_def_cleanup(struct command_def *parsed_cmd) {
> > +       /* Nothing to do for now */
> 
> Then why define it?

This function is populated later by another patch and it frees a dynamically allocated buffer. You suggested to use a static buffer instead, so I guess I will remove this functions since it's no longer needed.

> > +       if (!parsed_cmd.cmd_function) {
> > +               rc = -1;
> > +               goto __ret_cmd_cleanup;
> > +       }
> 
> I think this whole if statement is unneeded since this same test already exists
> in keywords_find().

Ok.


Thanks and best regards,
Codrin