Re: [PATCH v3 RESEND] gpio: rockchip: support acpi

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Wed, Sep 07, 2022 at 05:27:22PM +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

...

> -	ret = irq_alloc_domain_generic_chips(bank->domain, 32, 1,
> +	ret = irq_alloc_domain_generic_chips(bank->domain, GPIO_MAX_PINS , 1,

Extra space.

>  					     "rockchip_gpio_irq",
>  					     handle_level_irq,
>  					     clr, 0, 0);

...

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

In the below code will be a memory leak. Also at ->remove().

> +		if (!gc->label)
> +			return -ENOMEM;
> +	}

...

> +static int rockchip_gpio_of_get_bank_id(struct device *dev)
> +{
> +	static int gpio;
> +	int bank_id = -1;

> +	if (is_of_node(dev->of_node)) {

	struct fwnode_handle *fwnode = dev_fwnode(...);

> +		bank_id = of_alias_get_id(dev->of_node, "gpio");

to_of_node(fwnode)

> +		if (bank_id < 0)
> +			bank_id = gpio++;
> +	}
> +
> +	return bank_id;
> +}

...

> +	bank_id = rockchip_gpio_acpi_get_bank_id(dev);
> +	if (bank_id < 0) {
> +		bank_id = rockchip_gpio_of_get_bank_id(dev);
> +		if (bank_id < 0)
> +			return bank_id;
> +	}

Easier to read:

	bank_id = rockchip_gpio_acpi_get_bank_id(dev);
	if (bank_id < 0)
		bank_id = rockchip_gpio_of_get_bank_id(dev);
	if (bank_id < 0)
		return bank_id;

...

> +	if (!is_acpi_node(dev_fwnode(dev)) {
> +		struct device_node *pctlnp = of_get_parent(dev->of_node);
>  
> +		pctldev = of_pinctrl_get(pctlnp);
> +		if (!pctldev)
> +			return -EPROBE_DEFER;
>  
> +		bank = rockchip_gpio_find_bank(pctldev, bank_id);
> +		if (!bank)
> +			return -ENODEV;

> +	}
> +	if (!bank) {

Simply '} else {' ?

> +		bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL);
> +		if (!bank)
> +			return -ENOMEM;
> +	}

...

> +	if (is_of_node(dev_fwnode(dev)) {
> +		bank->clk = devm_clk_get(dev, "bus");
> +		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);
> +			}
> +		}
> +
> +		bank->db_clk = devm_clk_get(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;
> +		}
> +	}

This can be split to a separate

	int rockchip_gpio_get_clocks(...)

...

> +	clk_prepare_enable(bank->clk);
> +	clk_prepare_enable(bank->db_clk);

Either of them may fail.

...

> +	if (!device_property_read_bool(bank->dev, "gpio-ranges") && pctldev) {

Same question as before, why the GPIO library code is not okay for this?

> +		struct gpio_chip *gc = &bank->gpio_chip;
> +
> +		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 err_unlock;
> +		}
>  	}


-- 
With Best Regards,
Andy Shevchenko