Re: [RFC PATCH] gpiolib: chain irq_request/release_resources() and irq_dis/enable()

[Hans Verkuil <hverkuil@xxxxxxxxx>]

Re-ping!

If I don't see any replies in the next few days I'll just go ahead and make
a proper patch series of this.

Regards,

	Hans

On 02/08/18 09:00, Hans Verkuil wrote:
> Ping!
> 
> Regards,
> 
> 	Hans
> 
> On 07/23/2018 05:03 PM, Hans Verkuil wrote:
>> Hi all,
>>
>> Here is yet another attempt to allow drivers to disable the irq and drive
>> the gpio as an output.
>>
>> Please be gentle with me: I am neither an expert on the gpio internals, nor on
>> the irq internals.
>>
>> This patch lets gpiolib override the irq_chip's irq_request/release_resources and
>> irq_en/disable hooks.
>>
>> The old hooks are stored and called by gpiolib.
>>
>> As a result, the gpiochip_(un)lock_as_irq functions can become static (not yet
>> implemented in this patch) and these calls should be removed from all drivers.
>>
>> I haven't done that since I first want to know if what I am doing here even
>> makes sense.
>>
>> Reviewing the removal of those calls in drivers should be fairly easy.
>>
>> Regards,
>>
>> 	Hans
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>>  drivers/gpio/gpiolib.c      | 78 +++++++++++++++++++++++--------------
>>  include/linux/gpio/driver.h |  5 +++
>>  2 files changed, 53 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index e11a3bb03820..20429197756f 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1800,28 +1800,48 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
>>  static int gpiochip_irq_reqres(struct irq_data *d)
>>  {
>>  	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +	int res = 0;
>>
>>  	if (!try_module_get(chip->gpiodev->owner))
>>  		return -ENODEV;
>> -
>> -	if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>> -		chip_err(chip,
>> -			"unable to lock HW IRQ %lu for IRQ\n",
>> -			d->hwirq);
>> +	if (chip->irq.chip->irq_request_resources)
>> +		res = chip->irq.chip->irq_request_resources(d);
>> +	if (res)
>>  		module_put(chip->gpiodev->owner);
>> -		return -EINVAL;
>> -	}
>> -	return 0;
>> +	return res;
>>  }
>>
>>  static void gpiochip_irq_relres(struct irq_data *d)
>>  {
>>  	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>>
>> -	gpiochip_unlock_as_irq(chip, d->hwirq);
>> +	if (chip->irq.chip->irq_release_resources)
>> +		chip->irq.chip->irq_release_resources(d);
>>  	module_put(chip->gpiodev->owner);
>>  }
>>
>> +static void gpiochip_irq_enable(struct irq_data *d)
>> +{
>> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> +	WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq));
>> +	if (chip->irq.chip->irq_enable)
>> +		chip->irq.chip->irq_enable(d);
>> +	else
>> +		chip->irq.chip->irq_unmask(d);
>> +}
>> +
>> +static void gpiochip_irq_disable(struct irq_data *d)
>> +{
>> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> +	gpiochip_unlock_as_irq(chip, d->hwirq);
>> +	if (chip->irq.chip->irq_disable)
>> +		chip->irq.chip->irq_disable(d);
>> +	else
>> +		chip->irq.chip->irq_mask(d);
>> +}
>> +
>>  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
>>  {
>>  	if (!gpiochip_irqchip_irq_valid(chip, offset))
>> @@ -1889,16 +1909,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
>>  	if (!gpiochip->irq.domain)
>>  		return -EINVAL;
>>
>> -	/*
>> -	 * It is possible for a driver to override this, but only if the
>> -	 * alternative functions are both implemented.
>> -	 */
>> -	if (!irqchip->irq_request_resources &&
>> -	    !irqchip->irq_release_resources) {
>> -		irqchip->irq_request_resources = gpiochip_irq_reqres;
>> -		irqchip->irq_release_resources = gpiochip_irq_relres;
>> -	}
>> -
>>  	if (gpiochip->irq.parent_handler) {
>>  		void *data = gpiochip->irq.parent_handler_data ?: gpiochip;
>>
>> @@ -1956,8 +1966,16 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
>>  	}
>>
>>  	if (gpiochip->irq.chip) {
>> -		gpiochip->irq.chip->irq_request_resources = NULL;
>> -		gpiochip->irq.chip->irq_release_resources = NULL;
>> +		gpiochip->irq.chip->irq_request_resources =
>> +			gpiochip->irq.irq_request_resources;
>> +		gpiochip->irq.chip->irq_release_resources =
>> +			gpiochip->irq.irq_release_resources;
>> +		gpiochip->irq.chip->irq_enable = gpiochip->irq.irq_enable;
>> +		gpiochip->irq.chip->irq_disable = gpiochip->irq.irq_disable;
>> +		gpiochip->irq.irq_request_resources = NULL;
>> +		gpiochip->irq.irq_release_resources = NULL;
>> +		gpiochip->irq.irq_enable = NULL;
>> +		gpiochip->irq.irq_disable = NULL;
>>  		gpiochip->irq.chip = NULL;
>>  	}
>>
>> @@ -2048,15 +2066,15 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
>>  		return -EINVAL;
>>  	}
>>
>> -	/*
>> -	 * It is possible for a driver to override this, but only if the
>> -	 * alternative functions are both implemented.
>> -	 */
>> -	if (!irqchip->irq_request_resources &&
>> -	    !irqchip->irq_release_resources) {
>> -		irqchip->irq_request_resources = gpiochip_irq_reqres;
>> -		irqchip->irq_release_resources = gpiochip_irq_relres;
>> -	}
>> +	gpiochip->irq.irq_request_resources = irqchip->irq_request_resources;
>> +	gpiochip->irq.irq_release_resources = irqchip->irq_release_resources;
>> +	gpiochip->irq.irq_enable = irqchip->irq_enable;
>> +	gpiochip->irq.irq_disable = irqchip->irq_disable;
>> +
>> +	irqchip->irq_request_resources = gpiochip_irq_reqres;
>> +	irqchip->irq_release_resources = gpiochip_irq_relres;
>> +	irqchip->irq_enable = gpiochip_irq_enable;
>> +	irqchip->irq_disable = gpiochip_irq_disable;
>>
>>  	acpi_gpiochip_request_interrupts(gpiochip);
>>
>> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
>> index 5382b5183b7e..e352dd160424 100644
>> --- a/include/linux/gpio/driver.h
>> +++ b/include/linux/gpio/driver.h
>> @@ -138,6 +138,11 @@ struct gpio_irq_chip {
>>  	 * will allocate and map all IRQs during initialization.
>>  	 */
>>  	unsigned int first;
>> +
>> +	int		(*irq_request_resources)(struct irq_data *data);
>> +	void		(*irq_release_resources)(struct irq_data *data);
>> +	void		(*irq_enable)(struct irq_data *data);
>> +	void		(*irq_disable)(struct irq_data *data);
>>  };
>>
>>  static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip)
>>
>