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

In order to support multiple commands to configure the VSC9953
L2 Switch, the parser needs to be changed to be more flexible and
to support more complex commands. This patch adds a parser that
searches for defined keywords in the command and calls the proper
function when a match is found. Also, the parser allows for
optional keywords, such as "port", to apply the command on a port
or on all ports. The already defined commands are also changed a
bit to:
ethsw [port <port_no>] { enable | disable | show }

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com>
---
Changes for v2:
        - removed Change-id field;

 drivers/net/vsc9953.c | 381 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 310 insertions(+), 71 deletions(-)

Hi Codrin,

On Tue, Jun 23, 2015 at 11:48 AM, Codrin Ciubotariu
<codrin.ciubotariu@freescale.com> wrote:
> In order to support multiple commands to configure the VSC9953
> L2 Switch, the parser needs to be changed to be more flexible and
> to support more complex commands. This patch adds a parser that
> searches for defined keywords in the command and calls the proper
> function when a match is found. Also, the parser allows for
> optional keywords, such as "port", to apply the command on a port
> or on all ports. The already defined commands are also changed a
> bit to:
> ethsw [port <port_no>] { enable | disable | show }
>
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@freescale.com>
> ---
> Changes for v2:
>         - removed Change-id field;
>
>  drivers/net/vsc9953.c | 381 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 310 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/net/vsc9953.c b/drivers/net/vsc9953.c
> index 9dec683..4df751a 100644
> --- a/drivers/net/vsc9953.c
> +++ b/drivers/net/vsc9953.c
> @@ -10,6 +10,7 @@
>  #include <asm/fsl_serdes.h>
>  #include <fm_eth.h>
>  #include <fsl_memac.h>
> +#include <errno.h>
>  #include <vsc9953.h>
>
>  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.

> +
> +#define VSC9953_MAX_CMD_PARAMS 20
> +#define VSC9953_CMD_PORT_ALL   -1
> +
>  /* Enable/disable status of a VSC9953 port */
>  static void vsc9953_port_status_set(int port_no, u8 enabled)
>  {
> @@ -595,15 +600,6 @@ static void vsc9953_port_status_set(int port_no, u8 enabled)
>                              CONFIG_VSC9953_PORT_ENA);
>  }
>
> -/* Set all VSC9953 ports' status */
> -static void vsc9953_port_all_status_set(u8 enabled)
> -{
> -       int             i;
> -
> -       for (i = 0; i < VSC9953_MAX_PORTS; i++)
> -               vsc9953_port_status_set(i, enabled);
> -}
> -
>  /* Start autonegotiation for a VSC9953 PHY */
>  static void vsc9953_phy_autoneg(int port_no)
>  {
> @@ -615,15 +611,6 @@ static void vsc9953_phy_autoneg(int port_no)
>                 printf("Failed to start PHY for port %d\n", port_no);
>  }
>
> -/* Start autonegotiation for all VSC9953 PHYs */
> -static void vsc9953_phy_all_autoneg(void)
> -{
> -       int             i;
> -
> -       for (i = 0; i < VSC9953_MAX_PORTS; i++)
> -               vsc9953_phy_autoneg(i);
> -}
> -
>  /* Print a VSC9953 port's configuration */
>  static void vsc9953_port_config_show(int port_no)
>  {
> @@ -685,75 +672,327 @@ static void vsc9953_port_config_show(int port_no)
>         printf("%8s\n", duplex == DUPLEX_FULL ? "full" : "half");
>  }
>
> -/* Print VSC9953 ports' configuration */
> -static void vsc9953_port_all_config_show(void)
> -{
> -       int             i;
> +/* IDs used to track keywords in a command */
> +enum keyword_id {
> +       id_key_end = -1,
> +       id_help,
> +       id_show,
> +       id_port,
> +       id_enable,
> +       id_disable,
> +       id_count,       /* keep last */
> +};
>
> -       for (i = 0; i < VSC9953_MAX_PORTS; i++)
> -               vsc9953_port_config_show(i);
> -}
> +enum keyword_opt_id {
> +       id_port_no = id_count + 1,
> +       id_count_all,   /* keep last */
> +};
>
> -/* 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.

> +       int (*cmd_function)(struct command_def *parsed_cmd);
> +};
> +
> +#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.

> +
> +static int vsc9953_port_status_key_func(struct command_def *parsed_cmd)
>  {
> -       u8 enable;
> -       u32 port;
> +       int                     i;
> +       u8                      enabled;

Use a single space.

>
> -       if (argc < 4)
> +       /* Last keyword should tell us if we should enable/disable the port */
> +       if (parsed_cmd->cmd_to_keywords[parsed_cmd->cmd_keywords_nr - 1] ==
> +           id_enable)
> +               enabled = 1;
> +       else if (parsed_cmd->cmd_to_keywords[parsed_cmd->cmd_keywords_nr - 1] ==
> +                id_disable)
> +               enabled = 0;
> +       else {
> +               parsed_cmd->err = 1;

Remove this.

>                 return -1;

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

> +       }
>
> -       if (strcmp(argv[1], "port"))
> -               return -1;
> +       if (parsed_cmd->port != VSC9953_CMD_PORT_ALL) {
> +               vsc9953_port_status_set(parsed_cmd->port, enabled);
> +       } else {
> +               for (i = 0; i < VSC9953_MAX_PORTS; i++)
> +                       vsc9953_port_status_set(i, enabled);
> +       }
> +
> +       return 0;

Please use "return CMD_RET_SUCCESS;"

> +}
> +
> +static int vsc9953_port_config_key_func(struct command_def *parsed_cmd)
> +{
> +       int                     i;

Use a single space.

> +
> +       if (parsed_cmd->port != VSC9953_CMD_PORT_ALL) {
> +               vsc9953_phy_autoneg(parsed_cmd->port);
> +               printf("%8s %8s %8s %8s %8s\n",
> +                      "Port", "Status", "Link", "Speed",
> +                      "Duplex");
> +               vsc9953_port_config_show(parsed_cmd->port);
>
> -       if (!strcmp(argv[3], "show")) {
> -               if (!strcmp(argv[2], "all")) {
> -                       vsc9953_phy_all_autoneg();
> -                       printf("%8s %8s %8s %8s %8s\n",
> -                              "Port", "Status", "Link", "Speed",
> -                              "Duplex");
> -                       vsc9953_port_all_config_show();
> -                       return 0;
> -               } else {
> -                       port = simple_strtoul(argv[2], NULL, 10);
> -                       if (!VSC9953_PORT_CHECK(port))
> -                               return -1;
> -                       vsc9953_phy_autoneg(port);
> -                       printf("%8s %8s %8s %8s %8s\n",
> -                              "Port", "Status", "Link", "Speed",
> -                              "Duplex");
> -                       vsc9953_port_config_show(port);
> -                       return 0;
> -               }
> -       } else if (!strcmp(argv[3], "enable")) {
> -               enable = 1;
> -       } else if (!strcmp(argv[3], "disable")) {
> -               enable = 0;
>         } else {
> -               return -1;
> +               for (i = 0; i < VSC9953_MAX_PORTS; i++)
> +                       vsc9953_phy_autoneg(i);
> +               printf("%8s %8s %8s %8s %8s\n",
> +                      "Port", "Status", "Link", "Speed", "Duplex");
> +               for (i = 0; i < VSC9953_MAX_PORTS; i++)
> +                       vsc9953_port_config_show(i);
>         }
>
> -       if (!strcmp(argv[2], "all")) {
> -               vsc9953_port_all_status_set(enable);
> +       return 0;

Please use "return CMD_RET_SUCCESS;"

> +}
> +
> +struct keywords_to_function {
> +       enum keyword_id cmd_keyword[VSC9953_MAX_CMD_PARAMS];
> +       int (*keyword_function)(struct command_def *parsed_cmd);
> +} cmd_def[] = {
> +               {
> +                       .cmd_keyword = {
> +                                       id_enable,
> +                                       id_key_end,
> +                       },
> +                       .keyword_function = &vsc9953_port_status_key_func,
> +               }, {
> +                       .cmd_keyword = {
> +                                       id_disable,
> +                                       id_key_end,
> +                       },
> +                       .keyword_function = &vsc9953_port_status_key_func,
> +               }, {
> +                       .cmd_keyword = {
> +                                       id_show,
> +                                       id_key_end,
> +                       },
> +                       .keyword_function = &vsc9953_port_config_key_func,
> +               },
> +};
> +
> +struct keywords_optional {
> +       enum keyword_id cmd_keyword[VSC9953_MAX_CMD_PARAMS];
> +} cmd_opt_def[] = {
> +               {
> +                               .cmd_keyword = {
> +                                               id_port,
> +                                               id_port_no,
> +                                               id_key_end,
> +                               },
> +               },
> +};
> +
> +static int keyword_match_gen(enum keyword_id key_id, int argc,
> +                            char *const argv[], int *argc_nr,
> +                            struct command_def *parsed_cmd);
> +static int keyword_match_port(enum keyword_id key_id, int argc,
> +                             char *const argv[], int *argc_nr,
> +                             struct command_def *parsed_cmd);
> +
> +/* Define properties for each keyword;
> + * keep the order synced with enum keyword_id
> + */
> +struct keyword_def {
> +       const char *keyword_name;
> +       int (*match)(enum keyword_id key_id, int argc, char *const argv[],
> +                    int *argc_nr, struct command_def *parsed_cmd);
> +} keyword[] = {
> +               {
> +                               .keyword_name = "help",
> +                               .match = &keyword_match_gen,
> +               }, {
> +                               .keyword_name = "show",
> +                               .match = &keyword_match_gen,
> +               }, {
> +                               .keyword_name = "port",
> +                               .match = &keyword_match_port
> +               },  {
> +                               .keyword_name = "enable",
> +                               .match = &keyword_match_gen,
> +               }, {
> +                               .keyword_name = "disable",
> +                               .match = &keyword_match_gen,
> +               },
> +};
> +
> +/* Generic function used to match a keyword only by a string */
> +static int keyword_match_gen(enum keyword_id key_id, int argc,
> +                            char *const argv[], int *argc_nr,
> +                            struct command_def *parsed_cmd)
> +{
> +       if (strcmp(argv[*argc_nr], keyword[key_id].keyword_name) == 0) {
> +               parsed_cmd->cmd_to_keywords[*argc_nr] = key_id;
> +
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +/* Function used to match the command's port */
> +static int keyword_match_port(enum keyword_id key_id, int argc,
> +                             char *const argv[], int *argc_nr,
> +                             struct command_def *parsed_cmd)
> +{
> +       unsigned long                   val;

Use a single space.

> +
> +       if (!keyword_match_gen(key_id, argc, argv, argc_nr, parsed_cmd))
>                 return 0;
> -       } else {
> -               port = simple_strtoul(argv[2], NULL, 10);
> -               if (!VSC9953_PORT_CHECK(port))
> -                       return -1;
> -               vsc9953_port_status_set(port, enable);
> +
> +       if (*argc_nr + 1 >= argc)
>                 return 0;
> +
> +       if (strict_strtoul(argv[*argc_nr + 1], 10, &val) != -EINVAL) {
> +               if (!VSC9953_PORT_CHECK(val)) {
> +                       printf("Invalid port number: %lu\n", val);
> +                       return 0;
> +               }
> +               parsed_cmd->port = val;
> +               (*argc_nr)++;
> +               parsed_cmd->cmd_to_keywords[*argc_nr] = id_port_no;
> +               return 1;
> +       }
> +
> +       return 0;
> +}
> +
> +/* 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.

> +
> +       /* 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.

> +               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.

> +                               argc_val_max = *argc_val + keyw_opt_id;
>         }
>
> -       return -1;
> +       *argc_val = argc_val_max;
>  }
>
> -U_BOOT_CMD(ethsw, 5, 0, do_ethsw,
> +/* get the function to call based on keywords */
> +static void cmd_keywords_check(struct command_def *parsed_cmd, int *argc_val)
> +{
> +       int                     i, keyword_id;

Use a single space. Put each var on a separate line.

> +
> +       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.

> +               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.

> +                       *argc_val += keyword_id;
> +                       parsed_cmd->cmd_function = cmd_def[i].keyword_function;
> +                       return;
> +               }
> +       }
> +}
> +
> +/* find all the keywords in the command */
> +static void keywords_find(int argc, char * const argv[],

Make this function return an int.

> +                         struct command_def *parsed_cmd)
> +{
> +       int                     i, j, argc_val;

Use a single space. Put each var on a separate line.

> +
> +       for (i = 1; i < argc; i++) {
> +               for (j = 0; j < id_count; j++) {
> +                       if (keyword[j].match(j, argc, argv, &i,
> +                                            parsed_cmd))
> +                               break;
> +               }
> +       }
> +
> +       for (i = 1; i < argc; i++)
> +               if (parsed_cmd->cmd_to_keywords[i] == -1)
> +                       parsed_cmd->err = 1;

Instead return CMD_RET_USAGE;

> +
> +       parsed_cmd->cmd_keywords_nr = argc;
> +       argc_val = 1;
> +
> +       /* get optional parameters first */
> +       cmd_keywords_opt_check(parsed_cmd, &argc_val);
> +
> +       cmd_keywords_check(parsed_cmd, &argc_val);
> +
> +       /* error if not all commands' parameters were matched */
> +       if (argc_val != parsed_cmd->cmd_keywords_nr ||
> +           !parsed_cmd->cmd_function)
> +               parsed_cmd->err = 1;

Instead return CMD_RET_USAGE;

> +}
> +
> +static void command_def_init(struct command_def *parsed_cmd)
> +{
> +       int                     i;
> +
> +       for (i = 0; i < VSC9953_MAX_CMD_PARAMS; i++)
> +               parsed_cmd->cmd_to_keywords[i] = -1;
> +
> +       parsed_cmd->port = VSC9953_CMD_PORT_ALL;
> +       parsed_cmd->err = 0;

Remove this.

> +       parsed_cmd->cmd_function = NULL;
> +}
> +
> +static void command_def_cleanup(struct command_def *parsed_cmd)
> +{
> +       /* Nothing to do for now */

Then why define it?

> +}
> +
> +/* 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              parsed_cmd;
> +       int                             rc = 0;

Use a single space.

> +
> +       if (argc >= VSC9953_MAX_CMD_PARAMS) {
> +               rc = -1;

Use this:
+               rc = CMD_RET_USAGE;

> +               goto __ret;
> +       }
> +
> +       command_def_init(&parsed_cmd);
> +
> +       keywords_find(argc, argv, &parsed_cmd);

Use this:
+       rc = keywords_find(argc, argv, &parsed_cmd);
+       if (rc)
+               goto __ret_cmd_cleanup;

> +
> +       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().

> +
> +       rc = parsed_cmd.cmd_function(&parsed_cmd);
> +
> +       if (parsed_cmd.err) {
> +               rc = -1;
> +               goto __ret_cmd_cleanup;
> +       }

Remove this if statement along with parsed_cmd.err.

> +
> +__ret_cmd_cleanup:
> +       command_def_cleanup(&parsed_cmd);
> +__ret:
> +       return rc;
> +}
> +
> +U_BOOT_CMD(ethsw, VSC9953_MAX_CMD_PARAMS, 0, do_ethsw,
>            "vsc9953 l2 switch commands",
> -          "port <port_no> enable|disable\n"
> -          "    - enable/disable an l2 switch port\n"
> -          "      port_no=0..9; use \"all\" for all ports\n"
> -          "ethsw port <port_no> show\n"
> -          "    - show an l2 switch port's configuration\n"
> -          "      port_no=0..9; use \"all\" for all ports\n"
> +          VSC9953_PORT_CONF_HELP"\n"
>  );
> +
>  #endif /* CONFIG_VSC9953_CMD */
> --
> 1.9.3
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Hi Joe,

> -----Original Message-----

> From: Joe Hershberger [mailto:joe.hershberger@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

Hi Codrin,

On Tue, Jun 30, 2015 at 3:57 AM, Codrin Constantin Ciubotariu
<codrin.ciubotariu@freescale.com> wrote:
> Hi Joe,
>
>> -----Original Message-----
>> From: Joe Hershberger [mailto:joe.hershberger@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.

That's the idea. It would be great to keep it semi-generic.

Thanks,
-Joe

Hi Joe,

> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger@gmail.com]
> Sent: Wednesday, July 01, 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
> 
> Hi Codrin,
> 
> On Tue, Jun 30, 2015 at 3:57 AM, Codrin Constantin Ciubotariu
> <codrin.ciubotariu@freescale.com> wrote:
> > Hi Joe,
> >
> >> -----Original Message-----
> >> From: Joe Hershberger [mailto:joe.hershberger@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.
> 
> That's the idea. It would be great to keep it semi-generic.

I also need to create a do_ethsw.h that should contain at least struct keywords_to_function. I need this so that any driver could set its implementation in .keyword_function. Where should I put this header?

Best regards,
Codrin

Hi Codrin,

On Tue, Jul 7, 2015 at 8:08 AM, Codrin Constantin Ciubotariu
<codrin.ciubotariu@freescale.com> wrote:
> Hi Joe,
>
>> -----Original Message-----
>> From: Joe Hershberger [mailto:joe.hershberger@gmail.com]
>> Sent: Wednesday, July 01, 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
>>
>> Hi Codrin,
>>
>> On Tue, Jun 30, 2015 at 3:57 AM, Codrin Constantin Ciubotariu
>> <codrin.ciubotariu@freescale.com> wrote:
>> > Hi Joe,
>> >
>> >> -----Original Message-----
>> >> From: Joe Hershberger [mailto:joe.hershberger@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.
>>
>> That's the idea. It would be great to keep it semi-generic.
>
> I also need to create a do_ethsw.h that should contain at least struct keywords_to_function. I need this so that any driver could set its implementation in .keyword_function. Where should I put this header?

I think you should add a file simply called "include/ethsw.h"

Cheers,
-Joe