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

["jay.xu@rock-chips.com" <jay.xu@rock-chips.com>]

Hi Andy

--------------
jay.xu@rock-chips.com
>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. 
Got it.

>
>>       "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) 
Got it.

>
>> +	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;
> 
Got it.

>...
>
>> +	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 {' ? 
Got it.

>
>> +	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(...)
> 
Got it.

>...
>
>> +	clk_prepare_enable(bank->clk);
>> +	clk_prepare_enable(bank->db_clk);
>
>Either of them may fail.
> 
Got it.

>...
>
>> +	if (!device_property_read_bool(bank->dev, "gpio-ranges") && pctldev) {
>
>Same question as before, why the GPIO library code is not okay for this?
> 
The dev_name(pctldev->dev) is null since there is no pinctrl device when acpi enabled.
Currently we may set the pinctrl mux and config under uefi, and no pinctrl driver in linux.

>> +	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
>
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip