From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Fri, 22 Mar 2019 15:53:15 +0800
Subject: [U-Boot] [PATCH 2/3] cmd: thermal: Add command line interface
	to read out temperatures
In-Reply-To: <20190311061259.31048-3-j-keerthy@ti.com>
References: <20190311061259.31048-1-j-keerthy@ti.com>
 <20190311061259.31048-3-j-keerthy@ti.com>
Message-ID: <CAPnjgZ1a1HAYx+nkx32zydTAvGLFN7e2zFWrA3yCx_q2B4cOvw@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Keerthy,

On Mon, 11 Mar 2019 at 14:13, Keerthy <j-keerthy@ti.com> wrote:
>
> Add command line interface to read out temperatures from SoC.
> Takes two arguments. One is 'temperature' and the other is
> instance number. In case instance number is not provided by
> default 0th instance temperature is read out.
>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  cmd/Kconfig   |  5 +++++
>  cmd/Makefile  |  1 +
>  cmd/thermal.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
>  create mode 100644 cmd/thermal.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 4bcc5c4557..d252e93d64 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1044,6 +1044,11 @@ config CMD_SPI
>         help
>           SPI utility command.
>
> +config CMD_THERMAL
> +       bool "thermal"
> +       help
> +         THERMAL support.

Thermal support

> +
>  config CMD_TSI148
>         bool "tsi148 - Command to access tsi148 device"
>         help
> diff --git a/cmd/Makefile b/cmd/Makefile
> index acb85f49fb..2b66f9e36a 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -128,6 +128,7 @@ obj-$(CONFIG_CMD_SPI) += spi.o
>  obj-$(CONFIG_CMD_STRINGS) += strings.o
>  obj-$(CONFIG_CMD_SMC) += smccc.o
>  obj-$(CONFIG_CMD_TERMINAL) += terminal.o
> +obj-$(CONFIG_CMD_THERMAL) += thermal.o
>  obj-$(CONFIG_CMD_TIME) += time.o
>  obj-$(CONFIG_CMD_TRACE) += trace.o
>  obj-$(CONFIG_HUSH_PARSER) += test.o
> diff --git a/cmd/thermal.c b/cmd/thermal.c
> new file mode 100644
> index 0000000000..a31f992425
> --- /dev/null
> +++ b/cmd/thermal.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Thermal CMD
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +#include <common.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <dm/uclass-internal.h>

Should go at the end

> +#include <thermal.h>
> +
> +#define MAX_TEMP       150

Is this in celsius?

> +
> +static int do_read_temp(cmd_tbl_t *cmdtp, int flag, int argc,
> +                       char * const argv[])
> +{
> +       struct udevice *tdev;
> +       int cpu_temp, ret = 0;
> +       u8 instance;

uint

Please use channel instead

> +       const char *units;
> +
> +       if (argc > 2) {
> +               printf("Max Number of arguments is 2\n");
> +               return -EINVAL;

return CMD_RET_USAGE

If you return -ve values it can do strange things like cause the
calling shell to exit.

> +       }
> +
> +       /* In case instance is not given default 0th instnace is reported */

instance

> +       if (argc == 2)
> +               instance = (u8)simple_strtoul(argv[1], NULL, 10);

Drop u8 cast

> +       else
> +               instance = 0;
> +
> +       ret = uclass_get_device(UCLASS_THERMAL, 0, &tdev);

uclass_first_device_err()?

That is more robust than requiring it be seq #0.

> +       if (!ret) {
> +               ret = thermal_get_temp(tdev, instance, &cpu_temp);
> +               if (!ret) {
> +                       if (abs(cpu_temp) < MAX_TEMP)
> +                               units = "C";
> +                       else
> +                               units = "mC";

The interface should be in mC, then. We can't have the range of the
value determine the units.

> +
> +                       printf("Instance %d Temperature at %d%s\n", instance,
> +                              cpu_temp, units);
> +               } else {
> +                       debug(" - invalid sensor data\n");

Maybe -ENOENT means invalid channel
-ENODATA means the channel exists but temperature could not be read?

> +               }
> +       } else {
> +               printf(" - invalid sensor device\n");
> +       }
> +

if (ret)
   return CMD_RET_FAILURE;

return 0

> +       return ret;

Don't return -ve values from commands.

> +}
> +
> +U_BOOT_CMD(
> +       temperature, 2, 0, do_read_temp,
> +       "Reads temperature of a given sensor",
> +       " [sensor number] - number of the temperature sensor"
> +);
> --
> 2.17.1
>

Could we have a pytest for this command for sandbox?

Regards,
Simon