| Message ID | 20240906093630.2428329-1-bigfoot@classfun.cn |
|---|---|
| Headers | show |
| Series | Introduce Photonicat power management MCU driver | expand |
On 2024/9/6 17:43, Krzysztof Kozlowski wrote: > On 06/09/2024 11:36, Junhao Xie wrote: >> Add a driver for Photonicat power management MCU, which [...]>> +void *pcat_data_get_data(struct pcat_data *data) >> +{ >> + if (!data) >> + return NULL; >> + return data->data; >> +} >> +EXPORT_SYMBOL_GPL(pcat_data_get_data); > > You need kerneldoc... or just drop it. Looks a bit useless as an > export... Is it because you want to hide from your own driver pcat_data? > What for? It's your driver... Now struct pcat_data is in mfd-photonicat.c, I will move it to photonicat-pmu.h and remove pcat_data_get_data. > [...] >> +void pcat_pmu_unregister_notify(struct pcat_pmu *pmu, struct notifier_block *nb) > > You need kerneldoc. I will add missing kernel documentation for all exported functions. > [...] >> + >> +static const struct of_device_id pcat_pmu_dt_ids[] = { >> + { .compatible = "ariaboard,photonicat-pmu", }, > > Undocumented compatible. > > Remember about correct order of patches. ABI documentation is before users. > I will adjust order of patches. > [...] >> + >> +MODULE_ALIAS("platform:photonicat-pmu"); > > 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. > > And it is not even correct. This is not a platform driver! > Yes, I will remove MODULE_ALIAS line > > Best regards, > Krzysztof > Thanks for your review, I will fix all problems in next version! 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?
On 2024/9/8 17:30, Chukun Pan wrote: >> 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? Renesas RA2E1 is a MCU product line, and Ariaboard wrote firmware for this MCU. Maybe "Renesas RA2E1 MCU in Photonicat" would be better? Best regards, Junhao
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