[PATCH 0/9] Introduce Photonicat power management MCU driver

[PATCH 0/9] Introduce Photonicat power management MCU driver

[Junhao Xie]

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

-- 
2.46.0

[PATCH 1/9] mfd: Add driver for Photonicat power management MCU

[Junhao Xie]

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

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bc8be2e593b6..5fd339aa0f9a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1135,6 +1135,19 @@ config MFD_PM8XXX
 	  Say M here if you want to include support for PM8xxx chips as a
 	  module. This will build a module called "pm8xxx-core".
 
+config MFD_PHOTONICAT_PMU
+	tristate "Photonicat Power Management MCU"
+	depends on OF
+	depends on SERIAL_DEV_BUS
+	select CRC16
+	select MFD_CORE
+	help
+	  Driver for the Power Management MCU in the Ariaboard Photonicat,
+	  which provides battery and charger power supply, real-time clock,
+	  watchdog, hardware shutdown.
+
+	  Say M or Y here to include this support.
+
 config MFD_QCOM_RPM
 	tristate "Qualcomm Resource Power Manager (RPM)"
 	depends on ARCH_QCOM && OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 02b651cd7535..25872850f216 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -223,6 +223,7 @@ obj-$(CONFIG_MFD_INTEL_LPSS_PCI)	+= intel-lpss-pci.o
 obj-$(CONFIG_MFD_INTEL_LPSS_ACPI)	+= intel-lpss-acpi.o
 obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
 obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
+obj-$(CONFIG_MFD_PHOTONICAT_PMU)	+= photonicat-pmu.o
 obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
 obj-$(CONFIG_MFD_NTXEC)		+= ntxec.o
 obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
diff --git a/drivers/mfd/photonicat-pmu.c b/drivers/mfd/photonicat-pmu.c
new file mode 100644
index 000000000000..e6bbaf256c50
--- /dev/null
+++ b/drivers/mfd/photonicat-pmu.c
@@ -0,0 +1,501 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Junhao Xie <bigfoot@classfun.cn>
+ */
+
+#include <linux/atomic.h>
+#include <linux/completion.h>
+#include <linux/crc16.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/mfd/photonicat-pmu.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/serdev.h>
+#include <linux/spinlock.h>
+
+#define PCAT_ADDR_CPU(id)	((id & 0x7F))
+#define PCAT_ADDR_PMU(id)	((id & 0x7F) | 0x80)
+#define PCAT_ADDR_CPU_ALL	0x80
+#define PCAT_ADDR_PMU_ALL	0xFE
+#define PCAT_ADDR_ALL		0xFF
+
+#define PCAT_MAGIC_HEAD	0xA5
+#define PCAT_MAGIC_END	0x5A
+
+struct pcat_data_head {
+	u8 magic_head;
+	u8 source;
+	u8 dest;
+	u16 frame_id;
+	u16 length;
+	u16 command;
+} __packed;
+
+struct pcat_data_foot {
+	u8 need_ack;
+	u16 crc16;
+	u8 magic_end;
+} __packed;
+
+struct pcat_data {
+	struct pcat_pmu *pmu;
+	struct pcat_data_head *head;
+	struct pcat_data_foot *foot;
+	void *data;
+	size_t size;
+};
+
+struct pcat_request {
+	struct pcat_pmu *pmu;
+	u16 frame_id;
+	struct completion received;
+	struct pcat_request_request {
+		u16 cmd;
+		u16 want;
+		const void *data;
+		size_t size;
+	} request;
+	struct pcat_request_reply {
+		struct pcat_data_head head;
+		struct pcat_data_foot foot;
+		void *data;
+		size_t size;
+	} reply;
+};
+
+struct pcat_pmu {
+	struct device *dev;
+	struct serdev_device *serdev;
+	atomic_t frame;
+	char buffer[8192];
+	size_t length;
+	struct pcat_request *reply;
+	spinlock_t bus_lock;
+	struct mutex reply_lock;
+	struct mutex status_lock;
+	struct completion first_status;
+	struct blocking_notifier_head notifier_list;
+	u8 local_id;
+	u8 remote_id;
+};
+
+void *pcat_data_get_data(struct pcat_data *data)
+{
+	if (!data)
+		return NULL;
+	return data->data;
+}
+EXPORT_SYMBOL_GPL(pcat_data_get_data);
+
+static int pcat_pmu_raw_write(struct pcat_pmu *pmu, u16 frame_id,
+			      enum pcat_pmu_cmd cmd, bool need_ack,
+			      const void *data, size_t len)
+{
+	int ret;
+	struct pcat_data_head head;
+	struct pcat_data_foot foot;
+
+	head.magic_head = PCAT_MAGIC_HEAD;
+	head.source = PCAT_ADDR_CPU(pmu->local_id);
+	head.dest = PCAT_ADDR_PMU(pmu->remote_id);
+	head.frame_id = frame_id;
+	head.length = len + sizeof(struct pcat_data_foot) - 1;
+	head.command = cmd;
+
+	ret = serdev_device_write_buf(pmu->serdev, (u8 *)&head, sizeof(head));
+	if (ret < 0) {
+		dev_err(pmu->dev, "failed to write frame head: %d", ret);
+		return ret;
+	}
+
+	ret = serdev_device_write_buf(pmu->serdev, data, len);
+	if (ret < 0) {
+		dev_err(pmu->dev, "failed to write frame body: %d", ret);
+		return ret;
+	}
+
+	foot.need_ack = need_ack;
+	foot.crc16 = 0;
+	foot.magic_end = PCAT_MAGIC_END;
+	foot.crc16 = crc16(0xFFFF, (u8 *)&head + 1, sizeof(head) - 1);
+	foot.crc16 = crc16(foot.crc16, data, len);
+	foot.crc16 = crc16(foot.crc16, (u8 *)&foot, 1);
+
+	ret = serdev_device_write_buf(pmu->serdev, (u8 *)&foot, sizeof(foot));
+	if (ret < 0)
+		dev_err(pmu->dev, "failed to send frame foot: %d", ret);
+
+	return ret;
+}
+
+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);
+
+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);
+
+int pcat_pmu_read_string(struct pcat_pmu *pmu, enum pcat_pmu_cmd cmd,
+			 char *str, size_t len)
+{
+	int ret;
+	struct pcat_request request = {
+		.pmu = pmu,
+		.request.cmd = cmd,
+	};
+	memset(str, 0, len);
+	ret = pcat_pmu_execute(&request);
+	if (request.reply.data) {
+		memcpy(str, request.reply.data,
+		       min(len - 1, request.reply.size));
+		devm_kfree(pmu->dev, request.reply.data);
+	};
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pcat_pmu_read_string);
+
+int pcat_pmu_write_u8(struct pcat_pmu *pmu, enum pcat_pmu_cmd cmd, u8 v)
+{
+	return pcat_pmu_write_data(pmu, cmd, &v, sizeof(v));
+}
+EXPORT_SYMBOL_GPL(pcat_pmu_write_u8);
+
+static bool pcat_process_reply(struct pcat_data *p)
+{
+	bool processed = false;
+	struct pcat_pmu *pmu = p->pmu;
+	struct device *dev = pmu->dev;
+	struct pcat_request *request;
+	struct pcat_request_request *req;
+	struct pcat_request_reply *reply;
+
+	mutex_lock(&pmu->reply_lock);
+	request = pmu->reply;
+	if (!request)
+		goto skip;
+
+	req = &request->request;
+	reply = &request->reply;
+	if (request->frame_id != p->head->frame_id) {
+		dev_dbg_ratelimited(dev, "skip mismatch frame %04X != %04X",
+				    request->frame_id, p->head->frame_id);
+		goto skip;
+	}
+
+	if (req->want == 0)
+		req->want = req->cmd + 1;
+
+	if (req->want != p->head->command) {
+		dev_dbg_ratelimited(
+			dev, "frame %04X skip mismatch command %02X != %02X",
+			request->frame_id, req->want, p->head->command);
+		goto skip;
+	}
+
+	if (completion_done(&request->received)) {
+		dev_dbg_ratelimited(dev, "frame %04X skip done completion",
+				    request->frame_id);
+		goto skip;
+	}
+
+	memcpy(&reply->head, p->head, sizeof(struct pcat_data_head));
+	memcpy(&reply->foot, p->foot, sizeof(struct pcat_data_foot));
+	if (p->data && p->size > 0) {
+		reply->data = devm_kzalloc(pmu->dev, p->size + 1, GFP_KERNEL);
+		if (pmu->reply->reply.data) {
+			memcpy(reply->data, p->data, p->size);
+			reply->size = p->size;
+		}
+	}
+
+	complete(&request->received);
+	pmu->reply = NULL;
+	processed = true;
+
+skip:
+	mutex_unlock(&pmu->reply_lock);
+	return processed;
+}
+
+static int pcat_process_data(struct pcat_pmu *pmu, const u8 *data, size_t len)
+{
+	int ret;
+	u16 crc16_calc;
+	size_t data_size = 0;
+	struct pcat_data frame;
+
+	memset(&frame, 0, sizeof(frame));
+	frame.pmu = pmu;
+	if (len < sizeof(struct pcat_data_head)) {
+		dev_dbg_ratelimited(pmu->dev, "head too small %zu < %zu\n", len,
+				    sizeof(struct pcat_data_head));
+		return -EAGAIN;
+	}
+
+	frame.head = (struct pcat_data_head *)data;
+	if (frame.head->magic_head != PCAT_MAGIC_HEAD) {
+		dev_dbg_ratelimited(pmu->dev, "bad head magic %02X\n",
+				    frame.head->magic_head);
+		return -EBADMSG;
+	}
+
+	if (frame.head->source != PCAT_ADDR_PMU(pmu->remote_id)) {
+		dev_dbg_ratelimited(pmu->dev, "unknown data source %02X\n",
+				    frame.head->source);
+		return 0;
+	}
+	if (frame.head->dest != PCAT_ADDR_CPU(pmu->local_id) &&
+	    frame.head->dest != PCAT_ADDR_CPU_ALL &&
+	    frame.head->dest != PCAT_ADDR_ALL) {
+		dev_dbg_ratelimited(pmu->dev, "not data destination %02X\n",
+				    frame.head->dest);
+		return 0;
+	}
+	if (frame.head->length < sizeof(struct pcat_data_foot) - 1 ||
+	    frame.head->length >= U16_MAX - 4) {
+		dev_dbg_ratelimited(pmu->dev, "invalid length %d\n",
+				    frame.head->length);
+		return -EBADMSG;
+	}
+	data_size = sizeof(struct pcat_data_head) + frame.head->length + 1;
+	if (data_size > len) {
+		dev_dbg_ratelimited(pmu->dev, "data too small %zu > %zu\n",
+				    data_size, len);
+		return -EAGAIN;
+	}
+	frame.data = (u8 *)data + sizeof(struct pcat_data_head);
+	frame.size = frame.head->length + 1 - sizeof(struct pcat_data_foot);
+	frame.foot = (struct pcat_data_foot *)(data + frame.size +
+					       sizeof(struct pcat_data_head));
+	if (frame.foot->magic_end != PCAT_MAGIC_END) {
+		dev_dbg_ratelimited(pmu->dev, "bad foot magic %02X\n",
+				    frame.foot->magic_end);
+		return -EBADMSG;
+	}
+	crc16_calc = crc16(0xFFFF, data + 1, frame.head->length + 6);
+	if (frame.foot->crc16 != crc16_calc) {
+		dev_warn_ratelimited(pmu->dev, "crc16 mismatch %04X != %04X\n",
+				     frame.foot->crc16, crc16_calc);
+		return -EBADMSG;
+	}
+
+	if (pcat_process_reply(&frame))
+		return 0;
+
+	ret = blocking_notifier_call_chain(&pmu->notifier_list,
+					   frame.head->command, &frame);
+	if (ret == NOTIFY_DONE && frame.foot->need_ack) {
+		pcat_pmu_raw_write(pmu, frame.head->frame_id,
+				   frame.head->command + 1, false, NULL, 0);
+	}
+
+	return 0;
+}
+
+static size_t pcat_pmu_receive_buf(struct serdev_device *serdev,
+				   const unsigned char *buf, size_t size)
+{
+	struct device *dev = &serdev->dev;
+	struct pcat_pmu *pmu = dev_get_drvdata(dev);
+	size_t processed = size;
+	int ret;
+	size_t new_len = pmu->length + size;
+
+	if (!pmu || !buf || size <= 0)
+		return 0;
+	if (new_len > sizeof(pmu->buffer)) {
+		new_len = sizeof(pmu->buffer);
+		processed = new_len - pmu->length;
+	}
+	if (pmu->length)
+		dev_dbg(pmu->dev, "got remaining message at %zu size %zu (%zu)",
+			pmu->length, processed, new_len);
+	memcpy(pmu->buffer + pmu->length, buf, processed);
+	pmu->length = new_len;
+	ret = pcat_process_data(pmu, pmu->buffer, pmu->length);
+	if (ret != -EAGAIN)
+		pmu->length = 0;
+	else
+		dev_dbg(pmu->dev, "got partial message %zu", pmu->length);
+	return processed;
+}
+
+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)
+{
+	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)
+{
+	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);
+	if (!pmu)
+		return -ENOMEM;
+	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);
+
+	/* 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);
+
+	return devm_of_platform_populate(dev);
+}
+
+static const struct of_device_id pcat_pmu_dt_ids[] = {
+	{ .compatible = "ariaboard,photonicat-pmu", },
+	{ /* 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");
+MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>");
+MODULE_DESCRIPTION("Photonicat Power Management Unit");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/photonicat-pmu.h b/include/linux/mfd/photonicat-pmu.h
new file mode 100644
index 000000000000..1bada5efd83a
--- /dev/null
+++ b/include/linux/mfd/photonicat-pmu.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2024 Junhao Xie <bigfoot@classfun.cn>
+ */
+
+#ifndef _PHOTONICAT_PMU_H
+#define _PHOTONICAT_PMU_H
+
+#include <linux/notifier.h>
+#include <linux/types.h>
+
+struct pcat_data;
+struct pcat_pmu;
+struct pcat_request;
+
+struct pcat_data_cmd_date_time {
+	u16 year;
+	u8 month;
+	u8 day;
+	u8 hour;
+	u8 minute;
+	u8 second;
+} __packed;
+
+struct pcat_data_cmd_led_setup {
+	u16 on_time;
+	u16 down_time;
+	u16 repeat;
+} __packed;
+
+struct pcat_data_cmd_status {
+	u16 battery_microvolt;
+	u16 charger_microvolt;
+	u16 gpio_input;
+	u16 gpio_output;
+	struct pcat_data_cmd_date_time time;
+	u16 reserved;
+	u8 temp;
+} __packed;
+
+enum pcat_pmu_cmd {
+	PCAT_CMD_HEARTBEAT			= 0x01,
+	PCAT_CMD_HEARTBEAT_ACK			= 0x02,
+	PCAT_CMD_PMU_HW_VERSION_GET		= 0x03,
+	PCAT_CMD_PMU_HW_VERSION_GET_ACK		= 0x04,
+	PCAT_CMD_PMU_FW_VERSION_GET		= 0x05,
+	PCAT_CMD_PMU_FW_VERSION_GET_ACK		= 0x06,
+	PCAT_CMD_STATUS_REPORT			= 0x07,
+	PCAT_CMD_STATUS_REPORT_ACK		= 0x08,
+	PCAT_CMD_DATE_TIME_SYNC			= 0x09,
+	PCAT_CMD_DATE_TIME_SYNC_ACK		= 0x0A,
+	PCAT_CMD_SCHEDULE_STARTUP_TIME_SET	= 0x0B,
+	PCAT_CMD_SCHEDULE_STARTUP_TIME_SET_ACK	= 0x0C,
+	PCAT_CMD_PMU_REQUEST_SHUTDOWN		= 0x0D,
+	PCAT_CMD_PMU_REQUEST_SHUTDOWN_ACK	= 0x0E,
+	PCAT_CMD_HOST_REQUEST_SHUTDOWN		= 0x0F,
+	PCAT_CMD_HOST_REQUEST_SHUTDOWN_ACK	= 0x10,
+	PCAT_CMD_PMU_REQUEST_FACTORY_RESET	= 0x11,
+	PCAT_CMD_PMU_REQUEST_FACTORY_RESET_ACK	= 0x12,
+	PCAT_CMD_WATCHDOG_TIMEOUT_SET		= 0x13,
+	PCAT_CMD_WATCHDOG_TIMEOUT_SET_ACK	= 0x14,
+	PCAT_CMD_CHARGER_ON_AUTO_START		= 0x15,
+	PCAT_CMD_CHARGER_ON_AUTO_START_ACK	= 0x16,
+	PCAT_CMD_VOLTAGE_THRESHOLD_SET		= 0x17,
+	PCAT_CMD_VOLTAGE_THRESHOLD_SET_ACK	= 0x18,
+	PCAT_CMD_NET_STATUS_LED_SETUP		= 0x19,
+	PCAT_CMD_NET_STATUS_LED_SETUP_ACK	= 0x1A,
+	PCAT_CMD_POWER_ON_EVENT_GET		= 0x1B,
+	PCAT_CMD_POWER_ON_EVENT_GET_ACK		= 0x1C,
+};
+
+void *pcat_data_get_data(struct pcat_data *data);
+int pcat_pmu_send(struct pcat_pmu *pmu, enum pcat_pmu_cmd cmd,
+		  const void *data, size_t len);
+int pcat_pmu_execute(struct pcat_request *request);
+int pcat_pmu_write_data(struct pcat_pmu *pmu, enum pcat_pmu_cmd cmd,
+			const void *data, size_t size);
+int pcat_pmu_read_string(struct pcat_pmu *pmu, enum pcat_pmu_cmd cmd,
+			 char *str, size_t len);
+int pcat_pmu_write_u8(struct pcat_pmu *pmu, enum pcat_pmu_cmd cmd, u8 value);
+int pcat_pmu_register_notify(struct pcat_pmu *pmu,
+			     struct notifier_block *nb);
+void pcat_pmu_unregister_notify(struct pcat_pmu *pmu,
+				struct notifier_block *nb);
+
+#endif
-- 
2.46.0

Re: [PATCH 1/9] mfd: Add driver for Photonicat power management MCU

[Krzysztof Kozlowski]

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

Re: [PATCH 1/9] mfd: Add driver for Photonicat power management MCU

[Junhao Xie]

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

Re: [PATCH 1/9] mfd: Add driver for Photonicat power management MCU

[Markus Elfring]

…
> +++ b/drivers/mfd/photonicat-pmu.c
> @@ -0,0 +1,501 @@
…
> +int pcat_pmu_execute(struct pcat_request *request)
> +{
…

Under which circumstances would you become interested to apply statements
like the following?


> +	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);
…

A) guard(mutex)(&pmu->reply_lock);
   https://elixir.bootlin.com/linux/v6.11-rc6/source/include/linux/mutex.h#L196



> +		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);
…

B) guard(spinlock_irqsave)(&pmu->bus_lock);
   https://elixir.bootlin.com/linux/v6.11-rc6/source/include/linux/spinlock.h#L572


Regards,
Markus

Re: [PATCH 1/9] mfd: Add driver for Photonicat power management MCU

[Junhao Xie]

On 2024/9/7 16:10, Markus Elfring wrote:
> …
>> +++ b/drivers/mfd/photonicat-pmu.c
>> @@ -0,0 +1,501 @@
> …
>> +int pcat_pmu_execute(struct pcat_request *request)
>> +{
> …
> 
> Under which circumstances would you become interested to apply statements
> like the following?
> 
> 
>> +	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);
> …
> 
> A) guard(mutex)(&pmu->reply_lock);
>    https://elixir.bootlin.com/linux/v6.11-rc6/source/include/linux/mutex.h#L196
> 
> 
> 
>> +		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);
> …
> 
> B) guard(spinlock_irqsave)(&pmu->bus_lock);
>    https://elixir.bootlin.com/linux/v6.11-rc6/source/include/linux/spinlock.h#L572
> 
> 
> Regards,
> Markus

Thanks for your suggestions, I will try these statements.

Best regards,
Junhao

Re: [PATCH 1/9] mfd: Add driver for Photonicat power management MCU

[Markus Elfring]

…
> +++ b/include/linux/mfd/photonicat-pmu.h
> @@ -0,0 +1,86 @@
…
> +#ifndef _PHOTONICAT_PMU_H
> +#define _PHOTONICAT_PMU_H
…

I suggest to omit leading underscores from such identifiers.
https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

Regards,
Markus

Re: [PATCH 1/9] mfd: Add driver for Photonicat power management MCU

[Junhao Xie]

On 2024/9/7 16:44, Markus Elfring wrote:
> …
>> +++ b/include/linux/mfd/photonicat-pmu.h
>> @@ -0,0 +1,86 @@
> …
>> +#ifndef _PHOTONICAT_PMU_H
>> +#define _PHOTONICAT_PMU_H
> …
> 
> I suggest to omit leading underscores from such identifiers.
> https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
> 
> Regards,
> Markus

Thanks for your suggestion, does this look better?
#ifndef MFD_PHOTONICAT_PMU_H
#define MFD_PHOTONICAT_PMU_H

Best regards,
Junhao

Re: [PATCH 1/9] mfd: Add driver for Photonicat power management MCU

[Krzysztof Kozlowski]

On 07/09/2024 16:33, Junhao Xie wrote:
> On 2024/9/7 16:44, Markus Elfring wrote:
>> …
>>> +++ b/include/linux/mfd/photonicat-pmu.h
>>> @@ -0,0 +1,86 @@
>> …
>>> +#ifndef _PHOTONICAT_PMU_H
>>> +#define _PHOTONICAT_PMU_H
>> …
>>
>> I suggest to omit leading underscores from such identifiers.
>> https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
>>
>> Regards,
>> Markus
> 
> Thanks for your suggestion, does this look better?
> #ifndef MFD_PHOTONICAT_PMU_H
> #define MFD_PHOTONICAT_PMU_H

<form letter>
Feel free to ignore all comments from Markus, regardless whether the
suggestion is reasonable or not. This person is banned from LKML and
several maintainers ignore Markus' feedback, because it is just a waste
of time.
</form letter>

Best regards,
Krzysztof

Re: [PATCH 1/9] mfd: Add driver for Photonicat power management MCU

[Lee Jones]

On Sun, 08 Sep 2024, Krzysztof Kozlowski wrote:

> On 07/09/2024 16:33, Junhao Xie wrote:
> > On 2024/9/7 16:44, Markus Elfring wrote:
> >> …
> >>> +++ b/include/linux/mfd/photonicat-pmu.h
> >>> @@ -0,0 +1,86 @@
> >> …
> >>> +#ifndef _PHOTONICAT_PMU_H
> >>> +#define _PHOTONICAT_PMU_H
> >> …
> >>
> >> I suggest to omit leading underscores from such identifiers.
> >> https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
> >>
> >> Regards,
> >> Markus
> > 
> > Thanks for your suggestion, does this look better?
> > #ifndef MFD_PHOTONICAT_PMU_H
> > #define MFD_PHOTONICAT_PMU_H

Yes, this is better.

> <form letter>
> Feel free to ignore all comments from Markus, regardless whether the
> suggestion is reasonable or not. This person is banned from LKML and
> several maintainers ignore Markus' feedback, because it is just a waste
> of time.
> </form letter>

If you really _must_ do this, at least keep it factual.

To the best of my knowledge Markus is not banned from LKML.

-- 
Lee Jones [李琼斯]

[PATCH 2/9] power: reset: add Photonicat PMU poweroff driver

[Junhao Xie]

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
+	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");
+	orderly_poweroff(true);
+
+	return NOTIFY_DONE;
+}
+
+static int pcat_poweroff_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct pcat_poweroff *poweroff;
+
+	poweroff = devm_kzalloc(dev, sizeof(*poweroff), GFP_KERNEL);
+	if (!poweroff)
+		return -ENOMEM;
+
+	poweroff->dev = dev;
+	poweroff->pmu = dev_get_drvdata(dev->parent);
+	poweroff->nb.notifier_call = pcat_poweroff_notify;
+	platform_set_drvdata(pdev, poweroff);
+
+	ret = devm_register_sys_off_handler(&pdev->dev,
+					    SYS_OFF_MODE_POWER_OFF,
+					    SYS_OFF_PRIO_DEFAULT,
+					    pcat_do_poweroff,
+					    poweroff);
+	if (ret)
+		return ret;
+
+	return pcat_pmu_register_notify(poweroff->pmu, &poweroff->nb);
+}
+
+static void pcat_poweroff_remove(struct platform_device *pdev)
+{
+	struct pcat_poweroff *poweroff = platform_get_drvdata(pdev);
+
+	pcat_pmu_unregister_notify(poweroff->pmu, &poweroff->nb);
+}
+
+static const struct of_device_id pcat_poweroff_dt_ids[] = {
+	{ .compatible = "ariaboard,photonicat-pmu-poweroff", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pcat_poweroff_dt_ids);
+
+static struct platform_driver pcat_poweroff_driver = {
+	.driver = {
+		.name = "photonicat-poweroff",
+		.of_match_table = pcat_poweroff_dt_ids,
+	},
+	.probe = pcat_poweroff_probe,
+	.remove = pcat_poweroff_remove,
+};
+module_platform_driver(pcat_poweroff_driver);
+
+MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>");
+MODULE_DESCRIPTION("Photonicat PMU Poweroff");
+MODULE_LICENSE("GPL");
-- 
2.46.0

Re: [PATCH 2/9] power: reset: add Photonicat PMU poweroff driver

[Krzysztof Kozlowski]

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

Re: [PATCH 2/9] power: reset: add Photonicat PMU poweroff driver

[Junhao Xie]

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

[PATCH 3/9] watchdog: Add Photonicat PMU watchdog driver

[Junhao Xie]

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;
+	struct pcat_pmu *pmu;
+	struct watchdog_device wdd;
+	u8 timeout;
+	bool started;
+};
+
+static const struct watchdog_info pcat_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.identity = "Photonicat PMU Watchdog",
+};
+
+static int pcat_wdt_setup(struct pcat_watchdog *data, int timeout)
+{
+	int ret;
+	u8 time = 0;
+	u8 times[3] = { 60, 60, 0 };
+
+	time = MIN(255, MAX(0, timeout));
+
+	ret = pcat_pmu_write_data(data->pmu, PCAT_CMD_WATCHDOG_TIMEOUT_SET,
+				  times, sizeof(times));
+	if (!ret)
+		data->started = timeout != 0;
+
+	return ret;
+}
+
+static int pcat_wdt_start(struct watchdog_device *wdev)
+{
+	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
+
+	return pcat_wdt_setup(data, data->timeout);
+}
+
+static int pcat_wdt_stop(struct watchdog_device *wdev)
+{
+	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
+
+	return pcat_wdt_setup(data, 0);
+}
+
+static int pcat_wdt_ping(struct watchdog_device *wdev)
+{
+	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
+
+	return pcat_pmu_send(data->pmu, PCAT_CMD_HEARTBEAT, NULL, 0);
+}
+
+static int pcat_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val)
+{
+	int ret = 0;
+	struct pcat_watchdog *data = watchdog_get_drvdata(wdev);
+
+	data->timeout = val;
+	if (data->started)
+		ret = pcat_wdt_setup(data, data->timeout);
+
+	return ret;
+}
+
+static const struct watchdog_ops pcat_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = pcat_wdt_start,
+	.stop = pcat_wdt_stop,
+	.ping = pcat_wdt_ping,
+	.set_timeout = pcat_wdt_set_timeout,
+};
+
+static int pcat_watchdog_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pcat_watchdog *watchdog;
+
+	watchdog = devm_kzalloc(dev, sizeof(*watchdog), GFP_KERNEL);
+	if (!watchdog)
+		return -ENOMEM;
+
+	watchdog->dev = dev;
+	watchdog->pmu = dev_get_drvdata(dev->parent);
+	watchdog->wdd.info = &pcat_wdt_info;
+	watchdog->wdd.ops = &pcat_wdt_ops;
+	watchdog->wdd.timeout = 60;
+	watchdog->wdd.max_timeout = U8_MAX;
+	watchdog->wdd.min_timeout = 0;
+	watchdog->wdd.parent = dev;
+
+	watchdog_stop_on_reboot(&watchdog->wdd);
+	watchdog_set_drvdata(&watchdog->wdd, watchdog);
+	platform_set_drvdata(pdev, watchdog);
+
+	return devm_watchdog_register_device(dev, &watchdog->wdd);
+}
+
+static const struct of_device_id pcat_watchdog_dt_ids[] = {
+	{ .compatible = "ariaboard,photonicat-pmu-watchdog", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pcat_watchdog_dt_ids);
+
+static struct platform_driver pcat_watchdog_driver = {
+	.driver = {
+		.name = "photonicat-watchdog",
+		.of_match_table = pcat_watchdog_dt_ids,
+	},
+	.probe = pcat_watchdog_probe,
+};
+module_platform_driver(pcat_watchdog_driver);
+
+MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>");
+MODULE_DESCRIPTION("Photonicat PMU watchdog");
+MODULE_LICENSE("GPL");
-- 
2.46.0

Re: [PATCH 3/9] watchdog: Add Photonicat PMU watchdog driver

[Guenter Roeck]

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");

Re: [PATCH 3/9] watchdog: Add Photonicat PMU watchdog driver

[Junhao Xie]

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

[PATCH 4/9] power: supply: photonicat-supply: Add Photonicat PMU battery and charger

[Junhao Xie]

Photonicat PMU supports battery and charger power supply.
The MCU only provides voltage meter.

Signed-off-by: Junhao Xie <bigfoot@classfun.cn>
---
 drivers/power/supply/Kconfig             |  12 ++
 drivers/power/supply/Makefile            |   1 +
 drivers/power/supply/photonicat-supply.c | 250 +++++++++++++++++++++++
 3 files changed, 263 insertions(+)
 create mode 100644 drivers/power/supply/photonicat-supply.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index bcfa63fb9f1e..4d2fcf568810 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -455,6 +455,18 @@ config CHARGER_PCF50633
 	help
 	  Say Y to include support for NXP PCF50633 Main Battery Charger.
 
+config PHOTONICAT_POWER
+	tristate "Photonicat PMU power supply driver"
+	depends on MFD_PHOTONICAT_PMU
+	help
+	  Photonicat PMU supports battery and charger power supply.
+	  The MCU only provides voltage meter.
+
+	  Say Y here to enable support for Photonicat PMU power supply.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called photonicat-supply.
+
 config BATTERY_RX51
 	tristate "Nokia RX-51 (N900) battery driver"
 	depends on TWL4030_MADC
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 8dcb41545317..81ada9bb6438 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_CHARGER_RT9471)	+= rt9471.o
 obj-$(CONFIG_BATTERY_TWL4030_MADC)	+= twl4030_madc_battery.o
 obj-$(CONFIG_CHARGER_88PM860X)	+= 88pm860x_charger.o
 obj-$(CONFIG_CHARGER_PCF50633)	+= pcf50633-charger.o
+obj-$(CONFIG_PHOTONICAT_POWER)	+= photonicat-supply.o
 obj-$(CONFIG_BATTERY_RX51)	+= rx51_battery.o
 obj-$(CONFIG_AB8500_BM)		+= ab8500_bmdata.o ab8500_charger.o ab8500_fg.o ab8500_btemp.o ab8500_chargalg.o
 obj-$(CONFIG_CHARGER_CPCAP)	+= cpcap-charger.o
diff --git a/drivers/power/supply/photonicat-supply.c b/drivers/power/supply/photonicat-supply.c
new file mode 100644
index 000000000000..e6861a3904fe
--- /dev/null
+++ b/drivers/power/supply/photonicat-supply.c
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Junhao Xie <bigfoot@classfun.cn>
+ */
+
+#include <linux/module.h>
+#include <linux/mfd/photonicat-pmu.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+
+enum pcat_supply_type {
+	PCAT_SUPPLY_BATTERY,
+	PCAT_SUPPLY_CHARGER,
+};
+
+struct pcat_supply {
+	struct device *dev;
+	struct pcat_pmu *pmu;
+	struct notifier_block nb;
+	struct power_supply *psy;
+	struct power_supply_desc desc;
+	struct power_supply_battery_info *bat_info;
+	enum pcat_supply_type type;
+	u16 supply_microvolt;
+	struct completion initial_report;
+};
+
+static int pcat_pmu_get_capacity(struct pcat_supply *supply)
+{
+	int uv;
+
+	if (supply->type != PCAT_SUPPLY_BATTERY)
+		return 0;
+	uv = supply->supply_microvolt * 1000;
+
+	return power_supply_batinfo_ocv2cap(supply->bat_info, uv, 20);
+}
+
+static int pcat_pmu_get_energy(struct pcat_supply *supply)
+{
+	int capacity;
+
+	if (supply->type != PCAT_SUPPLY_BATTERY)
+		return 0;
+	capacity = pcat_pmu_get_capacity(supply);
+	if (capacity < 0)
+		return 0;
+
+	return supply->bat_info->energy_full_design_uwh / 100 * capacity;
+}
+
+static int pcat_pmu_get_status(struct pcat_supply *supply)
+{
+	if (supply->type != PCAT_SUPPLY_BATTERY)
+		return 0;
+
+	if (pcat_pmu_get_capacity(supply) < 100) {
+		if (power_supply_am_i_supplied(supply->psy))
+			return POWER_SUPPLY_STATUS_CHARGING;
+		else
+			return POWER_SUPPLY_STATUS_DISCHARGING;
+	}
+
+	return POWER_SUPPLY_STATUS_FULL;
+}
+
+static int pcat_pmu_get_supply_property(struct power_supply *psy,
+					enum power_supply_property psp,
+					union power_supply_propval *val)
+{
+	struct pcat_supply *supply = power_supply_get_drvdata(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = pcat_pmu_get_capacity(supply);
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = supply->supply_microvolt > 1000;
+		break;
+	case POWER_SUPPLY_PROP_ENERGY_FULL:
+		val->intval = supply->bat_info->energy_full_design_uwh;
+		break;
+	case POWER_SUPPLY_PROP_ENERGY_NOW:
+		val->intval = pcat_pmu_get_energy(supply);
+		break;
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = pcat_pmu_get_status(supply);
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX:
+		val->intval = supply->bat_info->voltage_max_design_uv;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN:
+		val->intval = supply->bat_info->voltage_min_design_uv;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = supply->supply_microvolt * 1000;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static enum power_supply_property pcat_charger_props[] = {
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_ONLINE,
+};
+
+static enum power_supply_property pcat_battery_props[] = {
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_ENERGY_FULL,
+	POWER_SUPPLY_PROP_ENERGY_NOW,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+};
+
+static int pcat_supply_notify(struct notifier_block *nb, unsigned long action,
+			      void *data)
+{
+	struct pcat_supply *supply = container_of(nb, struct pcat_supply, nb);
+	struct pcat_data_cmd_status *status = pcat_data_get_data(data);
+
+	if (action != PCAT_CMD_STATUS_REPORT)
+		return NOTIFY_DONE;
+
+	switch (supply->type) {
+	case PCAT_SUPPLY_BATTERY:
+		supply->supply_microvolt = status->battery_microvolt;
+		break;
+	case PCAT_SUPPLY_CHARGER:
+		supply->supply_microvolt = status->charger_microvolt;
+		break;
+	}
+
+	complete(&supply->initial_report);
+
+	return NOTIFY_DONE;
+}
+
+static int pcat_supply_probe(struct platform_device *pdev)
+{
+	int ret;
+	const char *label;
+	const char *supply_type;
+	struct device *dev = &pdev->dev;
+	struct pcat_supply *supply;
+	struct power_supply_config psy_cfg = {};
+
+	supply = devm_kzalloc(dev, sizeof(*supply), GFP_KERNEL);
+	if (!supply)
+		return -ENOMEM;
+
+	supply->dev = dev;
+	supply->pmu = dev_get_drvdata(dev->parent);
+	supply->nb.notifier_call = pcat_supply_notify;
+	init_completion(&supply->initial_report);
+	psy_cfg.drv_data = supply;
+	psy_cfg.of_node = dev->of_node;
+	platform_set_drvdata(pdev, supply);
+
+	ret = of_property_read_string(dev->of_node, "type", &supply_type);
+	if (ret)
+		return dev_err_probe(dev, ret, "No supply type property\n");
+
+	ret = of_property_read_string(dev->of_node, "label", &label);
+	if (ret)
+		return dev_err_probe(dev, ret, "No label property\n");
+
+	if (!strcmp(supply_type, "battery")) {
+		supply->type = PCAT_SUPPLY_BATTERY;
+		supply->desc.type = POWER_SUPPLY_TYPE_BATTERY;
+		supply->desc.properties = pcat_battery_props;
+		supply->desc.num_properties = ARRAY_SIZE(pcat_battery_props);
+	} else if (!strcmp(supply_type, "charger")) {
+		supply->type = PCAT_SUPPLY_CHARGER;
+		supply->desc.type = POWER_SUPPLY_TYPE_MAINS;
+		supply->desc.properties = pcat_charger_props;
+		supply->desc.num_properties = ARRAY_SIZE(pcat_charger_props);
+	} else
+		return dev_err_probe(dev, -EINVAL, "Unknown supply type %s\n",
+				     supply_type);
+
+	ret = pcat_pmu_register_notify(supply->pmu, &supply->nb);
+	if (ret)
+		return ret;
+
+	if (!wait_for_completion_timeout(&supply->initial_report,
+					 msecs_to_jiffies(3000))) {
+		ret = dev_err_probe(dev, -ETIMEDOUT,
+				    "timeout waiting for initial report\n");
+		goto error;
+	}
+
+	dev_info(dev, "Voltage: %u mV\n", supply->supply_microvolt);
+
+	supply->desc.name = label;
+	supply->desc.get_property = pcat_pmu_get_supply_property;
+
+	supply->psy = devm_power_supply_register(dev, &supply->desc, &psy_cfg);
+	if (IS_ERR(supply->psy)) {
+		ret = PTR_ERR(supply->psy);
+		dev_err_probe(dev, ret, "Failed to register supply\n");
+		goto error;
+	}
+
+	if (supply->type == PCAT_SUPPLY_BATTERY) {
+		ret = power_supply_get_battery_info(supply->psy,
+						    &supply->bat_info);
+		if (ret) {
+			dev_err_probe(dev, ret, "Unable to get battery info\n");
+			goto error;
+		}
+	}
+
+	return 0;
+error:
+	pcat_pmu_unregister_notify(supply->pmu, &supply->nb);
+	return ret;
+}
+
+static void pcat_supply_remove(struct platform_device *pdev)
+{
+	struct pcat_supply *supply = platform_get_drvdata(pdev);
+
+	pcat_pmu_unregister_notify(supply->pmu, &supply->nb);
+}
+
+static const struct of_device_id pcat_supply_dt_ids[] = {
+	{ .compatible = "ariaboard,photonicat-pmu-supply", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pcat_supply_dt_ids);
+
+static struct platform_driver pcat_supply_driver = {
+	.driver = {
+		.name = "photonicat-supply",
+		.of_match_table = pcat_supply_dt_ids,
+	},
+	.probe = pcat_supply_probe,
+	.remove = pcat_supply_remove,
+};
+module_platform_driver(pcat_supply_driver);
+
+MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>");
+MODULE_DESCRIPTION("Photonicat PMU Power Supply");
+MODULE_LICENSE("GPL");
-- 
2.46.0

[PATCH 5/9] rtc: Add Photonicat PMU real-time clock

[Junhao Xie]

Photonicat PMU MCU will send status reports regularly,
including the MCU RTC date time.

Use command 0x09 to update the RTC date time of MCU.

Signed-off-by: Junhao Xie <bigfoot@classfun.cn>
---
 drivers/rtc/Kconfig          |  12 +++
 drivers/rtc/Makefile         |   1 +
 drivers/rtc/rtc-photonicat.c | 190 +++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 drivers/rtc/rtc-photonicat.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 2a95b05982ad..3403ba3f5643 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -516,6 +516,18 @@ config RTC_DRV_PCF8583
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-pcf8583.
 
+config RTC_DRV_PHOTONICAT_PMU
+	tristate "Photonicat PMU RTC"
+	depends on MFD_PHOTONICAT_PMU
+	help
+	  Photonicat PMU MCU will send status reports regularly,
+	  including the MCU RTC date time.
+
+	  If you say yes here you get support for the Photonicat PMU RTC.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-photonicat.
+
 config RTC_DRV_M41T80
 	tristate "ST M41T62/65/M41T80/81/82/83/84/85/87 and compatible"
 	help
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 3004e372f25f..f050a134da1f 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -129,6 +129,7 @@ obj-$(CONFIG_RTC_DRV_PCF8523)	+= rtc-pcf8523.o
 obj-$(CONFIG_RTC_DRV_PCF85363)	+= rtc-pcf85363.o
 obj-$(CONFIG_RTC_DRV_PCF8563)	+= rtc-pcf8563.o
 obj-$(CONFIG_RTC_DRV_PCF8583)	+= rtc-pcf8583.o
+obj-$(CONFIG_RTC_DRV_PHOTONICAT_PMU)	+= rtc-photonicat.o
 obj-$(CONFIG_RTC_DRV_PIC32)	+= rtc-pic32.o
 obj-$(CONFIG_RTC_DRV_PL030)	+= rtc-pl030.o
 obj-$(CONFIG_RTC_DRV_PL031)	+= rtc-pl031.o
diff --git a/drivers/rtc/rtc-photonicat.c b/drivers/rtc/rtc-photonicat.c
new file mode 100644
index 000000000000..2e3115fb5c38
--- /dev/null
+++ b/drivers/rtc/rtc-photonicat.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Junhao Xie <bigfoot@classfun.cn>
+ */
+
+#include <linux/kernel.h>
+#include <linux/mfd/photonicat-pmu.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+struct pcat_data_cmd_startup {
+	struct pcat_data_cmd_date_time time;
+	union {
+		u8 value;
+		struct {
+			bool year : 1;
+			bool month : 1;
+			bool day : 1;
+			bool hour : 1;
+			bool minute : 1;
+			bool second : 1;
+		} __packed;
+	} match;
+} __packed;
+
+struct pcat_rtc {
+	struct device *dev;
+	struct pcat_pmu *pmu;
+	struct notifier_block nb;
+	struct rtc_device *rtc;
+	struct pcat_data_cmd_date_time time;
+	struct completion initial_report;
+};
+
+static bool pcat_time_to_rtc_time(const struct pcat_data_cmd_date_time *time,
+				  struct rtc_time *tm)
+{
+	if (time->second >= 60 || time->minute >= 60 || time->hour >= 24 ||
+	    time->day <= 0 || time->day > 31 || time->month <= 0 ||
+	    time->month > 12 || time->year < 1900 || time->year > 9999)
+		return false;
+
+	tm->tm_sec = time->second;
+	tm->tm_min = time->minute;
+	tm->tm_hour = time->hour;
+	tm->tm_mday = time->day;
+	tm->tm_mon = time->month - 1;
+	tm->tm_year = time->year - 1900;
+	tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, time->year);
+	tm->tm_wday = ((time->year * (365 % 7)) + ((time->year - 1) / 4) -
+		       ((time->year - 1) / 100) + ((time->year - 1) / 400) +
+		       tm->tm_yday) % 7;
+
+	return true;
+}
+
+static void pcat_time_from_rtc_time(struct pcat_data_cmd_date_time *time,
+				    const struct rtc_time *tm)
+{
+	time->year = tm->tm_year + 1900;
+	time->month = tm->tm_mon + 1;
+	time->day = tm->tm_mday;
+	time->hour = tm->tm_hour;
+	time->minute = tm->tm_min;
+	time->second = tm->tm_sec;
+}
+
+static int pcat_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct pcat_rtc *rtc = dev_get_drvdata(dev);
+
+	memset(tm, 0, sizeof(*tm));
+	if (!pcat_time_to_rtc_time(&rtc->time, tm))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int pcat_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	int ret;
+	struct pcat_rtc *rtc = dev_get_drvdata(dev);
+	struct pcat_data_cmd_date_time time;
+
+	pcat_time_from_rtc_time(&time, tm);
+
+	ret = pcat_pmu_write_data(rtc->pmu, PCAT_CMD_DATE_TIME_SYNC,
+				  &time, sizeof(time));
+	if (ret)
+		return ret;
+
+	memcpy(&rtc->time, &time, sizeof(rtc->time));
+
+	return 0;
+}
+
+static const struct rtc_class_ops pcat_rtc_ops = {
+	.read_time = pcat_rtc_read_time,
+	.set_time = pcat_rtc_set_time,
+};
+
+static int pcat_rtc_notify(struct notifier_block *nb, unsigned long action,
+			   void *data)
+{
+	struct pcat_rtc *rtc = container_of(nb, struct pcat_rtc, nb);
+	struct pcat_data_cmd_status *status = pcat_data_get_data(data);
+
+	if (action != PCAT_CMD_STATUS_REPORT)
+		return NOTIFY_DONE;
+
+	memcpy(&rtc->time, &status->time, sizeof(rtc->time));
+	complete(&rtc->initial_report);
+
+	return NOTIFY_DONE;
+}
+
+static int pcat_rtc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pcat_rtc *rtc;
+	int ret;
+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	rtc->dev = dev;
+	rtc->pmu = dev_get_drvdata(dev->parent);
+	rtc->nb.notifier_call = pcat_rtc_notify;
+	init_completion(&rtc->initial_report);
+	platform_set_drvdata(pdev, rtc);
+
+	ret = pcat_pmu_register_notify(rtc->pmu, &rtc->nb);
+	if (ret)
+		return ret;
+
+	if (!wait_for_completion_timeout(&rtc->initial_report,
+					 msecs_to_jiffies(3000))) {
+		ret = dev_err_probe(dev, -ETIMEDOUT,
+				    "timeout waiting for initial report\n");
+		goto error;
+	}
+
+	dev_info(dev, "RTC Time: %04d/%02d/%02d %02d:%02d:%02d\n",
+		 rtc->time.year, rtc->time.month, rtc->time.day, rtc->time.hour,
+		 rtc->time.minute, rtc->time.second);
+
+	rtc->rtc = devm_rtc_device_register(&pdev->dev, "pcat-rtc",
+					    &pcat_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc->rtc)) {
+		ret = PTR_ERR(rtc->rtc);
+		dev_err_probe(&pdev->dev, ret,
+			      "Failed to register RTC device\n");
+		goto error;
+	}
+
+	return 0;
+error:
+	pcat_pmu_unregister_notify(rtc->pmu, &rtc->nb);
+	return ret;
+}
+
+static void pcat_rtc_remove(struct platform_device *pdev)
+{
+	struct pcat_rtc *rtc = platform_get_drvdata(pdev);
+
+	pcat_pmu_unregister_notify(rtc->pmu, &rtc->nb);
+}
+
+static const struct of_device_id pcat_rtc_dt_ids[] = {
+	{ .compatible = "ariaboard,photonicat-pmu-rtc", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pcat_rtc_dt_ids);
+
+static struct platform_driver pcat_rtc_driver = {
+	.driver = {
+		.name = "rtc-photonicat",
+		.of_match_table = pcat_rtc_dt_ids,
+	},
+	.probe = pcat_rtc_probe,
+	.remove = pcat_rtc_remove,
+};
+module_platform_driver(pcat_rtc_driver);
+
+MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>");
+MODULE_DESCRIPTION("Photonicat PMU RTC");
+MODULE_LICENSE("GPL");
-- 
2.46.0

[PATCH 6/9] hwmon: Add support for Photonicat PMU board temperature sensor

[Junhao Xie]

Photonicat PMU MCU will send status reports regularly,
including board temperature.

Signed-off-by: Junhao Xie <bigfoot@classfun.cn>
---
 drivers/hwmon/Kconfig            |  10 +++
 drivers/hwmon/Makefile           |   1 +
 drivers/hwmon/photonicat-hwmon.c | 129 +++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+)
 create mode 100644 drivers/hwmon/photonicat-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index b60fe2e58ad6..b345e4856bcf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1781,6 +1781,16 @@ config SENSORS_PT5161L
 	  This driver can also be built as a module. If so, the module
 	  will be called pt5161l.
 
+config SENSORS_PHOTONICAT_PMU_HWMON
+	tristate "Photonicat PMU temperature sensor"
+	depends on MFD_PHOTONICAT_PMU
+	help
+	  If you say yes here you get support for temperature sensor on the
+	  Photonicat PMU.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called photonicat-hwmon.
+
 config SENSORS_PWM_FAN
 	tristate "PWM fan"
 	depends on PWM || COMPILE_TEST
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b1c7056c37db..a30658acb093 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -183,6 +183,7 @@ obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
+obj-$(CONFIG_SENSORS_PHOTONICAT_PMU_HWMON)	+= photonicat-hwmon.o
 obj-$(CONFIG_SENSORS_POWERZ)	+= powerz.o
 obj-$(CONFIG_SENSORS_POWR1220)  += powr1220.o
 obj-$(CONFIG_SENSORS_PT5161L)	+= pt5161l.o
diff --git a/drivers/hwmon/photonicat-hwmon.c b/drivers/hwmon/photonicat-hwmon.c
new file mode 100644
index 000000000000..f1e3ee5f5781
--- /dev/null
+++ b/drivers/hwmon/photonicat-hwmon.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024 Junhao Xie <bigfoot@classfun.cn>
+ */
+
+#include <linux/completion.h>
+#include <linux/hwmon.h>
+#include <linux/mfd/photonicat-pmu.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct pcat_hwmon {
+	struct device *dev;
+	struct pcat_pmu *pmu;
+	struct notifier_block nb;
+	struct device *hwmon;
+	int temperature;
+	struct completion initial_report;
+};
+
+static ssize_t temp1_input_show(struct device *dev,
+				struct device_attribute *devattr, char *buf)
+{
+	struct pcat_hwmon *hwmon = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", hwmon->temperature * 1000);
+}
+
+static DEVICE_ATTR_RO(temp1_input);
+
+static struct attribute *pcat_pmu_temp_attrs[] = {
+	&dev_attr_temp1_input.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(pcat_pmu_temp);
+
+static int pcat_hwmon_notify(struct notifier_block *nb, unsigned long action,
+			     void *data)
+{
+	struct pcat_hwmon *hwmon = container_of(nb, struct pcat_hwmon, nb);
+	struct pcat_data_cmd_status *status = pcat_data_get_data(data);
+
+	if (action != PCAT_CMD_STATUS_REPORT)
+		return NOTIFY_DONE;
+
+	hwmon->temperature = status->temp - 40;
+	complete(&hwmon->initial_report);
+
+	return NOTIFY_DONE;
+}
+
+static int pcat_hwmon_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct pcat_hwmon *hwmon;
+	const char *label;
+
+	hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+	if (!hwmon)
+		return -ENOMEM;
+
+	hwmon->dev = dev;
+	hwmon->pmu = dev_get_drvdata(dev->parent);
+	hwmon->nb.notifier_call = pcat_hwmon_notify;
+	init_completion(&hwmon->initial_report);
+	platform_set_drvdata(pdev, hwmon);
+
+	ret = of_property_read_string(dev->of_node, "label", &label);
+	if (ret)
+		return dev_err_probe(dev, ret, "No label property\n");
+
+	ret = pcat_pmu_register_notify(hwmon->pmu, &hwmon->nb);
+	if (ret)
+		return ret;
+
+	if (!wait_for_completion_timeout(&hwmon->initial_report,
+					 msecs_to_jiffies(3000))) {
+		ret = dev_err_probe(dev, -ETIMEDOUT,
+				    "timeout waiting for initial report\n");
+		goto error;
+	}
+
+	dev_info(dev, "Board Temprature: %d degress C\n", hwmon->temperature);
+
+	hwmon->hwmon = devm_hwmon_device_register_with_groups(
+		dev, label, hwmon, pcat_pmu_temp_groups);
+
+	if (IS_ERR(hwmon->hwmon)) {
+		ret = PTR_ERR(hwmon->hwmon);
+		dev_err_probe(&pdev->dev, ret,
+			      "Failed to register hwmon device\n");
+		goto error;
+	}
+
+	return 0;
+error:
+	pcat_pmu_unregister_notify(hwmon->pmu, &hwmon->nb);
+	return ret;
+}
+
+static void pcat_hwmon_remove(struct platform_device *pdev)
+{
+	struct pcat_hwmon *hwmon = platform_get_drvdata(pdev);
+
+	pcat_pmu_unregister_notify(hwmon->pmu, &hwmon->nb);
+}
+
+static const struct of_device_id pcat_hwmon_dt_ids[] = {
+	{ .compatible = "ariaboard,photonicat-pmu-hwmon", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, pcat_hwmon_dt_ids);
+
+static struct platform_driver pcat_hwmon_driver = {
+	.driver = {
+		.name = "photonicat-hwmon",
+		.of_match_table = pcat_hwmon_dt_ids,
+	},
+	.probe = pcat_hwmon_probe,
+	.remove = pcat_hwmon_remove,
+};
+module_platform_driver(pcat_hwmon_driver);
+
+MODULE_AUTHOR("Junhao Xie <bigfoot@classfun.cn>");
+MODULE_DESCRIPTION("Photonicat PMU Hardware Monitor");
+MODULE_LICENSE("GPL");
-- 
2.46.0

Re: [PATCH 6/9] hwmon: Add support for Photonicat PMU board temperature sensor

[Guenter Roeck]

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

Re: [PATCH 6/9] hwmon: Add support for Photonicat PMU board temperature sensor

[Junhao Xie]

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

Re: [PATCH 6/9] hwmon: Add support for Photonicat PMU board temperature sensor

[Guenter Roeck]

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

[PATCH 7/9] leds: add Photonicat PMU LED driver

[Junhao Xie]

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,
+	  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>
+
+struct pcat_leds {
+	struct device *dev;
+	struct pcat_pmu *pmu;
+	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;
+	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;
+	leds->pmu = dev_get_drvdata(dev->parent);
+	platform_set_drvdata(pdev, leds);
+
+	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", },
+	{ /* 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

Re: [PATCH 7/9] leds: add Photonicat PMU LED driver

[Lee Jones]

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
> 

-- 
0)
Lee Jones [李琼斯]

Re: [PATCH 7/9] leds: add Photonicat PMU LED driver

[Junhao Xie]

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

[PATCH 8/9] dt-bindings: Add documentation for Photonicat PMU

[Junhao Xie]

Add device tree binding documentation for Photonicat PMU MFD, LEDs,
hardware monitor, power off, power supply, real-time clock watchdog.

Signed-off-by: Junhao Xie <bigfoot@classfun.cn>
---
 .../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 ++++++
 7 files changed, 351 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

diff --git a/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
new file mode 100644
index 000000000000..c9b1bab20c31
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ariaboard,photonicat-pmu-hwmon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Photonicat PMU Hardware Monitor
+
+maintainers:
+  - Junhao Xie <bigfoot@classfun.cn>
+
+description:
+  Board temperature sensor on the Photonicat PMU MCU
+
+properties:
+  compatible:
+    const: ariaboard,photonicat-pmu-hwmon
+
+  label:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: Label for hwmon device
+
+required:
+  - compatible
+  - label
+
+additionalProperties: false
+
+examples:
+  - |
+      serial {
+          mcu {
+              compatible = "ariaboard,photonicat-pmu";
+
+              hwmon {
+                  compatible = "ariaboard,photonicat-pmu-hwmon";
+                  label = "pcat_board";
+              };
+          };
+      };
diff --git a/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml b/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
new file mode 100644
index 000000000000..6ccb0e691b09
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/ariaboard,photonicat-pmu-leds.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Photonicat PMU LEDs
+
+maintainers:
+  - Junhao Xie <bigfoot@classfun.cn>
+
+description:
+  LEDs on the Photonicat PMU MCU
+
+allOf:
+  - $ref: common.yaml#
+
+properties:
+  compatible:
+    const: ariaboard,photonicat-pmu-leds
+
+  label: true
+
+required:
+  - compatible
+  - label
+
+additionalProperties: false
+
+examples:
+  - |
+      serial {
+          mcu {
+              compatible = "ariaboard,photonicat-pmu";
+
+              leds-status {
+                  compatible = "ariaboard,photonicat-pmu-leds";
+                  label = "net-status";
+              };
+          };
+      };
diff --git a/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml b/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
new file mode 100644
index 000000000000..df16d9507821
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/ariaboard,photonicat-pmu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ariaboard Photonicat PMU
+
+maintainers:
+  - Junhao Xie <bigfoot@classfun.cn>
+
+description:
+  Driver for the Power Management MCU in the Ariaboard Photonicat,
+  which provides battery and charger power supply, real-time clock,
+  watchdog, hardware shutdown.
+
+properties:
+  compatible:
+    const: ariaboard,photonicat-pmu
+
+  current-speed:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 115200
+    description: PMU Serial baudrate
+
+  local-address:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 127
+    default: 1
+    description: CPU board address
+
+  remote-address:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 127
+    default: 1
+    description: PMU board address
+
+  hwmon:
+    $ref: /schemas/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
+
+  poweroff:
+    $ref: /schemas/power/reset/ariaboard,photonicat-pmu-poweroff.yaml
+
+  rtc:
+    $ref: /schemas/rtc/ariaboard,photonicat-pmu-rtc.yaml
+
+  watchdog:
+    $ref: /schemas/watchdog/ariaboard,photonicat-pmu-watchdog.yaml
+
+patternProperties:
+  '^leds-(status)':
+    $ref: /schemas/leds/ariaboard,photonicat-pmu-leds.yaml
+
+  '^supply-(battery|charger)$':
+    $ref: /schemas/power/supply/ariaboard,photonicat-pmu-supply.yaml
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+      serial {
+          photonicat-pmu {
+              compatible = "ariaboard,photonicat-pmu";
+              current-speed = <115200>;
+              local-address = <1>;
+              remote-address = <1>;
+
+              supply-battery {
+                  compatible = "ariaboard,photonicat-pmu-supply";
+                  label = "battery";
+                  type = "battery";
+              };
+
+              supply-charger {
+                  compatible = "ariaboard,photonicat-pmu-supply";
+                  label = "charger";
+                  type = "charger";
+              };
+
+              hwmon {
+                  compatible = "ariaboard,photonicat-pmu-hwmon";
+                  label = "pcat_board";
+              };
+
+              leds-status {
+                  compatible = "ariaboard,photonicat-pmu-leds";
+                  label = "net-status";
+              };
+
+              poweroff {
+                  compatible = "ariaboard,photonicat-pmu-poweroff";
+              };
+
+              rtc {
+                  compatible = "ariaboard,photonicat-pmu-rtc";
+              };
+
+              watchdog {
+                  compatible = "ariaboard,photonicat-pmu-watchdog";
+              };
+          };
+      };
diff --git a/Documentation/devicetree/bindings/power/reset/ariaboard,photonicat-pmu-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/ariaboard,photonicat-pmu-poweroff.yaml
new file mode 100644
index 000000000000..cfb6c043aa8d
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/ariaboard,photonicat-pmu-poweroff.yaml
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/reset/ariaboard,photonicat-pmu-poweroff.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Photonicat PMU Real-time clock
+
+maintainers:
+  - Junhao Xie <bigfoot@classfun.cn>
+
+description:
+  Poweroff on the Photonicat PMU MCU
+
+properties:
+  compatible:
+    const: ariaboard,photonicat-pmu-poweroff
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+      serial {
+          mcu {
+              compatible = "ariaboard,photonicat-pmu";
+
+              poweroff {
+                  compatible = "ariaboard,photonicat-pmu-poweroff";
+              };
+          };
+      };
diff --git a/Documentation/devicetree/bindings/power/supply/ariaboard,photonicat-pmu-supply.yaml b/Documentation/devicetree/bindings/power/supply/ariaboard,photonicat-pmu-supply.yaml
new file mode 100644
index 000000000000..73b1c3564c58
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/ariaboard,photonicat-pmu-supply.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/ariaboard,photonicat-pmu-supply.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Photonicat PMU Power supply
+
+maintainers:
+  - Junhao Xie <bigfoot@classfun.cn>
+
+description:
+  Power supply on the Photonicat PMU MCU
+
+allOf:
+  - $ref: power-supply.yaml#
+
+properties:
+  compatible:
+    const: ariaboard,photonicat-pmu-supply
+
+  label:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: Label for power supply device
+
+  type:
+    $ref: /schemas/types.yaml#/definitions/string
+    enum:
+      - battery
+      - charger
+    description: Photonicat power supply type
+
+  power-supplies: true
+  monitored-battery: true
+
+required:
+  - compatible
+  - label
+  - type
+
+additionalProperties: false
+
+examples:
+  - |
+      serial {
+          mcu {
+              compatible = "ariaboard,photonicat-pmu";
+
+              supply-charger {
+                  compatible = "ariaboard,photonicat-pmu-supply";
+                  label = "charger";
+                  type = "charger";
+              };
+          };
+      };
diff --git a/Documentation/devicetree/bindings/rtc/ariaboard,photonicat-pmu-rtc.yaml b/Documentation/devicetree/bindings/rtc/ariaboard,photonicat-pmu-rtc.yaml
new file mode 100644
index 000000000000..05f3c3768dbf
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/ariaboard,photonicat-pmu-rtc.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/ariaboard,photonicat-pmu-rtc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Photonicat PMU Real-time clock
+
+maintainers:
+  - Junhao Xie <bigfoot@classfun.cn>
+
+description:
+  Real-time clock on the Photonicat PMU MCU
+
+allOf:
+  - $ref: rtc.yaml#
+
+properties:
+  compatible:
+    const: ariaboard,photonicat-pmu-rtc
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+      serial {
+          mcu {
+              compatible = "ariaboard,photonicat-pmu";
+
+              rtc {
+                  compatible = "ariaboard,photonicat-pmu-rtc";
+              };
+          };
+      };
diff --git a/Documentation/devicetree/bindings/watchdog/ariaboard,photonicat-pmu-watchdog.yaml b/Documentation/devicetree/bindings/watchdog/ariaboard,photonicat-pmu-watchdog.yaml
new file mode 100644
index 000000000000..91bc6adea9a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/ariaboard,photonicat-pmu-watchdog.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/ariaboard,photonicat-pmu-watchdog.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Photonicat PMU Watchdog
+
+maintainers:
+  - Junhao Xie <bigfoot@classfun.cn>
+
+description:
+  Watchdog on the Photonicat PMU MCU
+
+allOf:
+  - $ref: watchdog.yaml#
+
+properties:
+  compatible:
+    const: ariaboard,photonicat-pmu-watchdog
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+      serial {
+          mcu {
+              compatible = "ariaboard,photonicat-pmu";
+
+              watchdog {
+                  compatible = "ariaboard,photonicat-pmu-watchdog";
+              };
+          };
+      };
-- 
2.46.0

Re: [PATCH 8/9] dt-bindings: Add documentation for Photonicat PMU

[Krzysztof Kozlowski]

On 06/09/2024 11:36, Junhao Xie wrote:
> Add device tree binding documentation for Photonicat PMU MFD, LEDs,
> hardware monitor, power off, power supply, real-time clock watchdog.
> 
> Signed-off-by: Junhao Xie <bigfoot@classfun.cn>
> ---
>  .../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 ++++++
>  7 files changed, 351 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
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
> new file mode 100644
> index 000000000000..c9b1bab20c31
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/ariaboard,photonicat-pmu-hwmon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Photonicat PMU Hardware Monitor
> +
> +maintainers:
> +  - Junhao Xie <bigfoot@classfun.cn>
> +
> +description:
> +  Board temperature sensor on the Photonicat PMU MCU
> +
> +properties:
> +  compatible:
> +    const: ariaboard,photonicat-pmu-hwmon
> +
> +  label:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Label for hwmon device

No resources here. Fold it into parent binding.

> +
> +required:
> +  - compatible
> +  - label
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      serial {
> +          mcu {
> +              compatible = "ariaboard,photonicat-pmu";

Drop, no need.

> +

Messed indentation.

Entire example is redundant. Merge it to parent binding.


> +              hwmon {
> +                  compatible = "ariaboard,photonicat-pmu-hwmon";
> +                  label = "pcat_board";
> +              };
> +          };
> +      };
> diff --git a/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml b/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
> new file mode 100644
> index 000000000000..6ccb0e691b09
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/ariaboard,photonicat-pmu-leds.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Photonicat PMU LEDs
> +
> +maintainers:
> +  - Junhao Xie <bigfoot@classfun.cn>
> +
> +description:
> +  LEDs on the Photonicat PMU MCU
> +
> +allOf:
> +  - $ref: common.yaml#
> +
> +properties:
> +  compatible:
> +    const: ariaboard,photonicat-pmu-leds

Your compatibles per device do not make much sense. You organized
bindings per drivers, but that's not what we want.

> +
> +  label: true

Drop

> +
> +required:
> +  - compatible
> +  - label
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      serial {
> +          mcu {
> +              compatible = "ariaboard,photonicat-pmu";
> +
> +              leds-status {
> +                  compatible = "ariaboard,photonicat-pmu-leds";
> +                  label = "net-status";
> +              };
> +          };
> +      };
> diff --git a/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml b/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
> new file mode 100644
> index 000000000000..df16d9507821
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
> @@ -0,0 +1,107 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/ariaboard,photonicat-pmu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ariaboard Photonicat PMU
> +
> +maintainers:
> +  - Junhao Xie <bigfoot@classfun.cn>
> +
> +description:
> +  Driver for the Power Management MCU in the Ariaboard Photonicat,

Bindings are for hardware, not drivers. Drop it everywhere and explain
hardware.

> +  which provides battery and charger power supply, real-time clock,
> +  watchdog, hardware shutdown.
> +
> +properties:
> +  compatible:
> +    const: ariaboard,photonicat-pmu

That's the only compatible you should have. Drop all others.

> +
> +  current-speed:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 115200
> +    description: PMU Serial baudrate
> +
> +  local-address:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 127
> +    default: 1
> +    description: CPU board address

Address of what? In which notation? It's part of this hardware.


> +
> +  remote-address:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 127
> +    default: 1
> +    description: PMU board address

Eee, no. Your board knows its address. You do not have to tell it.

> +
> +  hwmon:
> +    $ref: /schemas/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
> +
> +  poweroff:
> +    $ref: /schemas/power/reset/ariaboard,photonicat-pmu-poweroff.yaml
> +
> +  rtc:
> +    $ref: /schemas/rtc/ariaboard,photonicat-pmu-rtc.yaml
> +
> +  watchdog:
> +    $ref: /schemas/watchdog/ariaboard,photonicat-pmu-watchdog.yaml
> +
> +patternProperties:
> +  '^leds-(status)':

That's not a pattern.

> +    $ref: /schemas/leds/ariaboard,photonicat-pmu-leds.yaml
> +
> +  '^supply-(battery|charger)$':
> +    $ref: /schemas/power/supply/ariaboard,photonicat-pmu-supply.yaml

Why two nodes?

> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      serial {
> +          photonicat-pmu {
> +              compatible = "ariaboard,photonicat-pmu";
> +              current-speed = <115200>;
> +              local-address = <1>;
> +              remote-address = <1>;
> +
> +              supply-battery {
> +                  compatible = "ariaboard,photonicat-pmu-supply";
> +                  label = "battery";

Nope, drop label.

> +                  type = "battery";

No, there is no type property.

Missing monitored battery.

> +              };
> +
> +              supply-charger {
> +                  compatible = "ariaboard,photonicat-pmu-supply";
> +                  label = "charger";
> +                  type = "charger";
> +              };
> +
> +              hwmon {
> +                  compatible = "ariaboard,photonicat-pmu-hwmon";
> +                  label = "pcat_board";
> +              };
> +
> +              leds-status {
> +                  compatible = "ariaboard,photonicat-pmu-leds";
> +                  label = "net-status";
> +              };
> +
> +              poweroff {
> +                  compatible = "ariaboard,photonicat-pmu-poweroff";
> +              };
> +
> +              rtc {
> +                  compatible = "ariaboard,photonicat-pmu-rtc";
> +              };
> +
> +              watchdog {
> +                  compatible = "ariaboard,photonicat-pmu-watchdog";
> +              };

These are seriously redundant and useless nodes.  There is nothing
beneficial from the nodes above - they are all empty, without resources.
Drop all of them.

I finish the review here.

Best regards,
Krzysztof

Re: [PATCH 8/9] dt-bindings: Add documentation for Photonicat PMU

[Junhao Xie]

On 2024/9/6 17:51, Krzysztof Kozlowski wrote:
> On 06/09/2024 11:36, Junhao Xie wrote:
>> Add device tree binding documentation for Photonicat PMU MFD, LEDs,
>> hardware monitor, power off, power supply, real-time clock watchdog.
[...]
>> diff --git a/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/ariaboard,photonicat-pmu-hwmon.yaml
[...]>> +  label:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: Label for hwmon device
> 
> No resources here. Fold it into parent binding.
> 
>> +
>> +required:
>> +  - compatible
>> +  - label
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +      serial {
>> +          mcu {
>> +              compatible = "ariaboard,photonicat-pmu";
> 
> Drop, no need.
> 
>> +
> 
> Messed indentation.
> 
> Entire example is redundant. Merge it to parent binding.
> 

I will fix them.

> 
[...]
>> diff --git a/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml b/Documentation/devicetree/bindings/leds/ariaboard,photonicat-pmu-leds.yaml
[...]
>> +properties:
>> +  compatible:
>> +    const: ariaboard,photonicat-pmu-leds
> 
> Your compatibles per device do not make much sense. You organized
> bindings per drivers, but that's not what we want.
> 
>> +
>> +  label: true
> 
> Drop

I will drop it.

> 
[...]
>> diff --git a/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml b/Documentation/devicetree/bindings/mfd/ariaboard,photonicat-pmu.yaml
[...]
>> +description:
>> +  Driver for the Power Management MCU in the Ariaboard Photonicat,
> 
> Bindings are for hardware, not drivers. Drop it everywhere and explain
> hardware.
> 

I will edit it.
Maybe the following looks better?
  The Power Management MCU in Ariaboard Photonicat, which
  provides battery and charger power supply real-time clock,
  watchdog, hardware shutdown, status LEDs.

>> +  which provides battery and charger power supply, real-time clock,
>> +  watchdog, hardware shutdown.
>> +
>> +properties:
>> +  compatible:
>> +    const: ariaboard,photonicat-pmu
> 
> That's the only compatible you should have. Drop all others.
> 
>> +
[...]
>> +  local-address:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 127
>> +    default: 1
>> +    description: CPU board address
> 
> Address of what? In which notation? It's part of this hardware.
> 

Photonicat's MCU protocol documentation says it supports multiple hosts.
But Photonicat only uses one.
Is it necessary to remove local-address and use a fixed address?

> 
>> +
>> +  remote-address:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 127
>> +    default: 1
>> +    description: PMU board address
> 
> Eee, no. Your board knows its address. You do not have to tell it.

I will remove remote-address.

> 
[...]
>> +
>> +patternProperties:
>> +  '^leds-(status)':
> 
> That's not a pattern.
> 

I originally wanted to keep it for extensions, but it didn't seem like a good idea.
I will move it to properties.

>> +    $ref: /schemas/leds/ariaboard,photonicat-pmu-leds.yaml
>> +
>> +  '^supply-(battery|charger)$':
>> +    $ref: /schemas/power/supply/ariaboard,photonicat-pmu-supply.yaml
> 
> Why two nodes?
> 
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +      serial {
>> +          photonicat-pmu {
>> +              compatible = "ariaboard,photonicat-pmu";
>> +              current-speed = <115200>;
>> +              local-address = <1>;
>> +              remote-address = <1>;
>> +
>> +              supply-battery {
>> +                  compatible = "ariaboard,photonicat-pmu-supply";
>> +                  label = "battery";
> 
> Nope, drop label.
> 
>> +                  type = "battery";
> 
> No, there is no type property.

There are two supplies here, one is the charger and the other is the battery.
Is it necessary to change to use different compatible ones like
ariaboard,photonicat-pmu-battery and ariaboard,photonicat-pmu-charger?

> 
> Missing monitored battery.
> 

I will add it.

>> +              };
>> +
[...]
>> +
>> +              watchdog {
>> +                  compatible = "ariaboard,photonicat-pmu-watchdog";
>> +              };
> 
> These are seriously redundant and useless nodes.  There is nothing
> beneficial from the nodes above - they are all empty, without resources.
> Drop all of them.

How should I describe these devices? Using mfd_cell?

> 
> I finish the review here.
> 
> Best regards,
> Krzysztof
> 

Thanks for your review, I will fix all problems in next version!

Best regards,
Junhao

Re: [PATCH 8/9] dt-bindings: Add documentation for Photonicat PMU

[Krzysztof Kozlowski]

On 07/09/2024 16:27, Junhao Xie wrote:

>>> +
> [...]
>>> +  local-address:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 1
>>> +    maximum: 127
>>> +    default: 1
>>> +    description: CPU board address
>>
>> Address of what? In which notation? It's part of this hardware.
>>
> 
> Photonicat's MCU protocol documentation says it supports multiple hosts.
> But Photonicat only uses one.
> Is it necessary to remove local-address and use a fixed address?

I don't understand what this "address" is for.

> 
>>
>>> +
>>> +  remote-address:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 1
>>> +    maximum: 127
>>> +    default: 1
>>> +    description: PMU board address
>>
>> Eee, no. Your board knows its address. You do not have to tell it.
> 
> I will remove remote-address.
> 
>>
> [...]
>>> +
>>> +patternProperties:
>>> +  '^leds-(status)':
>>
>> That's not a pattern.
>>
> 
> I originally wanted to keep it for extensions, but it didn't seem like a good idea.
> I will move it to properties.
> 
>>> +    $ref: /schemas/leds/ariaboard,photonicat-pmu-leds.yaml
>>> +
>>> +  '^supply-(battery|charger)$':
>>> +    $ref: /schemas/power/supply/ariaboard,photonicat-pmu-supply.yaml
>>
>> Why two nodes?
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +      serial {
>>> +          photonicat-pmu {
>>> +              compatible = "ariaboard,photonicat-pmu";
>>> +              current-speed = <115200>;
>>> +              local-address = <1>;
>>> +              remote-address = <1>;
>>> +
>>> +              supply-battery {
>>> +                  compatible = "ariaboard,photonicat-pmu-supply";
>>> +                  label = "battery";
>>
>> Nope, drop label.
>>
>>> +                  type = "battery";
>>
>> No, there is no type property.
> 
> There are two supplies here, one is the charger and the other is the battery.
> Is it necessary to change to use different compatible ones like
> ariaboard,photonicat-pmu-battery and ariaboard,photonicat-pmu-charger?

Are the devices different? Why do you even need the type?

> 
>>
>> Missing monitored battery.
>>
> 
> I will add it.
> 
>>> +              };
>>> +
> [...]
>>> +
>>> +              watchdog {
>>> +                  compatible = "ariaboard,photonicat-pmu-watchdog";
>>> +              };
>>
>> These are seriously redundant and useless nodes.  There is nothing
>> beneficial from the nodes above - they are all empty, without resources.
>> Drop all of them.
> 
> How should I describe these devices? Using mfd_cell?

You mean drivers? MFD or auxiliary bus.

Best regards,
Krzysztof

[PATCH 9/9] arm64: dts: rockchip: add Photonicat PMU support for Ariaboard Photonicat

[Junhao Xie]

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 {
+			compatible = "ariaboard,photonicat-pmu-supply";
+			label = "battery";
+			monitored-battery = <&battery>;
+			power-supplies = <&pcat_pmu_charger>;
+			type = "battery";
+		};
+
+		pcat_pmu_charger: supply-charger {
+			compatible = "ariaboard,photonicat-pmu-supply";
+			label = "charger";
+			type = "charger";
+		};
+
+		pcat_pmu_hwmon: hwmon {
+			compatible = "ariaboard,photonicat-pmu-hwmon";
+			label = "pcat_board";
+		};
+
+		pcat_pmu_leds_status: leds-status {
+			compatible = "ariaboard,photonicat-pmu-leds";
+			label = "net-status";
+		};
+
+		pcat_pmu_poweroff: poweroff {
+			compatible = "ariaboard,photonicat-pmu-poweroff";
+		};
+
+		pcat_pmu_rtc: rtc {
+			compatible = "ariaboard,photonicat-pmu-rtc";
+		};
+
+		pcat_pmu_watchdog: watchdog {
+			compatible = "ariaboard,photonicat-pmu-watchdog";
+		};
+	};
 };
 
 &usb_host0_xhci {
-- 
2.46.0

Re: [PATCH 9/9] arm64: dts: rockchip: add Photonicat PMU support for Ariaboard Photonicat

[Krzysztof Kozlowski]

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

Re: [PATCH 9/9] arm64: dts: rockchip: add Photonicat PMU support for Ariaboard Photonicat

[Junhao Xie]

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

Re: [PATCH 0/9] Introduce Photonicat power management MCU driver

[Krzysztof Kozlowski]

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

Re: [PATCH 0/9] Introduce Photonicat power management MCU driver

[Junhao Xie]

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