Hi Jon,
On Wed, 04 May 2022 12:19:36 +0100,
Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
>
> Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> immutable") added a warning to indicate if the gpiolib is altering the
> internals of irqchips. Following this change the following warning is
> now observed for the max77620 gpio driver ...
>
> WARNING KERN gpio gpiochip0: (max77620-gpio): not an immutable chip,
> please consider fixing it!
>
> Fix the above warning by making the max77620 gpio driver immutable.
Thanks for looking into this. Comments below.
>
> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> ---
> This warning is observed with v5.18-rc5 and so it would be great if
> we can fix for v5.18. This is triggering a kernel warning test failure
> on one of our platforms.
I'm surprised. This is definitely *not* 5.18 material, and I can't see
the patches in Linus' tree. Are you sure you're not running -next
instead?
>
> drivers/gpio/gpio-max77620.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
> index ebf9dea6546b..aa92658780d6 100644
> --- a/drivers/gpio/gpio-max77620.c
> +++ b/drivers/gpio/gpio-max77620.c
> @@ -119,14 +119,23 @@ static void max77620_gpio_bus_sync_unlock(struct irq_data *data)
> mutex_unlock(&gpio->buslock);
> }
>
> -static struct irq_chip max77620_gpio_irqchip = {
> +static void max77620_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +
> + seq_printf(p, dev_name(gc->parent));
> +}
> +
I'd rather you don't do this. This was added as a band-aid for drivers
that display a device-specific string in /proc/interrupts, and that we
cannot fix because the string is in effect an ABI.
Here, you're going in the opposite direction (making the name
device-specific while it was constant so far). This has the same
ABI-breaking effect, and we shouldn't do that. It is also rather
pointless, as we already have all the required debugging information
in debugfs (and I assume that this is the reason this is added).
> +static const struct irq_chip max77620_gpio_irqchip = {
> .name = "max77620-gpio",
> .irq_mask = max77620_gpio_irq_mask,
> .irq_unmask = max77620_gpio_irq_unmask,
You seem to be missing the updates for these two functions. Please see
the updated documentation in commit 5644b66a9c63 ("Documentation:
Update the recommended pattern for GPIO irqchips").
> .irq_set_type = max77620_gpio_set_irq_type,
> .irq_bus_lock = max77620_gpio_bus_lock,
> .irq_bus_sync_unlock = max77620_gpio_bus_sync_unlock,
> - .flags = IRQCHIP_MASK_ON_SUSPEND,
> + .irq_print_chip = max77620_gpio_irq_print_chip,
> + .flags = IRQCHIP_IMMUTABLE | IRQCHIP_MASK_ON_SUSPEND,
> + GPIOCHIP_IRQ_RESOURCE_HELPERS,
> };
>
> static int max77620_gpio_dir_input(struct gpio_chip *gc, unsigned int offset)
> @@ -318,7 +327,7 @@ static int max77620_gpio_probe(struct platform_device *pdev)
> mgpio->gpio_chip.base = -1;
>
> girq = &mgpio->gpio_chip.irq;
> - girq->chip = &max77620_gpio_irqchip;
> + gpio_irq_chip_set_chip(girq, &max77620_gpio_irqchip);
> /* This will let us handle the parent IRQ in the driver */
> girq->parent_handler = NULL;
> girq->num_parents = 0;
> --
> 2.25.1
>
>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
