| Message ID | 20220517163820.86768-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] pinctrl: intel: make irq_chip immutable | expand |
On Tue, May 17, 2022 at 07:38:19PM +0300, Andy Shevchenko wrote: > +static const struct irq_chip intel_gpio_irq_chip = { > + .name = "intel-gpio", > + .irq_ack = intel_gpio_irq_ack, > + .irq_mask = intel_gpio_irq_mask, > + .irq_unmask = intel_gpio_irq_unmask, > + .irq_set_type = intel_gpio_irq_type, > + .irq_set_wake = intel_gpio_irq_wake, > + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, > + GPIOCHIP_IRQ_RESOURCE_HELPERS, > +}; You still have the inconsistent alignment here.
On Wed, May 18, 2022 at 08:08:17AM +0300, Mika Westerberg wrote: > On Tue, May 17, 2022 at 07:38:19PM +0300, Andy Shevchenko wrote: > > +static const struct irq_chip intel_gpio_irq_chip = { > > + .name = "intel-gpio", > > + .irq_ack = intel_gpio_irq_ack, > > + .irq_mask = intel_gpio_irq_mask, > > + .irq_unmask = intel_gpio_irq_unmask, > > + .irq_set_type = intel_gpio_irq_type, > > + .irq_set_wake = intel_gpio_irq_wake, > > + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, > > + GPIOCHIP_IRQ_RESOURCE_HELPERS, > > +}; > > You still have the inconsistent alignment here. I'm not sure what problem do you see. If you are talking about last line, it's special and that's how it's done in other drivers which have been converted already.
On Wed, May 18, 2022 at 11:46:06AM +0300, Andy Shevchenko wrote: > On Wed, May 18, 2022 at 08:08:17AM +0300, Mika Westerberg wrote: > > On Tue, May 17, 2022 at 07:38:19PM +0300, Andy Shevchenko wrote: > > > +static const struct irq_chip intel_gpio_irq_chip = { > > > + .name = "intel-gpio", > > > + .irq_ack = intel_gpio_irq_ack, > > > + .irq_mask = intel_gpio_irq_mask, > > > + .irq_unmask = intel_gpio_irq_unmask, > > > + .irq_set_type = intel_gpio_irq_type, > > > + .irq_set_wake = intel_gpio_irq_wake, > > > + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, > > > + GPIOCHIP_IRQ_RESOURCE_HELPERS, > > > +}; > > > > You still have the inconsistent alignment here. > > I'm not sure what problem do you see. I mean the tab alignment you use: .name = "intel-gpio", .irq_ack = intel_gpio_irq_ack, .irq_mask = intel_gpio_irq_mask, .irq_unmask = intel_gpio_irq_unmask, .irq_set_type = intel_gpio_irq_type, .irq_set_wake = intel_gpio_irq_wake, All the other struct initializations in the driver use this style (and I prefer it too): .name = "intel-gpio", .irq_ack = intel_gpio_irq_ack, .irq_mask = intel_gpio_irq_mask, .irq_unmask = intel_gpio_irq_unmask, .irq_set_type = intel_gpio_irq_type, .irq_set_wake = intel_gpio_irq_wake, Hope this explains.
On Wed, May 18, 2022 at 11:19 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Wed, May 18, 2022 at 11:46:06AM +0300, Andy Shevchenko wrote: > > On Wed, May 18, 2022 at 08:08:17AM +0300, Mika Westerberg wrote: > > > On Tue, May 17, 2022 at 07:38:19PM +0300, Andy Shevchenko wrote: > > > > +static const struct irq_chip intel_gpio_irq_chip = { > > > > + .name = "intel-gpio", > > > > + .irq_ack = intel_gpio_irq_ack, > > > > + .irq_mask = intel_gpio_irq_mask, > > > > + .irq_unmask = intel_gpio_irq_unmask, > > > > + .irq_set_type = intel_gpio_irq_type, > > > > + .irq_set_wake = intel_gpio_irq_wake, > > > > + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, > > > > + GPIOCHIP_IRQ_RESOURCE_HELPERS, > > > > +}; > > > > > > You still have the inconsistent alignment here. > > > > I'm not sure what problem do you see. > > I mean the tab alignment you use: > > .name = "intel-gpio", > .irq_ack = intel_gpio_irq_ack, > .irq_mask = intel_gpio_irq_mask, > .irq_unmask = intel_gpio_irq_unmask, > .irq_set_type = intel_gpio_irq_type, > .irq_set_wake = intel_gpio_irq_wake, > > All the other struct initializations in the driver use this style (and I > prefer it too): > > .name = "intel-gpio", > .irq_ack = intel_gpio_irq_ack, > .irq_mask = intel_gpio_irq_mask, > .irq_unmask = intel_gpio_irq_unmask, > .irq_set_type = intel_gpio_irq_type, > .irq_set_wake = intel_gpio_irq_wake, > > Hope this explains. Yes, thanks! Okay, can you give your conditional Ack if there are no other comments? I will fix it locally.
On Wed, May 18, 2022 at 12:05:38PM +0200, Andy Shevchenko wrote: > On Wed, May 18, 2022 at 11:19 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Wed, May 18, 2022 at 11:46:06AM +0300, Andy Shevchenko wrote: > > > On Wed, May 18, 2022 at 08:08:17AM +0300, Mika Westerberg wrote: > > > > On Tue, May 17, 2022 at 07:38:19PM +0300, Andy Shevchenko wrote: > > > > > +static const struct irq_chip intel_gpio_irq_chip = { > > > > > + .name = "intel-gpio", > > > > > + .irq_ack = intel_gpio_irq_ack, > > > > > + .irq_mask = intel_gpio_irq_mask, > > > > > + .irq_unmask = intel_gpio_irq_unmask, > > > > > + .irq_set_type = intel_gpio_irq_type, > > > > > + .irq_set_wake = intel_gpio_irq_wake, > > > > > + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, > > > > > + GPIOCHIP_IRQ_RESOURCE_HELPERS, > > > > > +}; > > > > > > > > You still have the inconsistent alignment here. > > > > > > I'm not sure what problem do you see. > > > > I mean the tab alignment you use: > > > > .name = "intel-gpio", > > .irq_ack = intel_gpio_irq_ack, > > .irq_mask = intel_gpio_irq_mask, > > .irq_unmask = intel_gpio_irq_unmask, > > .irq_set_type = intel_gpio_irq_type, > > .irq_set_wake = intel_gpio_irq_wake, > > > > All the other struct initializations in the driver use this style (and I > > prefer it too): > > > > .name = "intel-gpio", > > .irq_ack = intel_gpio_irq_ack, > > .irq_mask = intel_gpio_irq_mask, > > .irq_unmask = intel_gpio_irq_unmask, > > .irq_set_type = intel_gpio_irq_type, > > .irq_set_wake = intel_gpio_irq_wake, > > > > Hope this explains. > > Yes, thanks! > > Okay, can you give your conditional Ack if there are no other > comments? I will fix it locally. Sure. There was typo also in the second patch $subject, please fix it too while you apply them. For both, Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Wed, May 18, 2022 at 01:11:42PM +0300, Mika Westerberg wrote: > On Wed, May 18, 2022 at 12:05:38PM +0200, Andy Shevchenko wrote: > > On Wed, May 18, 2022 at 11:19 AM Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > On Wed, May 18, 2022 at 11:46:06AM +0300, Andy Shevchenko wrote: ... > > > Hope this explains. > > > > Yes, thanks! > > > > Okay, can you give your conditional Ack if there are no other > > comments? I will fix it locally. > > Sure. There was typo also in the second patch $subject, please fix it > too while you apply them. Fixed. > For both, > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> Pushed to my review and testing queue, thanks!
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 826d494f3cc6..4845d0b74df9 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1039,15 +1039,14 @@ static void intel_gpio_irq_ack(struct irq_data *d) } } -static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask) +static void intel_gpio_irq_mask_unmask(struct gpio_chip *gc, irq_hw_number_t hwirq, bool mask) { - struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct intel_pinctrl *pctrl = gpiochip_get_data(gc); const struct intel_community *community; const struct intel_padgroup *padgrp; int pin; - pin = intel_gpio_to_pin(pctrl, irqd_to_hwirq(d), &community, &padgrp); + pin = intel_gpio_to_pin(pctrl, hwirq, &community, &padgrp); if (pin >= 0) { unsigned int gpp, gpp_offset; unsigned long flags; @@ -1077,12 +1076,20 @@ static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask) static void intel_gpio_irq_mask(struct irq_data *d) { - intel_gpio_irq_mask_unmask(d, true); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + + intel_gpio_irq_mask_unmask(gc, hwirq, true); + gpiochip_disable_irq(gc, hwirq); } static void intel_gpio_irq_unmask(struct irq_data *d) { - intel_gpio_irq_mask_unmask(d, false); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + + gpiochip_enable_irq(gc, hwirq); + intel_gpio_irq_mask_unmask(gc, hwirq, false); } static int intel_gpio_irq_type(struct irq_data *d, unsigned int type) @@ -1157,6 +1164,17 @@ static int intel_gpio_irq_wake(struct irq_data *d, unsigned int on) return 0; } +static const struct irq_chip intel_gpio_irq_chip = { + .name = "intel-gpio", + .irq_ack = intel_gpio_irq_ack, + .irq_mask = intel_gpio_irq_mask, + .irq_unmask = intel_gpio_irq_unmask, + .irq_set_type = intel_gpio_irq_type, + .irq_set_wake = intel_gpio_irq_wake, + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + static int intel_gpio_community_irq_handler(struct intel_pinctrl *pctrl, const struct intel_community *community) { @@ -1319,15 +1337,6 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq) pctrl->chip.add_pin_ranges = intel_gpio_add_pin_ranges; pctrl->irq = irq; - /* Setup IRQ chip */ - pctrl->irqchip.name = dev_name(pctrl->dev); - pctrl->irqchip.irq_ack = intel_gpio_irq_ack; - pctrl->irqchip.irq_mask = intel_gpio_irq_mask; - pctrl->irqchip.irq_unmask = intel_gpio_irq_unmask; - pctrl->irqchip.irq_set_type = intel_gpio_irq_type; - pctrl->irqchip.irq_set_wake = intel_gpio_irq_wake; - pctrl->irqchip.flags = IRQCHIP_MASK_ON_SUSPEND; - /* * On some platforms several GPIO controllers share the same interrupt * line. @@ -1340,8 +1349,9 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq) return ret; } + /* Setup IRQ chip */ girq = &pctrl->chip.irq; - girq->chip = &pctrl->irqchip; + gpio_irq_chip_set_chip(girq, &intel_gpio_irq_chip); /* This will let us handle the IRQ in the driver */ girq->parent_handler = NULL; girq->num_parents = 0;
Since recently, the kernel is nagging about mutable irq_chips: "not an immutable chip, please consider fixing it!" Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new helper functions and call the appropriate gpiolib functions. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v2: dropped renaming part (Mika) drivers/pinctrl/intel/pinctrl-intel.c | 40 +++++++++++++++++---------- 1 file changed, 25 insertions(+), 15 deletions(-)