| Message ID | 20240906093630.2428329-1-bigfoot@classfun.cn |
|---|---|
| Headers | show |
| Series | Introduce Photonicat power management MCU driver | expand |
On 06/09/2024 11:36, Junhao Xie wrote: > Add a driver for Photonicat power management MCU, which > provides battery and charger power supply, real-time clock, > watchdog, hardware shutdown. > > This driver implementes core MFD/serdev device as well as > communication subroutines necessary for commanding the device. > > Signed-off-by: Junhao Xie <bigfoot@classfun.cn> > --- > drivers/mfd/Kconfig | 13 + > drivers/mfd/Makefile | 1 + > drivers/mfd/photonicat-pmu.c | 501 +++++++++++++++++++++++++++++ > include/linux/mfd/photonicat-pmu.h | 86 +++++ > 4 files changed, 601 insertions(+) > create mode 100644 drivers/mfd/photonicat-pmu.c > create mode 100644 include/linux/mfd/photonicat-pmu.h ... > +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... > + > +int pcat_pmu_send(struct pcat_pmu *pmu, enum pcat_pmu_cmd cmd, > + const void *data, size_t len) > +{ > + u16 frame_id = atomic_inc_return(&pmu->frame); > + > + return pcat_pmu_raw_write(pmu, frame_id, cmd, false, data, len); > +} > +EXPORT_SYMBOL_GPL(pcat_pmu_send); > + > +int pcat_pmu_execute(struct pcat_request *request) > +{ > + int ret = 0, retries = 0; > + unsigned long flags; > + struct pcat_pmu *pmu = request->pmu; > + struct pcat_request_request *req = &request->request; > + struct pcat_request_reply *reply = &request->reply; > + > + init_completion(&request->received); > + memset(reply, 0, sizeof(request->reply)); > + > + mutex_lock(&pmu->reply_lock); > + if (request->frame_id == 0) > + request->frame_id = atomic_inc_return(&pmu->frame); > + pmu->reply = request; > + mutex_unlock(&pmu->reply_lock); > + > + if (req->want == 0) > + req->want = req->cmd + 1; > + > + dev_dbg(pmu->dev, "frame 0x%04X execute cmd 0x%02X\n", > + request->frame_id, req->cmd); > + > + while (1) { > + spin_lock_irqsave(&pmu->bus_lock, flags); > + ret = pcat_pmu_raw_write(pmu, request->frame_id, req->cmd, > + true, req->data, req->size); > + spin_unlock_irqrestore(&pmu->bus_lock, flags); > + if (ret < 0) { > + dev_err(pmu->dev, > + "frame 0x%04X write 0x%02X cmd failed: %d\n", > + request->frame_id, req->cmd, ret); > + goto fail; > + } > + dev_dbg(pmu->dev, "frame 0x%04X waiting response for 0x%02X\n", > + request->frame_id, req->cmd); > + if (!wait_for_completion_timeout(&request->received, HZ)) { > + if (retries < 3) { > + retries++; > + continue; > + } else { > + dev_warn(pmu->dev, > + "frame 0x%04X cmd 0x%02X timeout\n", > + request->frame_id, req->cmd); > + ret = -ETIMEDOUT; > + goto fail; > + } > + } > + break; > + } > + dev_dbg(pmu->dev, "frame 0x%04X got response 0x%02X\n", > + request->frame_id, reply->head.command); > + > + return 0; > +fail: > + mutex_lock(&pmu->reply_lock); > + pmu->reply = NULL; > + mutex_unlock(&pmu->reply_lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(pcat_pmu_execute); You need kerneldoc. > + > +int pcat_pmu_write_data(struct pcat_pmu *pmu, enum pcat_pmu_cmd cmd, > + const void *data, size_t size) > +{ > + int ret; > + struct pcat_request request = { > + .pmu = pmu, > + .request.cmd = cmd, > + .request.data = data, > + .request.size = size, > + }; > + ret = pcat_pmu_execute(&request); > + if (request.reply.data) > + devm_kfree(pmu->dev, request.reply.data); > + return ret; > +} > +EXPORT_SYMBOL_GPL(pcat_pmu_write_data); You need kerneldoc. > + > +static const struct serdev_device_ops pcat_pmu_serdev_device_ops = { > + .receive_buf = pcat_pmu_receive_buf, > + .write_wakeup = serdev_device_write_wakeup, > +}; > + > +int pcat_pmu_register_notify(struct pcat_pmu *pmu, struct notifier_block *nb) You need kerneldoc. > +{ > + return blocking_notifier_chain_register(&pmu->notifier_list, nb); > +} > +EXPORT_SYMBOL_GPL(pcat_pmu_register_notify); > + > +void pcat_pmu_unregister_notify(struct pcat_pmu *pmu, struct notifier_block *nb) You need kerneldoc. > +{ > + blocking_notifier_chain_unregister(&pmu->notifier_list, nb); > +} > +EXPORT_SYMBOL_GPL(pcat_pmu_unregister_notify); > + > +static int pcat_pmu_probe(struct serdev_device *serdev) > +{ > + int ret; > + u32 baudrate; > + u32 address; > + char buffer[64]; > + struct pcat_pmu *pmu = NULL; > + struct device *dev = &serdev->dev; > + > + pmu = devm_kzalloc(dev, sizeof(struct pcat_pmu), GFP_KERNEL); sizeof(*) > + if (!pmu) > + return -ENOMEM; Blank line > + pmu->dev = dev; > + pmu->serdev = serdev; > + spin_lock_init(&pmu->bus_lock); > + mutex_init(&pmu->reply_lock); > + init_completion(&pmu->first_status); > + > + if (of_property_read_u32(dev->of_node, "current-speed", &baudrate)) > + baudrate = 115200; > + > + if (of_property_read_u32(dev->of_node, "local-address", &address)) > + address = 1; > + pmu->local_id = address; > + > + if (of_property_read_u32(dev->of_node, "remote-address", &address)) > + address = 1; > + pmu->remote_id = address; > + > + serdev_device_set_client_ops(serdev, &pcat_pmu_serdev_device_ops); > + ret = devm_serdev_device_open(dev, serdev); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Failed to open serdev\n"); > + > + serdev_device_set_baudrate(serdev, baudrate); > + serdev_device_set_flow_control(serdev, false); > + serdev_device_set_parity(serdev, SERDEV_PARITY_NONE); > + dev_set_drvdata(dev, pmu); > + > + /* Disable watchdog on boot */ > + pcat_pmu_write_data(pmu, PCAT_CMD_WATCHDOG_TIMEOUT_SET, > + (u8[]){ 60, 60, 0 }, 3); > + > + /* Read hardware version */ > + pcat_pmu_read_string(pmu, PCAT_CMD_PMU_HW_VERSION_GET, > + buffer, sizeof(buffer)); > + if (buffer[0]) > + dev_info(dev, "PMU Hardware version: %s\n", buffer); dev_dbg > + > + /* Read firmware version */ > + pcat_pmu_read_string(pmu, PCAT_CMD_PMU_FW_VERSION_GET, > + buffer, sizeof(buffer)); > + if (buffer[0]) > + dev_info(dev, "PMU Firmware version: %s\n", buffer); dev_dbg. Your driver is supposed to be silent. > + > + return devm_of_platform_populate(dev); > +} > + > +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. > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, pcat_pmu_dt_ids); > + > +static struct serdev_device_driver pcat_pmu_driver = { > + .driver = { > + .name = "photonicat-pmu", > + .of_match_table = pcat_pmu_dt_ids, > + }, > + .probe = pcat_pmu_probe, > +}; > +module_serdev_device_driver(pcat_pmu_driver); > + > +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! Best regards, Krzysztof
On 06/09/2024 11:36, Junhao Xie wrote: > This driver implements the shutdown function of Photonicat PMU: > > - Host notifies PMU to shutdown: > When powering off, a shutdown command (0x0F) needs to be sent > to the MCU. > > - PMU notifies host to shutdown: > If the power button is long pressed, the MCU will send a shutdown > command (0x0D) to the system. > If system does not shutdown within 60 seconds, > the power will be turned off directly. > > Signed-off-by: Junhao Xie <bigfoot@classfun.cn> > --- > drivers/power/reset/Kconfig | 12 +++ > drivers/power/reset/Makefile | 1 + > drivers/power/reset/photonicat-poweroff.c | 95 +++++++++++++++++++++++ > 3 files changed, 108 insertions(+) > create mode 100644 drivers/power/reset/photonicat-poweroff.c > > diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig > index fece990af4a7..c59529ce25a2 100644 > --- a/drivers/power/reset/Kconfig > +++ b/drivers/power/reset/Kconfig > @@ -148,6 +148,18 @@ config POWER_RESET_ODROID_GO_ULTRA_POWEROFF > help > This driver supports Power off for Odroid Go Ultra device. > > +config POWER_RESET_PHOTONICAT_POWEROFF > + tristate "Photonicat PMU power-off driver" > + depends on MFD_PHOTONICAT_PMU || COMPILE_TEST, no? > + help > + This driver supports Power off for Photonicat PMU device. > + > + Supports operations: > + Host notifies PMU to shutdown > + PMU notifies host to shutdown > + > + Say Y if you have a Photonicat board. > + > config POWER_RESET_PIIX4_POWEROFF > tristate "Intel PIIX4 power-off driver" > depends on PCI > diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile > index a95d1bd275d1..339b36812b95 100644 > --- a/drivers/power/reset/Makefile > +++ b/drivers/power/reset/Makefile > @@ -17,6 +17,7 @@ obj-$(CONFIG_POWER_RESET_MT6323) += mt6323-poweroff.o > obj-$(CONFIG_POWER_RESET_QCOM_PON) += qcom-pon.o > obj-$(CONFIG_POWER_RESET_OCELOT_RESET) += ocelot-reset.o > obj-$(CONFIG_POWER_RESET_ODROID_GO_ULTRA_POWEROFF) += odroid-go-ultra-poweroff.o > +obj-$(CONFIG_POWER_RESET_PHOTONICAT_POWEROFF) += photonicat-poweroff.o > obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o > obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o > obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o > diff --git a/drivers/power/reset/photonicat-poweroff.c b/drivers/power/reset/photonicat-poweroff.c > new file mode 100644 > index 000000000000..f9f1ea179247 > --- /dev/null > +++ b/drivers/power/reset/photonicat-poweroff.c > @@ -0,0 +1,95 @@ > +// 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/reboot.h> > + > +struct pcat_poweroff { > + struct device *dev; > + struct pcat_pmu *pmu; > + struct notifier_block nb; > +}; > + > +static int pcat_do_poweroff(struct sys_off_data *data) > +{ > + struct pcat_poweroff *poweroff = data->cb_data; > + > + dev_info(poweroff->dev, "Host request PMU shutdown\n"); > + pcat_pmu_write_data(poweroff->pmu, PCAT_CMD_HOST_REQUEST_SHUTDOWN, > + NULL, 0); > + > + return NOTIFY_DONE; > +} > + > +static int pcat_poweroff_notify(struct notifier_block *nb, unsigned long action, > + void *data) > +{ > + struct pcat_poweroff *poweroff = > + container_of(nb, struct pcat_poweroff, nb); > + > + if (action != PCAT_CMD_PMU_REQUEST_SHUTDOWN) > + return NOTIFY_DONE; > + > + dev_info(poweroff->dev, "PMU request host shutdown\n"); Nope. Drop. > + orderly_poweroff(true); > + > + return NOTIFY_DONE; Best regards, Krzysztof
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
On 06/09/2024 11:36, Junhao Xie wrote: > This commit adds support for Photonicat power management MCU on > Ariaboard Photonicat. > > Signed-off-by: Junhao Xie <bigfoot@classfun.cn> > --- > .../boot/dts/rockchip/rk3568-photonicat.dts | 43 +++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts > index 2fe403cd61cb..597275702408 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts > @@ -513,6 +513,49 @@ &uart4 { > dma-names = "tx", "rx"; > status = "okay"; > /* Onboard power management MCU */ > + > + pcat_pmu: mcu { > + compatible = "ariaboard,photonicat-pmu"; > + current-speed = <115200>; > + local-address = <1>; > + remote-address = <1>; > + > + pcat_pmu_battery: supply-battery { Drop unused labels. Everywhere. You are not making the code more readable. > + 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? Best regards, Krzysztof
On 2024/9/6 17:44, Krzysztof Kozlowski wrote: > On 06/09/2024 11:36, Junhao Xie wrote: >> This driver implements the shutdown function of Photonicat PMU: >> [...] >> >> +config POWER_RESET_PHOTONICAT_POWEROFF >> + tristate "Photonicat PMU power-off driver" >> + depends on MFD_PHOTONICAT_PMU > > || COMPILE_TEST, no? I will add it depends on MFD_PHOTONICAT_PMU || COMPILE_TEST > >> + help >> + This driver supports Power off for Photonicat PMU device. [...] >> + >> +static int pcat_poweroff_notify(struct notifier_block *nb, unsigned long action, >> + void *data) >> +{ >> + struct pcat_poweroff *poweroff = >> + container_of(nb, struct pcat_poweroff, nb); >> + >> + if (action != PCAT_CMD_PMU_REQUEST_SHUTDOWN) >> + return NOTIFY_DONE; >> + >> + dev_info(poweroff->dev, "PMU request host shutdown\n"); > > Nope. Drop. I will remove this log print. The PMU will send a shutdown command when the power button is long pressed or the battery is low. I added this line of log to indicate the shutdown command initiated by the PMU so that users will not be confused by the sudden shutdown. > >> + orderly_poweroff(true); >> + >> + return NOTIFY_DONE; > Best regards, > Krzysztof > Thanks for your review, I will fix all problems in next version! Best regards, Junhao
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 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: > Photonicat PMU MCU will send status reports regularly, > including board temperature. > This is not an appropriate description. > Signed-off-by: Junhao Xie <bigfoot@classfun.cn> > --- > drivers/hwmon/Kconfig | 10 +++ > drivers/hwmon/Makefile | 1 + > drivers/hwmon/photonicat-hwmon.c | 129 +++++++++++++++++++++++++++++++ Documentation missing. > +static int pcat_hwmon_probe(struct platform_device *pdev) > +{ ... > + dev_info(dev, "Board Temprature: %d degress C\n", hwmon->temperature); > + Unacceptable (misspelled) noise. > + hwmon->hwmon = devm_hwmon_device_register_with_groups( > + dev, label, hwmon, pcat_pmu_temp_groups); > + Please use the with_info API. I am not going to review the code because it will need to be completely rewritten. Guenter
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 19:41, Guenter Roeck wrote: > On 9/6/24 02:36, Junhao Xie wrote: >> Photonicat PMU MCU will send status reports regularly, >> including board temperature. >> > > This is not an appropriate description. I will change to a better description. > >> Signed-off-by: Junhao Xie <bigfoot@classfun.cn> >> --- >> drivers/hwmon/Kconfig | 10 +++ >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/photonicat-hwmon.c | 129 +++++++++++++++++++++++++++++++ > > Documentation missing. Does it need to be placed in Documentation/hwmon/photonicat-hwmon.rst? > >> +static int pcat_hwmon_probe(struct platform_device *pdev) >> +{ > ... >> + dev_info(dev, "Board Temprature: %d degress C\n", hwmon->temperature); >> + > > Unacceptable (misspelled) noise. > >> + hwmon->hwmon = devm_hwmon_device_register_with_groups( >> + dev, label, hwmon, pcat_pmu_temp_groups); >> + > > Please use the with_info API. I am not going to review the code because > it will need to be completely rewritten. > > Guenter > Thanks for your review, I will rewrite this driver! 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
On 9/6/24 06:49, Junhao Xie wrote: > On 2024/9/6 19:41, Guenter Roeck wrote: >> On 9/6/24 02:36, Junhao Xie wrote: >>> Photonicat PMU MCU will send status reports regularly, >>> including board temperature. >>> >> >> This is not an appropriate description. > > I will change to a better description. > >> >>> Signed-off-by: Junhao Xie <bigfoot@classfun.cn> >>> --- >>> drivers/hwmon/Kconfig | 10 +++ >>> drivers/hwmon/Makefile | 1 + >>> drivers/hwmon/photonicat-hwmon.c | 129 +++++++++++++++++++++++++++++++ >> >> Documentation missing. > > Does it need to be placed in Documentation/hwmon/photonicat-hwmon.rst? > Yes. Thanks, Guenter
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