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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

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)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html