Re: [PATCH 2/5] gpiolib: Initialize the hardware with a callback

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 09-10-2019 18:50, Andy Shevchenko wrote:
> After changing the drivers to use GPIO core to add an IRQ chip
> it appears that some of them requires a hardware initialization
> before adding the IRQ chip.
>
> Add an optional callback ->init_hw() to allow that drivers
> to initialize hardware if needed.
>
> This change is a part of the fix NULL pointer dereference
> brought to the several drivers recently.
>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

Hmm, IIRC Linus Walleij already added a callback for initializing the
mask before the irchip gets initialized which is basically intended for
what you want this callback for I think ?

Regards.

Hans



> ---
>   drivers/gpio/gpiolib.c      | 22 +++++++++++++++++++++-
>   include/linux/gpio/driver.h |  8 ++++++++
>   2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bdbc1649eafa..85601ad4fcd5 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -86,6 +86,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
>   				struct lock_class_key *lock_key,
>   				struct lock_class_key *request_key);
>   static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip);
> +static int gpiochip_irqchip_init_hw(struct gpio_chip *gpiochip);
>   static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip);
>   static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip);
>   
> @@ -1406,6 +1407,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
>   
>   	machine_gpiochip_add(chip);
>   
> +	ret = gpiochip_irqchip_init_hw(chip);
> +	if (ret)
> +		goto err_remove_acpi_chip;
> +
>   	ret = gpiochip_irqchip_init_valid_mask(chip);
>   	if (ret)
>   		goto err_remove_acpi_chip;
> @@ -1622,6 +1627,16 @@ static struct gpio_chip *find_chip_by_name(const char *name)
>    * The following is irqchip helper code for gpiochips.
>    */
>   
> +static int gpiochip_irqchip_init_hw(struct gpio_chip *gc)
> +{
> +	struct gpio_irq_chip *girq = &gc->irq;
> +
> +	if (!girq->init_hw)
> +		return 0;
> +
> +	return girq->init_hw(gc);
> +}
> +
>   static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gc)
>   {
>   	struct gpio_irq_chip *girq = &gc->irq;
> @@ -2446,8 +2461,13 @@ static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
>   {
>   	return 0;
>   }
> -
>   static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) {}
> +
> +static inline int gpiochip_irqchip_init_hw(struct gpio_chip *gpiochip)
> +{
> +	return 0;
> +}
> +
>   static inline int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip)
>   {
>   	return 0;
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index f8245d67f070..5dd9c982e2cb 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -201,6 +201,14 @@ struct gpio_irq_chip {
>   	 */
>   	bool threaded;
>   
> +	/**
> +	 * @init_hw: optional routine to initialize hardware before
> +	 * an IRQ chip will be added. This is quite useful when
> +	 * a particular driver wants to clear IRQ related registers
> +	 * in order to avoid undesired events.
> +	 */
> +	int (*init_hw)(struct gpio_chip *chip);
> +
>   	/**
>   	 * @init_valid_mask: optional routine to initialize @valid_mask, to be
>   	 * used if not all GPIO lines are valid interrupts. Sometimes some