| Message ID | 20240906093630.2428329-1-bigfoot@classfun.cn |
|---|---|
| Headers | show |
| Series | Introduce Photonicat power management MCU driver | expand |
On 2024/9/6 17:45, Krzysztof Kozlowski wrote: > On 06/09/2024 11:36, Junhao Xie wrote: >> Initial support for the power management MCU in the Ariaboard Photonicat >> This patch series depends on Add support for Ariaboard Photonicat RK3568 [1] > > How it depends? This prevents merging. You must decouple the patchsets. > > > > Best regards, > Krzysztof > Thanks for your correction, I will decouple the patchsets. Dependencies: * "dt-bindings: Add documentation for Photonicat PMU" [1] depends on "dt-bindings: vendor-prefixes: Add prefix for Ariaboard" [2], which requires this vendor prefix. * "arm64: dts: rockchip: add Photonicat PMU support for Ariaboard Photonicat" [3] add nodes based on "arm64: dts: rockchip: add dts for Ariaboard Photonicat RK3568" [4] [1] https://lore.kernel.org/all/20240906093630.2428329-9-bigfoot@classfun.cn/ [2] https://lore.kernel.org/all/20240906045706.1004813-2-bigfoot@classfun.cn/ [3] https://lore.kernel.org/all/20240906093630.2428329-10-bigfoot@classfun.cn/ [4] https://lore.kernel.org/all/20240906045706.1004813-4-bigfoot@classfun.cn/ Best regards, Junhao
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
On 2024/9/6 17:53, Krzysztof Kozlowski wrote: > On 06/09/2024 11:36, Junhao Xie wrote: >> This commit adds support for Photonicat power management MCU on >> Ariaboard Photonicat. >> [...] >> + >> + pcat_pmu_battery: supply-battery { > > Drop unused labels. Everywhere. You are not making the code more readable. I will remove them. > >> + compatible = "ariaboard,photonicat-pmu-supply"; >> + label = "battery"; >> + monitored-battery = <&battery>; >> + power-supplies = <&pcat_pmu_charger>; > > Why do you reference internal design of the device as DTS? You cannot > have here other power supply, can you? I mistakenly thought power_supply_am_i_supplied() required power-supplies property, it actually does not, I will remove it. > > Best regards, > Krzysztof > Thanks for your review, I will fix all problems in next version! Best regards, Junhao
Hi Junhao, > Initial support for the power management MCU in the Ariaboard Photonicat > This patch series depends on Add support for Ariaboard Photonicat RK3568 [1] The official website says it's "Renesas RA2E1 cortex M23 ultra-low power MCU" Perhaps renaming the 'Photonicat MCU' to 'Renesas RA2E1 MCU' would be better?
Initial support for the power management MCU in the Ariaboard Photonicat This patch series depends on Add support for Ariaboard Photonicat RK3568 [1] Currently implemented features: Implement serial communication protocol with MCU [2]. Support watchdog in MCU. Shutdown by power button and system notifies MCU to power-off. Read charger and battery supply voltage and simply calculate capacity. Read board temperature sensor. Set status of the network status LED. Read and set the MCU real-time clock date time. [1] https://lore.kernel.org/linux-arm-kernel/20240906045706.1004813-1-bigfoot@classfun.cn/ [2] https://photonicat.com/wiki/PMU_Protocol Junhao Xie (9): mfd: Add driver for Photonicat power management MCU power: reset: add Photonicat PMU poweroff driver watchdog: Add Photonicat PMU watchdog driver power: supply: photonicat-supply: Add Photonicat PMU battery and charger rtc: Add Photonicat PMU real-time clock hwmon: Add support for Photonicat PMU board temperature sensor leds: add Photonicat PMU LED driver dt-bindings: Add documentation for Photonicat PMU arm64: dts: rockchip: add Photonicat PMU support for Ariaboard Photonicat .../hwmon/ariaboard,photonicat-pmu-hwmon.yaml | 40 ++ .../leds/ariaboard,photonicat-pmu-leds.yaml | 41 ++ .../mfd/ariaboard,photonicat-pmu.yaml | 107 ++++ .../ariaboard,photonicat-pmu-poweroff.yaml | 34 ++ .../ariaboard,photonicat-pmu-supply.yaml | 55 ++ .../rtc/ariaboard,photonicat-pmu-rtc.yaml | 37 ++ .../ariaboard,photonicat-pmu-watchdog.yaml | 37 ++ .../boot/dts/rockchip/rk3568-photonicat.dts | 43 ++ drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/photonicat-hwmon.c | 129 +++++ drivers/leds/Kconfig | 11 + drivers/leds/Makefile | 1 + drivers/leds/leds-photonicat.c | 75 +++ drivers/mfd/Kconfig | 13 + drivers/mfd/Makefile | 1 + drivers/mfd/photonicat-pmu.c | 501 ++++++++++++++++++ drivers/power/reset/Kconfig | 12 + drivers/power/reset/Makefile | 1 + drivers/power/reset/photonicat-poweroff.c | 95 ++++ drivers/power/supply/Kconfig | 12 + drivers/power/supply/Makefile | 1 + drivers/power/supply/photonicat-supply.c | 250 +++++++++ drivers/rtc/Kconfig | 12 + drivers/rtc/Makefile | 1 + drivers/rtc/rtc-photonicat.c | 190 +++++++ drivers/watchdog/Kconfig | 12 + drivers/watchdog/Makefile | 1 + drivers/watchdog/photonicat-wdt.c | 124 +++++ include/linux/mfd/photonicat-pmu.h | 86 +++ 30 files changed, 1933 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml create mode 100644 Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml create mode 100644 Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml create mode 100644 Documentation/devicetree/bindings/power/reset/ariaboard,photonicat-pmu-poweroff.yaml create mode 100644 Documentation/devicetree/bindings/power/supply/ariaboard,photonicat-pmu-supply.yaml create mode 100644 Documentation/devicetree/bindings/rtc/ariaboard,photonicat-pmu-rtc.yaml create mode 100644 Documentation/devicetree/bindings/watchdog/ariaboard,photonicat-pmu-watchdog.yaml create mode 100644 drivers/hwmon/photonicat-hwmon.c create mode 100644 drivers/leds/leds-photonicat.c create mode 100644 drivers/mfd/photonicat-pmu.c create mode 100644 drivers/power/reset/photonicat-poweroff.c create mode 100644 drivers/power/supply/photonicat-supply.c create mode 100644 drivers/rtc/rtc-photonicat.c create mode 100644 drivers/watchdog/photonicat-wdt.c create mode 100644 include/linux/mfd/photonicat-pmu.h