On Fri, Aug 12, 2022 at 5:11 PM Simon Glass <[email protected]> wrote: > > Hi Robert, > > On Fri, 12 Aug 2022 at 06:19, Robert Marko <[email protected]> wrote: > > > > Currently, there is no way for users to check the readings from thermal > > sensors from U-boot console, only some boards print it during boot. > > > > So, lets add a simple "temperature" command that allows listing thermal > > uclass devices and getting their value. > > > > Note that the thermal devices are intenionally probed if list is used as > > almost always they will not get probed otherwise and there is no way for > > users to manually call probe on a certain device from console. > > > > Assumption is made that temperature is returned in degrees C and not > > milidegrees like in Linux as this is what most drivers seem to return. > > > > Signed-off-by: Robert Marko <[email protected]> > > --- > > cmd/Kconfig | 6 ++++ > > cmd/Makefile | 1 + > > cmd/temperature.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 93 insertions(+) > > create mode 100644 cmd/temperature.c > > Don't forget to add doc/usage/command/.. and a sandbox test in test/cmd/ > > https://u-boot.readthedocs.io/en/latest/develop/testing.html
Yeah, forgot those, will include them in v2. > > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig > > index 3625ff2a50..9bd639c740 100644 > > --- a/cmd/Kconfig > > +++ b/cmd/Kconfig > > @@ -1435,6 +1435,12 @@ config DEFAULT_SPI_MODE > > depends on CMD_SPI > > default 0 > > > > +config CMD_TEMPERATURE > > + bool "temperature" > > How about: > > temperature - display the temperature from thermal sensors > > > + depends on DM_THERMAL > > + help > > + Provides a way to get the temperature reading from thermal > > sensors. > > It also allows listing. Will expand on this and the previous point. > > > + > > config CMD_TSI148 > > bool "tsi148 - Command to access tsi148 device" > > help > > diff --git a/cmd/Makefile b/cmd/Makefile > > index 5e43a1e022..8874462f1a 100644 > > --- a/cmd/Makefile > > +++ b/cmd/Makefile > > @@ -152,6 +152,7 @@ obj-$(CONFIG_CMD_STRINGS) += strings.o > > obj-$(CONFIG_CMD_SMC) += smccc.o > > obj-$(CONFIG_CMD_SYSBOOT) += sysboot.o > > obj-$(CONFIG_CMD_STACKPROTECTOR_TEST) += stackprot_test.o > > +obj-$(CONFIG_CMD_TEMPERATURE) += temperature.o > > obj-$(CONFIG_CMD_TERMINAL) += terminal.o > > obj-$(CONFIG_CMD_TIME) += time.o > > obj-$(CONFIG_CMD_TIMER) += timer.o > > diff --git a/cmd/temperature.c b/cmd/temperature.c > > new file mode 100644 > > index 0000000000..ccf839058e > > --- /dev/null > > +++ b/cmd/temperature.c > > @@ -0,0 +1,86 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > + > > +/* > > + * Copyright (c) 2022 Sartura Ltd. > > + * Written by Robert Marko <[email protected]> > > + */ > > + > > +#include <common.h> > > +#include <command.h> > > +#include <dm.h> > > +#include <dm/uclass-internal.h> > > I hope you can drop this > > > +#include <thermal.h> > > + > > +#define LIMIT_DEVNAME 30 > > + > > +static int do_get(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + struct udevice *dev; > > + int ret, temp; > > + > > + if (argc < 2) { > > + printf("thermal device not selected\n"); > > + return CMD_RET_FAILURE; > > + } > > + > > + ret = uclass_get_device_by_name(UCLASS_THERMAL, argv[1], &dev); > > You should use the get function normally, since it probes the device > and for most devices you should only call their methods when the > device is probed/activated. I actually wanted the behavior of uclass_get_device_by_name() but completely missed that it exists, so that is why uclass_foreach_dev_probe() was used in list to probe the drivers, but it still allowed for user to directly feed the device-name and crash U-boot as device being passed to thermal_get_temp was not probed. Will switch to uclass_get_device_by_name() in v2 to solve that. > > > + if (ret) { > > + printf("thermal device not found\n"); > > + return CMD_RET_FAILURE; > > + } > > + > > + ret = thermal_get_temp(dev, &temp); > > + if (ret) > > + return CMD_RET_FAILURE; > > + > > + printf("%s: %d°C\n", dev->name, temp); > > + > > + return CMD_RET_SUCCESS; > > +} > > + > > +static int do_list(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + struct udevice *dev; > > + > > + printf("| %-*.*s| %-*.*s| %s\n", > > + LIMIT_DEVNAME, LIMIT_DEVNAME, "Device", > > + LIMIT_DEVNAME, LIMIT_DEVNAME, "Driver", > > + "Parent"); > > + > > + uclass_foreach_dev_probe(UCLASS_THERMAL, dev) { > > + printf("| %-*.*s| %-*.*s| %s\n", > > + LIMIT_DEVNAME, LIMIT_DEVNAME, dev->name, > > + LIMIT_DEVNAME, LIMIT_DEVNAME, dev->driver->name, > > + dev->parent->name); > > + } > > BTW as I'm sure you know, this loop probes the device, which is fine. Yeah, that was done intentionally to make sure that all sensors are visible, it also had a side-effect of making an oversight with using non probing DM internal device get helper. > > > + > > + return CMD_RET_SUCCESS; > > +} > > + > > +static struct cmd_tbl temperature_subcmd[] = { > > + U_BOOT_CMD_MKENT(list, 1, 1, do_list, "", ""), > > + U_BOOT_CMD_MKENT(get, 2, 1, do_get, "", ""), > > +}; > > + > > +static int do_temperature(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + struct cmd_tbl *cmd; > > + > > + argc--; > > + argv++; > > + > > + cmd = find_cmd_tbl(argv[0], temperature_subcmd, > > ARRAY_SIZE(temperature_subcmd)); > > + if (!cmd || argc > cmd->maxargs) > > + return CMD_RET_USAGE; > > + > > + return cmd->cmd(cmdtp, flag, argc, argv); > > +} > > + > > +U_BOOT_CMD(temperature, CONFIG_SYS_MAXARGS, 1, do_temperature, > > + "thermal sensor temperature", > > + "list\t\tshow list of temperature sensors\n" > > + "get [thermal device name]\tprint temperature" > > in degrees C ? Will add it in v2. Thanks for the review. Regards, Robert > > > +); > > -- > > 2.37.1 > > > > Regards, > Simon -- Robert Marko Staff Embedded Linux Engineer Sartura Ltd. Lendavska ulica 16a 10000 Zagreb, Croatia Email: [email protected] Web: www.sartura.hr
