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

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

Hi Andy

--------------
jay.xu@rock-chips.com
>On Tue, Sep 06, 2022 at 09:30:25AM +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
>
>...
>
>> -	bank->domain = irq_domain_add_linear(bank->of_node, 32,
>> +	bank->domain = irq_domain_create_linear(dev_fwnode(bank->dev), 32,
>>  &irq_generic_chip_ops, NULL);
>
>Should it be converted to use GPIO_MAX_PINS at the same time? 
okay, will fix in v3

>
>...
>
>> +static int rockchip_gpio_of_get_bank_id(struct device *dev)
>> +{
>> +	static int gpio;
>> +	int bank_id = -1;
>
>> +	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
>
>Can't is_of_node() be sufficient check? 
okay, will fix in v3

>
>> +	bank_id = of_alias_get_id(dev->of_node, "gpio");
>> +	if (bank_id < 0)
>> +	bank_id = gpio++;
>> +	}
>> +
>> +	return bank_id;
>> +}
>
>...
>
>> +#ifdef CONFIG_ACPI
>
>Why? 
Because the acpi_device_uid only defined under CONFIG_ACPI,

>
>> +static int rockchip_gpio_acpi_get_bank_id(struct device *dev)
>> +{
>> +	struct acpi_device *adev;
>> +	unsigned long bank_id = -1;
>> +	const char *uid;
>> +	int ret;
>> +
>> +	adev = ACPI_COMPANION(dev);
>> +	if (!adev)
>> +	return -ENXIO;
>> +
>> +	uid = acpi_device_uid(adev);
>> +	if (!uid || !(*uid)) {
>> +	dev_err(dev, "Cannot retrieve UID\n");
>> +	return -ENODEV;
>
>Why is it a fatal error? Can't be the same approach as for OF be used? 
For the OF, the very early driver get the gpio id from the node name of dt node, such as
gpio0@xxxxxx

After a patch to change the dt node as common name like gpio0: gpio@xxxxx, but lack of the alias
the driver should to conside that so use a static gpio number id, also support get the controller id
from the aliasment.

For the ACPI, here we locat at the first version, and reference to the ACPI ducument, the gpio controller
id should passed  by the uid, so this patch treat it as a requirement but a option.

>
>> +	}
>> +
>> +	ret = kstrtoul(uid, 0, &bank_id);
>> +
>> +	return !ret ? bank_id : -ERANGE;
>
>Use standard pattern, i.e.
>
>	if (ret)
>	return ret;
>
>> +}
>> +#else
>> +static int rockchip_gpio_acpi_get_bank_id(struct device *dev)
>> +{
>> +	return -ENOENT;
>> +}
>> +#endif /* CONFIG_ACPI */
>
>...
>
>> +	int bank_id = 0;
>
>Redundant assignment.
>
>...
>
>> +	if (!ACPI_COMPANION(dev)) {
>
>One of:
>
>	is_of_node()
>	!is_acpi_node()
>
>?
>
>...
>
>> +	if (!ACPI_COMPANION(dev)) {
>
>Ditto.
>
>But looking how this disrupts the code, just leave OF and ACPI parts separate,
>don't mix them.
>
>...
>
>> +	if (!device_property_read_bool(bank->dev, "gpio-ranges") && pctldev) {
>> +	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;
>> +	}
>>  }
>
>Why? What's wrong with GPIO library doing this for all chips? 
Under ACPI case, the pinctrl device maybe not exist, currently our board runs without pinctrl,
so add a check to the pctldev.

>
>--
>With Best Regards,
>Andy Shevchenko
>
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip