Re: [PATCH v4] gpio: rockchip: support acpi

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, Sep 08, 2022 at 03:26:21PM +0800, Jianqun Xu wrote:
> This patch fix driver to support acpi by following changes:
>  * support get gpio bank number from uid of acpi
>  * try to get clocks for dt nodes but for acpi
>  * try to get clocks by a char id first, if a dt patch applied

...

> +	if (!gc->label) {

See below.

> +		gc->label = kasprintf(GFP_KERNEL, "gpio%d", bank->bank_num);
> +		if (!gc->label)
> +			return -ENOMEM;
> +	}

...

> -	/*
> -	 * For DeviceTree-supported systems, the gpio core checks the
> -	 * pinctrl's device node for the "gpio-ranges" property.
> -	 * If it is present, it takes care of adding the pin ranges
> -	 * for the driver. In this case the driver can skip ahead.
> -	 *
> -	 * In order to remain compatible with older, existing DeviceTree
> -	 * files which don't set the "gpio-ranges" property or systems that
> -	 * utilize ACPI the driver has to call gpiochip_add_pin_range().
> -	 */

Why did you remove this comment? It's exactly an answer to what I was asking.

> +	if (!device_property_read_bool(bank->dev, "gpio-ranges") && pctldev) {
>  		ret = gpiochip_add_pin_range(gc, dev_name(pctldev->dev), 0,
>  					     gc->base, gc->ngpio);
>  		if (ret) {
>  			dev_err(bank->dev, "Failed to add pin range\n");
> -			goto fail;
> +			goto err_remove;
>  		}
>  	}

...

> +	if (!bank->name)

It's not obvious check for this. Perhaps you can synchronize above to have
the same check and add a comment why bank->name presence guarantees that
label wasn't allocated by us here.

OTOH, why not to use devm_kasprintf()?

> +		kfree(gc->label);

...

> +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)) {
> +		bank->clk = of_clk_get(to_of_node(fwnode), 0);
> +		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;
> +
> +	bank->db_clk = devm_clk_get(dev, "db");
> +	if (IS_ERR(bank->db_clk)) {
> +		bank->db_clk = of_clk_get(to_of_node(fwnode), 1);
> +		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);

You can't mix devm_ with non-devm_ calls.

> +		return ret;
> +	}

...

> +	if (!bank->name)
> +		kfree(gc->label);

As per above.

-- 
With Best Regards,
Andy Shevchenko