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@xxxxxxxxxxx>
> ---
> 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
