Re: [PATCH 2/6] gpiolib: override irq_request/release_resources hooks

[Hans Verkuil <hverkuil@xxxxxxxxx>]

On 09/07/2018 12:33 PM, Mika Westerberg wrote:
> Hi,
> 
> On Fri, Sep 07, 2018 at 12:20:49PM +0200, Hans Verkuil wrote:
>> -static int intel_gpio_irq_reqres(struct irq_data *d)
>> -{
>> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> -	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
>> -	int pin;
>> -	int ret;
>> -
>> -	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
> 
> I don't think you can just remove this hook because we depend on the
> above translation.
> 
>> -	if (pin >= 0) {
>> -		ret = gpiochip_lock_as_irq(gc, pin);
>> -		if (ret) {
>> -			dev_err(pctrl->dev, "unable to lock HW IRQ %d for IRQ\n",
>> -				pin);
>> -			return ret;
>> -		}
>> -	}
>> -	return 0;
>> -}
>> -
>> -static void intel_gpio_irq_relres(struct irq_data *d)
>> -{
>> -	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> -	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
>> -	int pin;
>> -
>> -	pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), NULL, NULL);
> 
> Same here.
> 
>> -	if (pin >= 0)
>> -		gpiochip_unlock_as_irq(gc, pin);
>> -}

Oops, I didn't see this.

Linus, you said here https://www.spinics.net/lists/linux-gpio/msg32297.html:

"Actually the latter method, to use local copies of the enable/disable
function pointers in gpiochip->irq is what we should use also
for the request/release_resources callbacks."

I did that in this series, but in the original code gpiolib never overrides
the irq_chip callbacks, only if both request and release callbacks are NULL
will gpiolib fill in its own callback.

And that's no doubt for this reason (irq translation).

Unless you have a better idea, then I will just revert this and only set
these callbacks if the driver doesn't specify them (i.e. pretty much what I
did in my RFCv3 series).

The advantage is that I no longer need to modify this driver and the
pinctrl-st.c driver.

Regards,

	Hans