| Message ID | 20230822152244.214394-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | HID: cp2112: make the irqchip immutable | expand |
On Tue, Aug 22, 2023 at 05:22:44PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > This make the GPIO irqchip exposed by the CP2112 driver use an immutable > irq_chip struct thus addressing the following warning on probe: > > (cp2112_gpio): not an immutable chip, please consider fixing it! https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=3e2977c425ad2789ca18084fff913cceacae75a2 Can you test HID for-next for this hardware?
On Tue, Aug 22, 2023 at 05:38:45PM +0200, Bartosz Golaszewski wrote: > On Tue, Aug 22, 2023 at 5:28 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Aug 22, 2023 at 05:22:44PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > This make the GPIO irqchip exposed by the CP2112 driver use an immutable > > > irq_chip struct thus addressing the following warning on probe: > > > > > > (cp2112_gpio): not an immutable chip, please consider fixing it! > > > > https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=3e2977c425ad2789ca18084fff913cceacae75a2 > > > > Can you test HID for-next for this hardware? > > Ah you beat me to it. I didn't see this one. Now looking at it, I don't understand why I added a call to IRQ shutdown callback... (Looks like it slipped from experimental patch.) If you can test this, would be nice.
On Tue, Aug 22, 2023 at 5:50 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Aug 22, 2023 at 05:38:45PM +0200, Bartosz Golaszewski wrote: > > On Tue, Aug 22, 2023 at 5:28 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Tue, Aug 22, 2023 at 05:22:44PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > This make the GPIO irqchip exposed by the CP2112 driver use an immutable > > > > irq_chip struct thus addressing the following warning on probe: > > > > > > > > (cp2112_gpio): not an immutable chip, please consider fixing it! > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/commit/?h=for-next&id=3e2977c425ad2789ca18084fff913cceacae75a2 > > > > > > Can you test HID for-next for this hardware? > > > > Ah you beat me to it. I didn't see this one. > > Now looking at it, I don't understand why I added a call to IRQ shutdown > callback... (Looks like it slipped from experimental patch.) If you can > test this, would be nice. > Yeah it works alright. It's already in next but FWIW: Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
On Tue, Aug 22, 2023 at 5:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > This make the GPIO irqchip exposed by the CP2112 driver use an immutable > irq_chip struct thus addressing the following warning on probe: > > (cp2112_gpio): not an immutable chip, please consider fixing it! > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Thanks for fixing this. FWIW: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Marc Z, have you checked how we stand with immutable GPIO irqchips? We should be able to smoke it out to default behaviour soon I think. Yours, Linus Walleij
On Wed, 23 Aug 2023 13:14:59 +0100, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Aug 22, 2023 at 5:22 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > This make the GPIO irqchip exposed by the CP2112 driver use an immutable > > irq_chip struct thus addressing the following warning on probe: > > > > (cp2112_gpio): not an immutable chip, please consider fixing it! > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Thanks for fixing this. FWIW: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Marc Z, have you checked how we stand with immutable GPIO > irqchips? We should be able to smoke it out to default behaviour > soon I think. I haven't recently checked, but I'm still in favour of actively make the new behaviour the only one supported. The sooner we break things, the better... M.
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c index 27cadadda7c9..01f2a7211033 100644 --- a/drivers/hid/hid-cp2112.c +++ b/drivers/hid/hid-cp2112.c @@ -163,7 +163,6 @@ struct cp2112_device { atomic_t read_avail; atomic_t xfer_avail; struct gpio_chip gc; - struct irq_chip irq; u8 *in_out_buffer; struct mutex lock; @@ -1082,6 +1081,7 @@ static void cp2112_gpio_irq_mask(struct irq_data *d) struct cp2112_device *dev = gpiochip_get_data(gc); __clear_bit(d->hwirq, &dev->irq_mask); + gpiochip_disable_irq(gc, irqd_to_hwirq(d)); } static void cp2112_gpio_irq_unmask(struct irq_data *d) @@ -1089,6 +1089,7 @@ static void cp2112_gpio_irq_unmask(struct irq_data *d) struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct cp2112_device *dev = gpiochip_get_data(gc); + gpiochip_enable_irq(gc, irqd_to_hwirq(d)); __set_bit(d->hwirq, &dev->irq_mask); } @@ -1228,6 +1229,18 @@ static int __maybe_unused cp2112_allocate_irq(struct cp2112_device *dev, return ret; } +static const struct irq_chip cp2112_irq_chip = { + .name = "cp2112-gpio", + .irq_startup = cp2112_gpio_irq_startup, + .irq_shutdown = cp2112_gpio_irq_shutdown, + .irq_ack = cp2112_gpio_irq_ack, + .irq_mask = cp2112_gpio_irq_mask, + .irq_unmask = cp2112_gpio_irq_unmask, + .irq_set_type = cp2112_gpio_irq_type, + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) { struct cp2112_device *dev; @@ -1337,17 +1350,8 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) dev->gc.can_sleep = 1; dev->gc.parent = &hdev->dev; - dev->irq.name = "cp2112-gpio"; - dev->irq.irq_startup = cp2112_gpio_irq_startup; - dev->irq.irq_shutdown = cp2112_gpio_irq_shutdown; - dev->irq.irq_ack = cp2112_gpio_irq_ack; - dev->irq.irq_mask = cp2112_gpio_irq_mask; - dev->irq.irq_unmask = cp2112_gpio_irq_unmask; - dev->irq.irq_set_type = cp2112_gpio_irq_type; - dev->irq.flags = IRQCHIP_MASK_ON_SUSPEND; - girq = &dev->gc.irq; - girq->chip = &dev->irq; + gpio_irq_chip_set_chip(girq, &cp2112_irq_chip); /* The event comes from the outside so no parent handler */ girq->parent_handler = NULL; girq->num_parents = 0;