| 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
On Fri, 06 Sep 2024, Junhao Xie wrote: > Photonicat has a network status LED that can be controlled by system. > The LED status can be set through command 0x19. > > Signed-off-by: Junhao Xie <bigfoot@classfun.cn> > --- > drivers/leds/Kconfig | 11 +++++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-photonicat.c | 75 ++++++++++++++++++++++++++++++++++ > 3 files changed, 87 insertions(+) > create mode 100644 drivers/leds/leds-photonicat.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 8d9d8da376e4..539adb5944e6 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -381,6 +381,17 @@ config LEDS_PCA9532_GPIO > To use a pin as gpio pca9532_type in pca9532_platform data needs to > set to PCA9532_TYPE_GPIO. > > +config LEDS_PHOTONICAT_PMU > + tristate "LED Support for Photonicat PMU" > + depends on LEDS_CLASS > + depends on MFD_PHOTONICAT_PMU > + help > + Photonicat has a network status LED that can be controlled by system, "the system" > + this option enables support for LEDs connected to the Photonicat PMU. > + > + To compile this driver as a module, choose M here: the > + module will be called leds-photonicat. > + > config LEDS_GPIO > tristate "LED Support for GPIO connected LEDs" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 18afbb5a23ee..dcd5312aee12 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o > obj-$(CONFIG_LEDS_PCA955X) += leds-pca955x.o > obj-$(CONFIG_LEDS_PCA963X) += leds-pca963x.o > obj-$(CONFIG_LEDS_PCA995X) += leds-pca995x.o > +obj-$(CONFIG_LEDS_PHOTONICAT_PMU) += leds-photonicat.o > obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o > obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o > obj-$(CONFIG_LEDS_PWM) += leds-pwm.o > diff --git a/drivers/leds/leds-photonicat.c b/drivers/leds/leds-photonicat.c > new file mode 100644 > index 000000000000..3aa5ce525b83 > --- /dev/null > +++ b/drivers/leds/leds-photonicat.c > @@ -0,0 +1,75 @@ > +// 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/leds.h> Alphabetical. > +struct pcat_leds { > + struct device *dev; Where is this used? > + struct pcat_pmu *pmu; Why do you need to store this? Can't you get this at the call-site by: dev_get_drvdata(cdev->dev->parent) > + struct led_classdev cdev; > +}; > + > +static int pcat_led_status_set(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct pcat_leds *leds = container_of(cdev, struct pcat_leds, cdev); > + struct pcat_data_cmd_led_setup setup = { 0, 0, 0 }; > + > + if (brightness) > + setup.on_time = 100; > + else > + setup.down_time = 100; > + return pcat_pmu_write_data(leds->pmu, PCAT_CMD_NET_STATUS_LED_SETUP, > + &setup, sizeof(setup)); > +} > + > +static int pcat_leds_probe(struct platform_device *pdev) > +{ > + int ret; Small sized variables at the bottom please. > + struct device *dev = &pdev->dev; > + struct pcat_leds *leds; > + const char *label; > + > + leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL); > + if (!leds) > + return -ENOMEM; > + > + leds->dev = dev; Where is this used? > + leds->pmu = dev_get_drvdata(dev->parent); > + platform_set_drvdata(pdev, leds); Where do you platform_get_drvdata() > + ret = of_property_read_string(dev->of_node, "label", &label); > + if (ret) > + return dev_err_probe(dev, ret, "No label property\n"); > + > + leds->cdev.name = label; > + leds->cdev.max_brightness = 1; > + leds->cdev.brightness_set_blocking = pcat_led_status_set; > + > + return devm_led_classdev_register(dev, &leds->cdev); > +} > + > +static const struct of_device_id pcat_leds_dt_ids[] = { > + { .compatible = "ariaboard,photonicat-pmu-leds", }, How many LEDs are there? > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, pcat_leds_dt_ids); > + > +static struct platform_driver pcat_leds_driver = { > + .driver = { > + .name = "photonicat-leds", > + .of_match_table = pcat_leds_dt_ids, > + }, > + .probe = pcat_leds_probe, > +}; > +module_platform_driver(pcat_leds_driver); > + > +MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>"); > +MODULE_DESCRIPTION("Photonicat PMU Status LEDs"); > +MODULE_LICENSE("GPL"); > -- > 2.46.0 >
On 2024/10/2 23:35, Lee Jones wrote: > On Fri, 06 Sep 2024, Junhao Xie wrote: > >> Photonicat has a network status LED that can be controlled by system. >> The LED status can be set through command 0x19. [...] >> +config LEDS_PHOTONICAT_PMU >> + tristate "LED Support for Photonicat PMU" >> + depends on LEDS_CLASS >> + depends on MFD_PHOTONICAT_PMU >> + help >> + Photonicat has a network status LED that can be controlled by system, > > "the system" > >> + this option enables support for LEDs connected to the Photonicat PMU. [...] >> +++ b/drivers/leds/leds-photonicat.c >> @@ -0,0 +1,75 @@ >> +// 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/leds.h> > > Alphabetical. > >> +struct pcat_leds { >> + struct device *dev; > > Where is this used? I used it to print logs, but now it doesn't, I will remove it. > >> + struct pcat_pmu *pmu; > > Why do you need to store this? > > Can't you get this at the call-site by: > > dev_get_drvdata(cdev->dev->parent) Yes, I will change it. >> + struct led_classdev cdev; >> +}; [...] >> +static int pcat_leds_probe(struct platform_device *pdev) >> +{ >> + int ret; > > Small sized variables at the bottom please. > >> + struct device *dev = &pdev->dev; >> + struct pcat_leds *leds; >> + const char *label; >> + >> + leds = devm_kzalloc(dev, sizeof(*leds), GFP_KERNEL); >> + if (!leds) >> + return -ENOMEM; >> + >> + leds->dev = dev; > > Where is this used? > >> + leds->pmu = dev_get_drvdata(dev->parent); >> + platform_set_drvdata(pdev, leds); > > Where do you platform_get_drvdata() > >> + ret = of_property_read_string(dev->of_node, "label", &label); [...] >> +static const struct of_device_id pcat_leds_dt_ids[] = { >> + { .compatible = "ariaboard,photonicat-pmu-leds", }, > > How many LEDs are there? Photonicat has three LEDs: - system operation status indicator - charging status indicator - network status indicator and currently only one LED (network status indicator) can be controlled. >> + { /* sentinel */ } >> +}; [...] >> -- >> 2.46.0 Thanks for your review, I will fix all problems in next version! 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