Re: [PATCH v4 05/10] accel/rocket: Add a new driver for Rockchip's NPU

[Krzysztof Kozlowski <krzk@kernel.org>]

On Mon, May 19, 2025 at 03:43:37PM GMT, Tomeu Vizoso wrote:
> +#endif
> diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..bb469ac87d36249157f4ba9d9f7106ad558309e4
> --- /dev/null
> +++ b/drivers/accel/rocket/rocket_device.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright 2024-2025 Tomeu Vizoso <tomeu@tomeuvizoso.net> */
> +
> +#include <linux/clk.h>
> +#include <linux/dev_printk.h>
> +
> +#include "rocket_device.h"
> +
> +int rocket_device_init(struct rocket_device *rdev)
> +{
> +	struct device *dev = rdev->cores[0].dev;
> +	int err;
> +
> +	rdev->clk_npu = devm_clk_get(dev, "npu");
> +	if (IS_ERR(rdev->clk_npu)) {
> +		err = PTR_ERR(rdev->clk_npu);
> +		dev_err(dev, "devm_clk_get failed %d for clock npu\n", err);
> +		return err;
> +	}

That's probe path? so use standard syntax:

return dev_err_probe(). One line instead of four.

> +
> +	rdev->pclk = devm_clk_get(dev, "pclk");
> +	if (IS_ERR(rdev->pclk)) {
> +		err = PTR_ERR(rdev->pclk);
> +		dev_err(dev, "devm_clk_get failed %d for clock pclk\n", err);
> +		return err;

Same here... except that this should be blk API and entire function gets
smaller.

> +	}
> +
> +	/* Initialize core 0 (top) */
> +	err = rocket_core_init(&rdev->cores[0]);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}

...

> +static int rocket_device_runtime_resume(struct device *dev)
> +{
> +	struct rocket_device *rdev = dev_get_drvdata(dev);
> +	int core = find_core_for_dev(dev);
> +	int err = 0;
> +
> +	if (core < 0)
> +		return -ENODEV;
> +
> +	if (core == 0) {
> +		err = clk_prepare_enable(rdev->clk_npu);
> +		if (err) {
> +			dev_err(dev, "clk_prepare_enable failed %d for clock npu\n", err);
> +			return err;
> +		}
> +
> +		err = clk_prepare_enable(rdev->pclk);
> +		if (err) {
> +			dev_err(dev, "clk_prepare_enable failed %d for clock pclk\n", err);
> +			goto error_clk_npu;
> +		}
> +	}
> +
> +	err = clk_prepare_enable(rdev->cores[core].a_clk);
> +	if (err) {
> +		dev_err(dev, "clk_prepare_enable failed %d for a_clk in core %d\n", err, core);
> +		goto error_pclk;
> +	}
> +
> +	err = clk_prepare_enable(rdev->cores[core].h_clk);
> +	if (err) {
> +		dev_err(dev, "clk_prepare_enable failed %d for h_clk in core %d\n", err, core);
> +		goto error_a_clk;
> +	}

All four above calls could be just one call with bulk API.

> +
> +	return 0;
> +
> +error_a_clk:
> +	clk_disable_unprepare(rdev->cores[core].a_clk);
> +
> +error_pclk:
> +	if (core == 0)
> +		clk_disable_unprepare(rdev->pclk);
> +
> +error_clk_npu:
> +	if (core == 0)
> +		clk_disable_unprepare(rdev->clk_npu);

And all this would be gone...

> +
> +	return err;

Best regards,
Krzysztof