On 17/01/2024 11:24, Lukasz Majczak wrote:
> This adds EC-based watchdog support for ChromeOS
> based devices equipped with embedded controller (EC).
>
> Signed-off-by: Lukasz Majczak <lma@xxxxxxxxxxxx>
> ---
> MAINTAINERS | 6 +
> drivers/mfd/cros_ec_dev.c | 9 +
> drivers/watchdog/Kconfig | 15 +
> drivers/watchdog/Makefile | 3 +
> drivers/watchdog/cros_ec_wdt.c | 303 ++++++++++++++++++
> .../linux/platform_data/cros_ec_commands.h | 78 ++---
> 6 files changed, 370 insertions(+), 44 deletions(-)
> create mode 100644 drivers/watchdog/cros_ec_wdt.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 391bbb855cbe..55cd626a525f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4952,6 +4952,12 @@ R: Sami Kyöstilä <skyostil@xxxxxxxxxxxx>
> S: Maintained
> F: drivers/platform/chrome/cros_hps_i2c.c
>
> +CHROMEOS EC WATCHDOG
> +M: Lukasz Majczak <lma@xxxxxxxxxxxx>
> +L: chrome-platform@xxxxxxxxxxxxxxx
> +S: Maintained
> +F: drivers/watchdog/cros_ec_wdt.c
> +
> CHRONTEL CH7322 CEC DRIVER
> M: Joe Tessler <jrt@xxxxxxxxxx>
> L: linux-media@xxxxxxxxxxxxxxx
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 79d393b602bf..60fe831cf30a 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -91,6 +91,10 @@ static const struct mfd_cell cros_usbpd_notify_cells[] = {
> { .name = "cros-usbpd-notify", },
> };
>
> +static const struct mfd_cell cros_ec_wdt_cells[] = {
> + { .name = "cros-ec-wdt-drv", }
> +};
> +
> static const struct cros_feature_to_cells cros_subdevices[] = {
> {
> .id = EC_FEATURE_CEC,
> @@ -107,6 +111,11 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
> .mfd_cells = cros_usbpd_charger_cells,
> .num_cells = ARRAY_SIZE(cros_usbpd_charger_cells),
> },
> + {
> + .id = EC_FEATURE_HANG_DETECT,
> + .mfd_cells = cros_ec_wdt_cells,
> + .num_cells = ARRAY_SIZE(cros_ec_wdt_cells),
> + },
> };
>
> static const struct mfd_cell cros_ec_platform_cells[] = {
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..1da4be661be8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2251,4 +2251,19 @@ config KEEMBAY_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called keembay_wdt.
>
> +#
> +# ChromeOS EC-based Watchdog
> +#
Drop comment, useless, copies what's below.
> +
> +config CROS_EC_WATCHDOG
> + tristate "ChromeOS EC-based watchdog driver"
> + select WATCHDOG_CORE
> + depends on CROS_EC
> + help
> + This is the watchdog driver for ChromeOS devices equipped with EC.
> + The EC watchdog - when enabled - expects to be kicked within a given
> + time (default set to 30 seconds) otherwise it will simple reboot
> + the AP. Priori to that it can also send an event (configurable timeout)
> + about upcoming reboot.
Instead you could say what will be the name of the module.
> +
> endif # WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 7cbc34514ec1..8295c209ddb0 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -234,3 +234,6 @@ obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
> obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
> obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
> obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
> +
> +# Cros EC watchdog
Drop comment.
Also, are you sure you placed it in appropriate place, not just at the
end of both files?
> +obj-$(CONFIG_CROS_EC_WATCHDOG) += cros_ec_wdt.o
> diff --git a/drivers/watchdog/cros_ec_wdt.c b/drivers/watchdog/cros_ec_wdt.c
> new file mode 100644
> index 000000000000..b461c0613269
> --- /dev/null
> +++ b/drivers/watchdog/cros_ec_wdt.c
> @@ -0,0 +1,303 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/of_device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/uaccess.h>
> +
> +#define CROS_EC_WATCHDOG_DEFAULT_TIME 30 /* seconds */
> +
> +#define DEV_NAME "cros-ec-wdt-dev"
Drop unused defines.
> +#define DRV_NAME "cros-ec-wdt-drv"
> +
> +static int cros_ec_wdt_ping(struct watchdog_device *wdd);
> +static int cros_ec_wdt_start(struct watchdog_device *wdd);
> +static int cros_ec_wdt_stop(struct watchdog_device *wdd);
> +static int cros_ec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int t);
> +
...
> +
> +static int cros_ec_wdt_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + /* We need to get a reference to cros_ec_devices
> + * (provides communication layer) which is a parent of
> + * the cros-ec-dev (our parent)
> + */
> + struct cros_ec_device *cros_ec = dev_get_drvdata(dev->parent->parent);
> + int ret = 0;
> + uint32_t bootstatus;
> +
> + if (!cros_ec) {
> + ret = -ENODEV;
> + dev_err_probe(dev, ret, "There is no coresponding EC device!");
Syntax is return dev_err_probe
> + return ret;
> + }
> +
> + cros_ec_wdd.parent = &pdev->dev;
> + wd_data.cros_ec = cros_ec;
> + wd_data.wdd = &cros_ec_wdd;
> + wd_data.start_on_resume = false;
> + wd_data.keepalive_on = false;
> + wd_data.wdd->timeout = CROS_EC_WATCHDOG_DEFAULT_TIME;
> +
> + watchdog_stop_on_reboot(&cros_ec_wdd);
> + watchdog_stop_on_unregister(&cros_ec_wdd);
> + watchdog_set_drvdata(&cros_ec_wdd, &wd_data);
> + platform_set_drvdata(pdev, &wd_data);
> +
> + /* Get current AP boot status */
> + ret = cros_ec_wdt_send_hang_detect(&wd_data, EC_HANG_DETECT_CMD_GET_STATUS, 0,
> + &bootstatus);
> + if (ret < 0) {
> + dev_err_probe(dev, ret, "Couldn't get AP boot status from EC");
Syntax is return dev_err_probe
> + return ret;
> + }
> +
> + /*
> + * If bootstatus is not EC_HANG_DETECT_AP_BOOT_NORMAL
> + * it mens EC has rebooted the AP due to watchdog timeout.
> + * Lets translate it to watchdog core status code.
> + */
> + if (bootstatus != EC_HANG_DETECT_AP_BOOT_NORMAL)
> + wd_data.wdd->bootstatus = WDIOF_CARDRESET;
> +
> + ret = watchdog_register_device(&cros_ec_wdd);
> + if (ret < 0)
> + dev_err_probe(dev, ret, "Couldn't get AP boot status from EC");
Syntax is return dev_err_probe
> +
> + return ret;
return 0
> +}
> +
> +static int cros_ec_wdt_remove(struct platform_device *pdev)
> +{
> + struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> +
> + watchdog_unregister_device(wd_data->wdd);
> +
> + return 0;
> +}
> +
> +static void cros_ec_wdt_shutdown(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> + int ret;
> +
> + /*
> + * Clean only bootstatus flag.
> + * EC wdt is are already stopped by watchdog framework.
> + */
> + ret = cros_ec_wdt_send_hang_detect(wd_data,
> + EC_HANG_DETECT_CMD_CLEAR_STATUS, 0, NULL);
> + if (ret < 0)
> + dev_err(dev, "%s failed (%d)", __func__, ret);
> +
> + watchdog_unregister_device(wd_data->wdd);
> +}
> +
> +static int __maybe_unused cros_ec_wdt_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + struct device *dev = &pdev->dev;
> + struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> + int ret;
> +
> + if (watchdog_active(wd_data->wdd)) {
> + ret = cros_ec_wdt_send_hang_detect(wd_data,
> + EC_HANG_DETECT_CMD_CANCEL, 0, NULL);
> + if (ret < 0)
> + dev_err(dev, "%s failed (%d)", __func__, ret);
> + wd_data->start_on_resume = true;
> + }
> +
> + return 0;
> +}
> +
> +static int __maybe_unused cros_ec_wdt_resume(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cros_ec_wdt_data *wd_data = platform_get_drvdata(pdev);
> + int ret;
> +
> + /* start_on_resume is only set if watchdog was active
> + * when device was going to sleep
> + */
> + if (wd_data->start_on_resume) {
> + /* On resume we just need to setup a EC watchdog the same way as
Use comment style from Linux coding style.
> + * in cros_ec_wdt_start(). When userspace resumes from suspend
> + * the watchdog app should just start petting the watchdog again.
> + */
> + ret = cros_ec_wdt_start(wd_data->wdd);
> + if (ret < 0)
> + dev_err(dev, "%s failed (%d)", __func__, ret);
That's not really helpful. This applies everywhere. You have all over
the place debugging prints with __func__. This is not useful pattern.
Print something useful, not function name.
> +
> + wd_data->start_on_resume = false;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver cros_ec_wdt_driver = {
> + .probe = cros_ec_wdt_probe,
> + .remove = cros_ec_wdt_remove,
> + .shutdown = cros_ec_wdt_shutdown,
> + .suspend = pm_ptr(cros_ec_wdt_suspend),
> + .resume = pm_ptr(cros_ec_wdt_resume),
> + .driver = {
> + .name = DRV_NAME,
> + },
> +};
> +
> +module_platform_driver(cros_ec_wdt_driver);
> +
> +MODULE_ALIAS("platform:" DRV_NAME);
You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.
> +MODULE_DESCRIPTION("Cros EC Watchdog Device Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 7dae17b62a4d..35a7a2d32819 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -3961,60 +3961,50 @@ struct ec_response_i2c_passthru {
> } __ec_align1;
>
> /*****************************************************************************/
> -/* Power button hang detect */
> -
> +/* AP hang detect */
I don't understand how this change is related to the new driver. This
entire hunk is confusing.
Best regards,
Krzysztof
