Re: [PATCH 6/6] gpio: rockchip: support acpi

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Fri, Sep 09, 2022 at 05:05:58PM +0800, Jianqun Xu wrote:
> The gpio driver for rockchip gpio controller is seperated from rockchip
> pinctrl driver, at the first version, it keeps things original to make
> the patch easy to be reviewed, such as the gpio driver must work with
> the pinctrl dt node to be its parent node.
> 
> This patch wants to fix driver to support acpi since gpio controller
> should work well during acpi is enabled. But during upstream, driver is
> better to fix other thing together includes:
>  - add 'clock-names' to allow driver to get clocks by devm_clk_get().
>  - get io resource and irq by platform common apis.
>  - use fwnode instead of of_node from device structure.

The dependency patch is the part of another series. Dunno what will happen
to that series. In either way we would need maintainer's (Rafael) tag.

...

> +static int rockchip_gpio_get_clocks(struct rockchip_pin_bank *bank)
> +{
> +	struct device *dev = bank->dev;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	int ret;
> +
> +	if (!is_of_node(fwnode))
> +		return 0;
> +
> +	bank->clk = devm_clk_get(dev, "bus");
> +	if (IS_ERR(bank->clk)) {
> +		dev_err(dev, "fail to get apb clock\n");
> +		return PTR_ERR(bank->clk);
> +	}

> +	ret = clk_prepare_enable(bank->clk);
> +	if (ret < 0)
> +		return ret;

Now, you are mixinig devm_ with non-devm_ APIs. Perhaps you wanted
to use devm_clk_get_enabled()?

> +	bank->db_clk = devm_clk_get(dev, "db");
> +	if (IS_ERR(bank->db_clk)) {
> +		bank->db_clk = NULL;
> +	}
> +
> +	ret = clk_prepare_enable(bank->db_clk);
> +	if (ret < 0) {
> +		clk_disable_unprepare(bank->clk);
> +		return ret;
> +	}
>  
>  	return 0;
>  }

...

> +	if (is_acpi_node(fwnode)) {
> +		if (!acpi_dev_uid_to_integer(ACPI_COMPANION(dev), &uid))
> +			bank_id = (int)uid;

It probably needs to be

	if (is_acpi_node(fwnode)) {
		ret = acpi_dev_uid_to_integer(ACPI_COMPANION(dev), &uid);
		if (ret)
			return ret;
		bank_id = uid;
> +	} else {
> +		bank_id = of_alias_get_id(to_of_node(fwnode), "gpio");
> +		if (bank_id < 0)
> +			bank_id = gpio++;
> +	}

-- 
With Best Regards,
Andy Shevchenko