Re: [PATCH 08/21] gpio: acpi: provide acpi_gpio_device_free_interrupts()

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Tue, Sep 05, 2023 at 08:52:56PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We're moving away from public functions that take struct gpio_chip as
> argument as the chip - unlike struct gpio_device - is owned by its
> provider and tied to its lifetime.
> 
> Provide an alternative to acpi_gpiochip_free_interrupts().

Looks good to me, few minor comments below.

> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> ---
>  drivers/gpio/gpiolib-acpi.c | 29 +++++++++++++++++++++++------
>  include/linux/gpio/driver.h | 12 ++++++++++++
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index fbda452fb4d6..5633e39396bc 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -558,12 +558,9 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
>  }
>  EXPORT_SYMBOL_GPL(acpi_gpiochip_request_interrupts);
>  
> -/**
> - * acpi_gpiochip_free_interrupts() - Free GPIO ACPI event interrupts.
> - * @chip:      GPIO chip
> - *
> - * Free interrupts associated with GPIO ACPI event method for the given
> - * GPIO chip.
> +/*
> + * This function is deprecated, use acpi_gpio_device_free_interrupts()
> + * instead.
>   */
>  void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
>  {
> @@ -604,6 +601,26 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
>  }
>  EXPORT_SYMBOL_GPL(acpi_gpiochip_free_interrupts);
>  
> +/**
> + * acpi_gpio_device_free_interrupts() - Free GPIO ACPI event interrupts.
> + * @chip	GPIO device

Should be:

@chip: GPIO device

> + *
> + * Free interrupts associated with GPIO ACPI event method for the given
> + * GPIO device.
> + */
> +void acpi_gpio_device_free_interrupts(struct gpio_device *gdev)
> +{
> +	struct gpio_chip *gc;
> +
> +	/* TODO: protect gdev->chip once SRCU is in place in GPIOLIB. */
> +	gc = gdev->chip;
> +	if (!gc)
> +		return;
> +
> +	acpi_gpiochip_free_interrupts(gc);
> +}
> +EXPORT_SYMBOL_GPL(acpi_gpio_device_free_interrupts);
> +
>  int acpi_dev_add_driver_gpios(struct acpi_device *adev,
>  			      const struct acpi_gpio_mapping *gpios)
>  {
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index b68b3493b29d..47906bc56b3d 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -835,4 +835,16 @@ static inline struct fwnode_handle *gpiochip_node_get_first(struct device *dev)
>  	return NULL;
>  }
>  
> +/*
> + * FIXME: Remove this once the only driver that uses it - android tablets -
> + * becomes a good citizen and stops abusing GPIOLIB.

There are a acouple of more when grepping for acpi_gpiochip_free_interrupts().

I'm not entirely sure why these functions are exposed to the drivers in
the first place. IMHO GPIOLIB should deal with these but perhaps there
is some good reason these drivers do it...

> + */
> +#ifdef CONFIG_ACPI
> +void acpi_gpio_device_free_interrupts(struct gpio_device *gdev);
> +#else
> +static inline void acpi_gpio_device_free_interrupts(struct gpio_device *gdev)
> +{
> +}

I would put these {} to the same line:

static inline void acpi_gpio_device_free_interrupts(struct gpio_device *gdev) { }

> +#endif
> +
>  #endif /* __LINUX_GPIO_DRIVER_H */
> -- 
> 2.39.2