[PATCH v4] gpio: rockchip: support acpi

[jay.xu at rock-chips.com]

Hi Andy

--------------
jay.xu at rock-chips.com
>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. 
It's a mistake, I will restore this comment in next version patch.

>
>> +	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()? 
Got it
>
>> +	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.
> 
Okay, I add 'clock-names' property for all existed related dt files and fix driver to only use
devm_clk_get(), I push a version please help to review

>> +	return ret;
>> +	}
>
>...
>
>> +	if (!bank->name)
>> +	kfree(gc->label);
>
>As per above.
>
>--
>With Best Regards,
>Andy Shevchenko
>
>