Re: [U-Boot] [PATCH 2/3] cmd: thermal: Add command line interface to read out temperatures

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