[PATCH v2 2/4] cmd: Add a sysinfo command

[Simon Glass]

Hi Detlev,

On Wed, 28 Jun 2023 at 20:33, Detlev Casanova
<detlev.casanova at collabora.com> wrote:
>
> The command is able to show different information for the running
> system:
> * Model name
> * Board ID
> * Revision
>
> This command can be used by boot shell scripts to select configurations
> depending on the specific running system.
>
> Signed-off-by: Detlev Casanova <detlev.casanova at collabora.com>
> ---
>  cmd/Kconfig   |   6 +++
>  cmd/Makefile  |   1 +
>  cmd/sysinfo.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 141 insertions(+)
>  create mode 100644 cmd/sysinfo.c

Please add a test - see test/dm/sysinfo.c

Please also add doc/usage/cmd/sysinfo.rst

>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 02e54f1e50f..9fb778ce809 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -210,6 +210,12 @@ config CMD_SBI
>         help
>           Display information about the SBI implementation.
>
> +config CMD_SYSINFO
> +       bool "sysinfo"
> +       depends on SYSINFO
> +       help
> +         Display information about the system.
> +
>  endmenu
>
>  menu "Boot commands"
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 6c37521b4e2..ba4d6de9a1b 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -165,6 +165,7 @@ obj-$(CONFIG_CMD_SPI) += spi.o
>  obj-$(CONFIG_CMD_STRINGS) += strings.o
>  obj-$(CONFIG_CMD_SMC) += smccc.o
>  obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o
> +obj-$(CONFIG_CMD_SYSINFO) += sysinfo.o
>  obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o
>  obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o
>  obj-$(CONFIG_CMD_TERMINAL) += terminal.o
> diff --git a/cmd/sysinfo.c b/cmd/sysinfo.c
> new file mode 100644
> index 00000000000..513ea0416a2
> --- /dev/null
> +++ b/cmd/sysinfo.c
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2023
> + * Detlev Casanova <detlev.casanova at collabora.com>
> + */
> +
> +#include <command.h>
> +#include <env.h>
> +#include <sysinfo.h>
> +#include <vsprintf.h>
> +
> +static int get_sysinfo(struct udevice **dev)

Please use devp since this is the common convention (indicating it is
a return value)

> +{
> +       int ret = sysinfo_get(dev);
> +
> +       if (ret) {
> +               debug("Cannot get sysinfo: %d\n", ret);

printf() to be more helpful?

> +               return ret;
> +       }
> +
> +       ret = sysinfo_detect(*dev);
> +       if (ret) {
> +               debug("Cannot detect sysinfo: %d\n", ret);

printf() to be more helpful?

> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int do_sysinfo_model(struct cmd_tbl *cmdtp, int flag, int argc,
> +                           char *const argv[])
> +{
> +       struct udevice *dev;
> +       char model[64];
> +       int ret = get_sysinfo(&dev);
> +
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +
> +       ret = sysinfo_get_str(dev,
> +                             SYSINFO_ID_BOARD_MODEL,
> +                             sizeof(model),
> +                             model);
> +
> +       if (ret) {
> +               debug("Cannot get sysinfo str: %d\n", ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc == 2)
> +               env_set(argv[1], model);

check return value

same below

> +       else
> +               printf("%s\n", model);
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static int do_sysinfo_id(struct cmd_tbl *cmdtp, int flag, int argc,
> +                        char *const argv[])
> +{
> +       struct udevice *dev;
> +       u32 board_id;
> +       int ret = get_sysinfo(&dev);
> +
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +
> +       ret = sysinfo_get_int(dev,
> +                             SYSINFO_ID_BOARD_ID,
> +                             &board_id);

can you fit on one line?

> +
> +       if (ret) {
> +               debug("Cannot get sysinfo int: %d\n", ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       if (argc == 2)
> +               env_set_hex(argv[1], board_id);
> +       else
> +               printf("0x%02x\n", board_id);
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static int do_sysinfo_revision(struct cmd_tbl *cmdtp, int flag, int argc,
> +                              char *const argv[])
> +{
> +       struct udevice *dev;
> +       int rev_major;
> +       int rev_minor;
> +       char rev[4];
> +       int ret = get_sysinfo(&dev);
> +
> +       if (ret)
> +               return CMD_RET_FAILURE;
> +
> +       ret = sysinfo_get_int(dev,
> +                             SYSINFO_ID_BOARD_REVISION_MAJOR,
> +                             &rev_major);
> +
> +       if (ret) {
> +               debug("Cannot get sysinfo int: %d\n", ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       ret = sysinfo_get_int(dev,
> +                             SYSINFO_ID_BOARD_REVISION_MINOR,
> +                             &rev_minor);
> +

drop blank line between ret = and if (ret) ...please fix globally

> +       if (ret) {
> +               debug("Cannot get sysinfo int: %d\n", ret);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       snprintf(rev, sizeof(rev), "%d.%d", rev_major, rev_minor);
> +
> +       if (argc == 2)
> +               env_set(argv[1], rev);
> +       else
> +               printf("%s\n", rev);
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +static char sysinfo_help_text[] =
> +       "model <varname>     - Show or set the board model in varname\n"
> +       "sysinfo id <varname>        - Show or set the board id in varname (in format 0xHH)\n"
> +       "sysinfo revision <varname>  - Show or set the board revision in varname";
> +
> +U_BOOT_CMD_WITH_SUBCMDS(sysinfo, "System information", sysinfo_help_text,
> +                       U_BOOT_SUBCMD_MKENT(model, 2, 1, do_sysinfo_model),
> +                       U_BOOT_SUBCMD_MKENT(id, 2, 1, do_sysinfo_id),
> +                       U_BOOT_SUBCMD_MKENT(revision, 2, 1, do_sysinfo_revision),
> +);
> --
> 2.39.3
>

Regards,
Simon