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