Re: [PATCH v3] gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Wed, May 27, 2020 at 02:27:49PM +0300, Mika Westerberg wrote:
> On Tue, May 26, 2020 at 08:12:22PM +0300, Andy Shevchenko wrote:
> > ACPI table on Intel Galileo Gen 2 has wrong pin number for IRQ resource
> > of one of the I²C GPIO expanders. Since we know what that number is and
> > luckily have GPIO bases fixed for SoC's controllers, we may use a simple
> > DMI quirk to match the platform and retrieve GpioInt() pin on it for
> > the expander in question.

...

> > +static int pca953x_acpi_get_pin(struct acpi_resource *ares, void *data)
> > +{
> > +	struct acpi_resource_gpio *agpio;
> > +	int *pin = data;
> > +
> > +	if (!acpi_gpio_get_irq_resource(ares, &agpio))
> > +		return 1;
> > +
> > +	*pin = agpio->pin_table[0];
> 
> Writing it like below looks better IMHO:
> 
> 	if (acpi_gpio_get_irq_resource(ares, &agpio))
> 		*pin = agpio->pin_table[0];
> 	return 1;

Actually this reveals a suboptimal behaviour in comparison to
acpi_walk_resources(), i.e. there is no way to stop traversing when
(1st) resource is found.

But okay, I will rewrite.

> > +}

...

> > +	ret = acpi_dev_get_resources(adev, &r, pca953x_acpi_get_pin, &pin);
> > +	acpi_dev_free_resource_list(&r);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return pin;
> 
> Or
> 	return ret < 0 ? ret : pin;

I think former is better to read since it has been grouped appropriately.

...

> > +static int pca953x_acpi_get_irq(struct device *dev)
> > +{
> > +	int pin, ret;
> > +
> > +	pin = pca953x_acpi_find_pin(dev);
> > +	if (pin < 0)
> > +		return pin;
> 
> Since you don't actually check the error value you may also return
> simply 0 here (invalid IRQ) and other places.

I don't think it is a good idea, because...

> > +	dev_info(dev, "Applying ACPI interrupt quirk (GPIO %d)\n", pin);
> > +
> > +	if (!gpio_is_valid(pin))
> > +		return -EINVAL;
> > +
> > +	ret = gpio_request(pin, "pca953x interrupt");
> > +	if (ret)
> > +		return ret;
> > +
> > +	return gpio_to_irq(pin);

... this will become to something like

	ret = gpio_to_irq();
	if (ret < 0)
		return 0;

	return ret;

> > +}

...

I agree on the rest.


-- 
With Best Regards,
Andy Shevchenko