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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

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?

...

> +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?

> +		bank_id = of_alias_get_id(dev->of_node, "gpio");
> +		if (bank_id < 0)
> +			bank_id = gpio++;
> +	}
> +
> +	return bank_id;
> +}

...

> +#ifdef CONFIG_ACPI

Why?

> +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?

> +	}
> +
> +	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?

-- 
With Best Regards,
Andy Shevchenko