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 <redacted>
---
 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