Messages in this thread | | | Date | Thu, 09 Jun 2022 13:08:53 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip |
| |
On 2022-06-09 11:00, Paul Cercueil wrote: > Hi Aidan, > > Le mar., juin 7 2022 at 17:47:19 +0100, Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> a écrit : >> >> Paul Cercueil <paul@crapouillou.net> writes: >> >>> Hi Aidan, >>> >>> Le mar., juin 7 2022 at 12:05:25 +0100, Aidan MacDonald >>> <aidanmacdonald.0x0@gmail.com> a écrit : >>>> Update the driver to use an immutable IRQ chip to fix this warning: >>>> "not an immutable chip, please consider fixing it!" >>>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> >>>> --- >>>> drivers/pinctrl/pinctrl-ingenic.c | 33 >>>> ++++++++++++++++++------------- >>>> 1 file changed, 19 insertions(+), 14 deletions(-) >>>> diff --git a/drivers/pinctrl/pinctrl-ingenic.c >>>> b/drivers/pinctrl/pinctrl-ingenic.c >>>> index 1ca11616db74..37258fb05be3 100644 >>>> --- a/drivers/pinctrl/pinctrl-ingenic.c >>>> +++ b/drivers/pinctrl/pinctrl-ingenic.c >>>> @@ -135,7 +135,6 @@ struct ingenic_pinctrl { >>>> struct ingenic_gpio_chip { >>>> struct ingenic_pinctrl *jzpc; >>>> struct gpio_chip gc; >>>> - struct irq_chip irq_chip; >>>> unsigned int irq, reg_base; >>>> }; >>>> @@ -3419,6 +3418,8 @@ static void ingenic_gpio_irq_enable(struct >>>> irq_data >>>> *irqd) >>>> struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc); >>>> int irq = irqd->hwirq; >>>> + gpiochip_enable_irq(gc, irq); >>>> + >>>> if (is_soc_or_above(jzgc->jzpc, ID_JZ4770)) >>>> ingenic_gpio_set_bit(jzgc, JZ4770_GPIO_INT, irq, true); >>>> else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740)) >>>> @@ -3443,6 +3444,8 @@ static void ingenic_gpio_irq_disable(struct >>>> irq_data >>>> *irqd) >>>> ingenic_gpio_set_bit(jzgc, JZ4740_GPIO_SELECT, irq, false); >>>> else >>>> ingenic_gpio_set_bit(jzgc, JZ4730_GPIO_GPIER, irq, false); >>>> + >>>> + gpiochip_disable_irq(gc, irq); >>>> } >>>> static void ingenic_gpio_irq_ack(struct irq_data *irqd) >>>> @@ -3684,6 +3687,20 @@ static void ingenic_gpio_irq_release(struct >>>> irq_data >>>> *data) >>>> return gpiochip_relres_irq(gpio_chip, data->hwirq); >>>> } >>>> +static const struct irq_chip ingenic_gpio_irqchip = { >>>> + .name = "gpio", >>>> + .irq_enable = ingenic_gpio_irq_enable, >>>> + .irq_disable = ingenic_gpio_irq_disable, >>>> + .irq_unmask = ingenic_gpio_irq_unmask, >>>> + .irq_mask = ingenic_gpio_irq_mask, >>>> + .irq_ack = ingenic_gpio_irq_ack, >>>> + .irq_set_type = ingenic_gpio_irq_set_type, >>>> + .irq_set_wake = ingenic_gpio_irq_set_wake, >>>> + .irq_request_resources = ingenic_gpio_irq_request, >>>> + .irq_release_resources = ingenic_gpio_irq_release, >>>> + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, >>>> +}; >>>> + >>>> static int ingenic_pinmux_set_pin_fn(struct ingenic_pinctrl *jzpc, >>>> int pin, int func) >>>> { >>>> @@ -4172,20 +4189,8 @@ static int __init ingenic_gpio_probe(struct >>>> ingenic_pinctrl *jzpc, >>>> if (!jzgc->irq) >>>> return -EINVAL; >>>> - jzgc->irq_chip.name = jzgc->gc.label; >>>> - jzgc->irq_chip.irq_enable = ingenic_gpio_irq_enable; >>>> - jzgc->irq_chip.irq_disable = ingenic_gpio_irq_disable; >>>> - jzgc->irq_chip.irq_unmask = ingenic_gpio_irq_unmask; >>>> - jzgc->irq_chip.irq_mask = ingenic_gpio_irq_mask; >>>> - jzgc->irq_chip.irq_ack = ingenic_gpio_irq_ack; >>>> - jzgc->irq_chip.irq_set_type = ingenic_gpio_irq_set_type; >>>> - jzgc->irq_chip.irq_set_wake = ingenic_gpio_irq_set_wake; >>>> - jzgc->irq_chip.irq_request_resources = ingenic_gpio_irq_request; >>>> - jzgc->irq_chip.irq_release_resources = ingenic_gpio_irq_release; >>>> - jzgc->irq_chip.flags = IRQCHIP_MASK_ON_SUSPEND; >>>> - >>>> girq = &jzgc->gc.irq; >>>> - girq->chip = &jzgc->irq_chip; >>>> + gpio_irq_chip_set_chip(girq, &ingenic_gpio_irqchip); >>> >>> This will change each irq_chip's name to "gpio", do we want that? >>> >>> You didn't remove jzgc->irq_chip, so maybe what you could do is >>> jzgc->irq_chip = ingenic_gpio_irqchip; >>> jzgc->irq_chip.name = jzgc->gc.label; >>> gpio_irq_chip_set_chip(girq, &jzgc->irq_chip); >>> >>> Thoughts? >>> >>> Cheers, >>> -Paul >>> >> >> I wondered that myself, but it doesn't seem to affect anything except >> what is displayed in /proc/interrupts. Is the name used anywhere else >> where it might cause confusion? > > I don't really know. If it only really affects the display in > /proc/interrupts then I'm fine with it. In doubt, I'd prefer to keep > the existing names. > >> The only similar case I could find was pinctrl-microchip-sgpio.c where >> microchip_sgpio_register_bank() is called in a loop and registers the >> same irq chip repeatedly, so it's probably(?) okay to do this here. It >> seems to defeat the point of immutable irqchips if they just have to >> be >> copied anyway... > > The point of immutable irqchips is that they aren't modified by the > core, if I understand it correctly. Immutable doesn't mean it has to > be static const.
I want these to be made const. I agree that the fancy string should be kept (sadly), as it is a userspace visible change, and we don't do that.
You can solve it using the irq_print_chip() callback as part of your irq_chip structures. See 3344265a2692 for an example.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |