Re: [PATCH v3 07/12] gpio: rockchip: support 'clock-names' from dt nodes

From: Andy Shevchenko
Date: Tue Sep 03 2024 - 11:59:22 EST


On Tue, Sep 03, 2024 at 03:36:44PM +0800, Ye Zhang wrote:
> Added support for retrieving clocks using 'clock-names' from dt nodes

...

> + bank->clk = devm_clk_get_enabled(dev, "bus");

I would use _optional() variant.

> + if (IS_ERR(bank->clk)) {
> + bank->clk = of_clk_get(dev->of_node, 0);
> + if (IS_ERR(bank->clk)) {
> + dev_err(dev, "fail to get apb clock\n");
> + return PTR_ERR(bank->clk);
> + }

> + clk_prepare_enable(bank->clk);

Why? How was it tested?

> + bank->manual_clk_release = true;
> + }

It seems you need to unnest the conditionals and think through the logic.

> + bank->db_clk = devm_clk_get_enabled(dev, "db");
> + if (IS_ERR(bank->db_clk)) {
> + bank->db_clk = of_clk_get(dev->of_node, 1);
> + if (IS_ERR(bank->db_clk)) {
> + bank->db_clk = NULL;
> + } else {
> + clk_prepare_enable(bank->db_clk);

Ditto.

> + bank->manual_dbclk_release = true;
> + }
> + }

...

> ret = rockchip_get_bank_data(bank);
> if (ret)
> - return ret;
> + goto err_disabled_clk;

No, just make sure it's properly wrapped by devm.

...

> + bool manual_clk_release;
> + bool manual_dbclk_release;

Why do you need them?

--
With Best Regards,
Andy Shevchenko