| Message ID | 20240906093630.2428329-4-bigfoot@classfun.cn |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Introduce Photonicat power management MCU driver | expand |
On 9/6/24 02:36, Junhao Xie wrote: > This driver provides access to Photonicat PMU watchdog functionality. > > Signed-off-by: Junhao Xie <bigfoot@classfun.cn> > --- > drivers/watchdog/Kconfig | 12 +++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/photonicat-wdt.c | 124 ++++++++++++++++++++++++++++++ > 3 files changed, 137 insertions(+) > create mode 100644 drivers/watchdog/photonicat-wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index bae1d97cce89..4094216a1c09 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -300,6 +300,18 @@ config MENZ069_WATCHDOG > This driver can also be built as a module. If so the module > will be called menz069_wdt. > > +config PHOTONICAT_PMU_WDT > + tristate "Photonicat PMU Watchdog" > + depends on MFD_PHOTONICAT_PMU > + select WATCHDOG_CORE > + help > + This driver provides access to Photonicat PMU watchdog functionality. > + > + Say Y here to include support for the Photonicat PMU Watchdog. > + > + This driver can also be built as a module. If so the module > + will be called photonicat-wdt. > + > config WDAT_WDT > tristate "ACPI Watchdog Action Table (WDAT)" > depends on ACPI > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index b51030f035a6..14375af84039 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -234,6 +234,7 @@ obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o > obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o > obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o > obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o > +obj-$(CONFIG_PHOTONICAT_PMU_WDT) += photonicat-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 > diff --git a/drivers/watchdog/photonicat-wdt.c b/drivers/watchdog/photonicat-wdt.c > new file mode 100644 > index 000000000000..1e758fcfb49f > --- /dev/null > +++ b/drivers/watchdog/photonicat-wdt.c > @@ -0,0 +1,124 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2024 Junhao Xie <bigfoot@classfun.cn> > + */ > + > +#include <linux/mfd/photonicat-pmu.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/watchdog.h> > + > +struct pcat_watchdog { > + struct device *dev; I don't see what this is used for. > + struct pcat_pmu *pmu; > + struct watchdog_device wdd; > + u8 timeout; > + bool started; > +}; > + > +static const struct watchdog_info pcat_wdt_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, > + .identity = "Photonicat PMU Watchdog", > +}; > + > +static int pcat_wdt_setup(struct pcat_watchdog *data, int timeout) > +{ > + int ret; > + u8 time = 0; Unnecessary initialization. > + u8 times[3] = { 60, 60, 0 }; > + > + time = MIN(255, MAX(0, timeout)); > + > + ret = pcat_pmu_write_data(data->pmu, PCAT_CMD_WATCHDOG_TIMEOUT_SET, > + times, sizeof(times)); Where does this actually send the timeout to the chip ? > + if (!ret) > + data->started = timeout != 0; > + > + return ret; > +} > + > +static int pcat_wdt_start(struct watchdog_device *wdev) > +{ > + struct pcat_watchdog *data = watchdog_get_drvdata(wdev); > + > + return pcat_wdt_setup(data, data->timeout); > +} > + > +static int pcat_wdt_stop(struct watchdog_device *wdev) > +{ > + struct pcat_watchdog *data = watchdog_get_drvdata(wdev); > + > + return pcat_wdt_setup(data, 0); > +} > + > +static int pcat_wdt_ping(struct watchdog_device *wdev) > +{ > + struct pcat_watchdog *data = watchdog_get_drvdata(wdev); > + > + return pcat_pmu_send(data->pmu, PCAT_CMD_HEARTBEAT, NULL, 0); > +} > + > +static int pcat_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val) > +{ > + int ret = 0; > + struct pcat_watchdog *data = watchdog_get_drvdata(wdev); > + > + data->timeout = val; This needs to store 'timeout' in wdev. Storing it locally is unnecessary. > + if (data->started) > + ret = pcat_wdt_setup(data, data->timeout); This is misleading because it would permit setting the timeout to 0 when the watchdog isn't running, and then when the watchdog is started it would not really start it. The code should not use a local "started" variable but call watchdog_active(). It should also not accept "0" as a valid timeout. > + > + return ret; > +} > + > +static const struct watchdog_ops pcat_wdt_ops = { > + .owner = THIS_MODULE, > + .start = pcat_wdt_start, > + .stop = pcat_wdt_stop, > + .ping = pcat_wdt_ping, > + .set_timeout = pcat_wdt_set_timeout, > +}; > + > +static int pcat_watchdog_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct pcat_watchdog *watchdog; > + > + watchdog = devm_kzalloc(dev, sizeof(*watchdog), GFP_KERNEL); > + if (!watchdog) > + return -ENOMEM; > + > + watchdog->dev = dev; > + watchdog->pmu = dev_get_drvdata(dev->parent); > + watchdog->wdd.info = &pcat_wdt_info; > + watchdog->wdd.ops = &pcat_wdt_ops; > + watchdog->wdd.timeout = 60; > + watchdog->wdd.max_timeout = U8_MAX; > + watchdog->wdd.min_timeout = 0; This effectively lets the user ... kind of ... stop the watchdog by setting the timeout to 0. This is not acceptable. > + watchdog->wdd.parent = dev; > + > + watchdog_stop_on_reboot(&watchdog->wdd); > + watchdog_set_drvdata(&watchdog->wdd, watchdog); > + platform_set_drvdata(pdev, watchdog); > + No watchdog_init_timeout() ? > + return devm_watchdog_register_device(dev, &watchdog->wdd); > +} > + > +static const struct of_device_id pcat_watchdog_dt_ids[] = { > + { .compatible = "ariaboard,photonicat-pmu-watchdog", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, pcat_watchdog_dt_ids); > + > +static struct platform_driver pcat_watchdog_driver = { > + .driver = { > + .name = "photonicat-watchdog", > + .of_match_table = pcat_watchdog_dt_ids, > + }, > + .probe = pcat_watchdog_probe, > +}; > +module_platform_driver(pcat_watchdog_driver); > + > +MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>"); > +MODULE_DESCRIPTION("Photonicat PMU watchdog"); > +MODULE_LICENSE("GPL");
On 2024/9/6 19:52, Guenter Roeck wrote: > On 9/6/24 02:36, Junhao Xie wrote: >> This driver provides access to Photonicat PMU watchdog functionality. >> [...] >> + >> +struct pcat_watchdog { >> + struct device *dev; > > I don't see what this is used for. I used to use this for logging, but now they are gone, I will delete it. > [...] >> + >> +static int pcat_wdt_setup(struct pcat_watchdog *data, int timeout) >> +{ >> + int ret; >> + u8 time = 0; > > Unnecessary initialization. > >> + u8 times[3] = { 60, 60, 0 }; >> + >> + time = MIN(255, MAX(0, timeout)); >> + >> + ret = pcat_pmu_write_data(data->pmu, PCAT_CMD_WATCHDOG_TIMEOUT_SET, >> + times, sizeof(times)); > > Where does this actually send the timeout to the chip ? > I forgot to fill in timeout into times[2] during refactoring process, I will fix it. >> + if (!ret) [...]>> + >> +static int pcat_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val) >> +{ >> + int ret = 0; >> + struct pcat_watchdog *data = watchdog_get_drvdata(wdev); >> + >> + data->timeout = val; > > This needs to store 'timeout' in wdev. Storing it locally is unnecessary. > >> + if (data->started) >> + ret = pcat_wdt_setup(data, data->timeout); > > This is misleading because it would permit setting the timeout to > 0 when the watchdog isn't running, and then when the watchdog is started > it would not really start it. The code should not use a local "started" > variable but call watchdog_active(). It should also not accept "0" > as a valid timeout. > I will fix the pcat_wdt_set_timeout. >> + [...] >> + >> + watchdog->dev = dev; >> + watchdog->pmu = dev_get_drvdata(dev->parent); >> + watchdog->wdd.info = &pcat_wdt_info; >> + watchdog->wdd.ops = &pcat_wdt_ops; >> + watchdog->wdd.timeout = 60; >> + watchdog->wdd.max_timeout = U8_MAX; >> + watchdog->wdd.min_timeout = 0; > > This effectively lets the user ... kind of ... stop the watchdog > by setting the timeout to 0. This is not acceptable. > >> + watchdog->wdd.parent = dev; >> + >> + watchdog_stop_on_reboot(&watchdog->wdd); >> + watchdog_set_drvdata(&watchdog->wdd, watchdog); >> + platform_set_drvdata(pdev, watchdog); >> + > No watchdog_init_timeout() ? Thanks for your correction, I will fix it. > >> + return devm_watchdog_register_device(dev, &watchdog->wdd); [...] >> +MODULE_LICENSE("GPL"); > Thanks for your review, I will fix all problems in next version! Best regards, Junhao
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index bae1d97cce89..4094216a1c09 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -300,6 +300,18 @@ config MENZ069_WATCHDOG This driver can also be built as a module. If so the module will be called menz069_wdt. +config PHOTONICAT_PMU_WDT + tristate "Photonicat PMU Watchdog" + depends on MFD_PHOTONICAT_PMU + select WATCHDOG_CORE + help + This driver provides access to Photonicat PMU watchdog functionality. + + Say Y here to include support for the Photonicat PMU Watchdog. + + This driver can also be built as a module. If so the module + will be called photonicat-wdt. + config WDAT_WDT tristate "ACPI Watchdog Action Table (WDAT)" depends on ACPI diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index b51030f035a6..14375af84039 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -234,6 +234,7 @@ obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o +obj-$(CONFIG_PHOTONICAT_PMU_WDT) += photonicat-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 diff --git a/drivers/watchdog/photonicat-wdt.c b/drivers/watchdog/photonicat-wdt.c new file mode 100644 index 000000000000..1e758fcfb49f --- /dev/null +++ b/drivers/watchdog/photonicat-wdt.c @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2024 Junhao Xie <bigfoot@classfun.cn> + */ + +#include <linux/mfd/photonicat-pmu.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/watchdog.h> + +struct pcat_watchdog { + struct device *dev; + struct pcat_pmu *pmu; + struct watchdog_device wdd; + u8 timeout; + bool started; +}; + +static const struct watchdog_info pcat_wdt_info = { + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, + .identity = "Photonicat PMU Watchdog", +}; + +static int pcat_wdt_setup(struct pcat_watchdog *data, int timeout) +{ + int ret; + u8 time = 0; + u8 times[3] = { 60, 60, 0 }; + + time = MIN(255, MAX(0, timeout)); + + ret = pcat_pmu_write_data(data->pmu, PCAT_CMD_WATCHDOG_TIMEOUT_SET, + times, sizeof(times)); + if (!ret) + data->started = timeout != 0; + + return ret; +} + +static int pcat_wdt_start(struct watchdog_device *wdev) +{ + struct pcat_watchdog *data = watchdog_get_drvdata(wdev); + + return pcat_wdt_setup(data, data->timeout); +} + +static int pcat_wdt_stop(struct watchdog_device *wdev) +{ + struct pcat_watchdog *data = watchdog_get_drvdata(wdev); + + return pcat_wdt_setup(data, 0); +} + +static int pcat_wdt_ping(struct watchdog_device *wdev) +{ + struct pcat_watchdog *data = watchdog_get_drvdata(wdev); + + return pcat_pmu_send(data->pmu, PCAT_CMD_HEARTBEAT, NULL, 0); +} + +static int pcat_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val) +{ + int ret = 0; + struct pcat_watchdog *data = watchdog_get_drvdata(wdev); + + data->timeout = val; + if (data->started) + ret = pcat_wdt_setup(data, data->timeout); + + return ret; +} + +static const struct watchdog_ops pcat_wdt_ops = { + .owner = THIS_MODULE, + .start = pcat_wdt_start, + .stop = pcat_wdt_stop, + .ping = pcat_wdt_ping, + .set_timeout = pcat_wdt_set_timeout, +}; + +static int pcat_watchdog_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct pcat_watchdog *watchdog; + + watchdog = devm_kzalloc(dev, sizeof(*watchdog), GFP_KERNEL); + if (!watchdog) + return -ENOMEM; + + watchdog->dev = dev; + watchdog->pmu = dev_get_drvdata(dev->parent); + watchdog->wdd.info = &pcat_wdt_info; + watchdog->wdd.ops = &pcat_wdt_ops; + watchdog->wdd.timeout = 60; + watchdog->wdd.max_timeout = U8_MAX; + watchdog->wdd.min_timeout = 0; + watchdog->wdd.parent = dev; + + watchdog_stop_on_reboot(&watchdog->wdd); + watchdog_set_drvdata(&watchdog->wdd, watchdog); + platform_set_drvdata(pdev, watchdog); + + return devm_watchdog_register_device(dev, &watchdog->wdd); +} + +static const struct of_device_id pcat_watchdog_dt_ids[] = { + { .compatible = "ariaboard,photonicat-pmu-watchdog", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, pcat_watchdog_dt_ids); + +static struct platform_driver pcat_watchdog_driver = { + .driver = { + .name = "photonicat-watchdog", + .of_match_table = pcat_watchdog_dt_ids, + }, + .probe = pcat_watchdog_probe, +}; +module_platform_driver(pcat_watchdog_driver); + +MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>"); +MODULE_DESCRIPTION("Photonicat PMU watchdog"); +MODULE_LICENSE("GPL");
This driver provides access to Photonicat PMU watchdog functionality. Signed-off-by: Junhao Xie <bigfoot@classfun.cn> --- drivers/watchdog/Kconfig | 12 +++ drivers/watchdog/Makefile | 1 + drivers/watchdog/photonicat-wdt.c | 124 ++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 drivers/watchdog/photonicat-wdt.c