[PATCH] cmd: add temperature command
Robert Marko
robert.marko at sartura.hr
Fri Aug 12 17:59:13 CEST 2022
On Fri, Aug 12, 2022 at 5:11 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Robert,
>
> On Fri, 12 Aug 2022 at 06:19, Robert Marko <robert.marko at sartura.hr> 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 <robert.marko at sartura.hr>
> > ---
> > 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 <robert.marko at sartura.hr>
> > + */
> > +
> > +#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: robert.marko at sartura.hr
Web: www.sartura.hr
More information about the U-Boot
mailing list