Skip to contentSkip to conversation

pinctrl: intel: make irq_chip immutable

PATCH In review Andy Shevchenko 3 participants 8 messages May 17, 2022 – May 18, 2022

Subsystems (3)
GPIO Pin control Pin controller - Intel
lore
Share & tools
lore

Andy Shevchenko

author
#0 lore raw

PATCH v2 1/2 pinctrl: intel: make irq_chip immutable

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 <[email protected]>

---
v2: dropped renaming part (Mika)

 drivers/pinctrl/intel/pinctrl-intel.c | 40 +++++++++++++++++----------
 1 file changed, 25 insertions(+), 15 deletions(-)
drivers/pinctrl/intel/pinctrl-intel.c +25 -15
index 826d494f3cc6..4845d0b74df9 100644
@@ -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;
Signature · 1 line
2.35.1
author
#1 lore raw

PATCH v2 2/2 pinctrl: intel: Drop unsued irqchip member in struct intel_pinctrl

There is no users of irqchip member in struct intel_pinctrl. Drop it.

Signed-off-by Andy Shevchenko <[email protected]>

---
v2: no changes

 drivers/pinctrl/intel/pinctrl-intel.h | 2 --
 1 file changed, 2 deletions(-)
drivers/pinctrl/intel/pinctrl-intel.h +0 -2
index c4fef03b663f..710341bb67cc 100644
@@ -223,7 +223,6 @@ struct intel_pinctrl_context {
  * @pctldesc: Pin controller description
  * @pctldev: Pointer to the pin controller device
  * @chip: GPIO chip in this pin controller
- * @irqchip: IRQ chip in this pin controller
  * @soc: SoC/PCH specific pin configuration data
  * @communities: All communities in this pin controller
  * @ncommunities: Number of communities in this pin controller

@@ -236,7 +235,6 @@ struct intel_pinctrl {
 	struct pinctrl_desc pctldesc;
 	struct pinctrl_dev *pctldev;
 	struct gpio_chip chip;
-	struct irq_chip irqchip;
 	const struct intel_pinctrl_soc_data *soc;
 	struct intel_community *communities;
 	size_t ncommunities;
Signature · 1 line
2.35.1
maintainer
#2 lore raw
In reply to Andy Shevchenko

drivers/pinctrl/intel/pinctrl-intel.c static int intel_gpio_irq_wake(struct irq_data *d, unsigned int on) · line 1176

Quoted 10 lines · Andy Shevchenko
drivers/pinctrl/intel/pinctrl-intel.c +10 -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,
+};

You still have the inconsistent alignment here.

author
#3 lore raw
Quoted 13 lines · Mika Westerberg

drivers/pinctrl/intel/pinctrl-intel.c static int intel_gpio_irq_wake(struct irq_data *d, unsigned int on) · line 1176

Quoted 10 lines · Andy Shevchenko
drivers/pinctrl/intel/pinctrl-intel.c +10 -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,
+};

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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#4 lore raw
Quoted 15 lines · Andy Shevchenko
Quoted 13 lines · Mika Westerberg

drivers/pinctrl/intel/pinctrl-intel.c static int intel_gpio_irq_wake(struct irq_data *d, unsigned int on) · line 1176

Quoted 10 lines · Andy Shevchenko
drivers/pinctrl/intel/pinctrl-intel.c +10 -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,
+};

You still have the inconsistent alignment here.

Quoted 1 line

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.

#5 lore raw
Quoted 30 lines · Mika Westerberg
Quoted 15 lines · Andy Shevchenko
Quoted 13 lines · Mika Westerberg

drivers/pinctrl/intel/pinctrl-intel.c static int intel_gpio_irq_wake(struct irq_data *d, unsigned int on) · line 1176

Quoted 10 lines · Andy Shevchenko
drivers/pinctrl/intel/pinctrl-intel.c +10 -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,
+};

You still have the inconsistent alignment here.

Quoted 1 line

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):

Quoted 8 lines

.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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer Acked-by
#6 lore raw
Quoted 39 lines · Andy Shevchenko
Quoted 37 lines · Mika Westerberg
Quoted 15 lines · Andy Shevchenko
Quoted 13 lines · Mika Westerberg

drivers/pinctrl/intel/pinctrl-intel.c static int intel_gpio_irq_wake(struct irq_data *d, unsigned int on) · line 1176

Quoted 10 lines · Andy Shevchenko
drivers/pinctrl/intel/pinctrl-intel.c +10 -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,
+};

You still have the inconsistent alignment here.

Quoted 1 line

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,

Quoted 6 lines
Quoted 1 line

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 <[email protected]>
#7 lore raw
Quoted 4 lines · Mika Westerberg
Quoted 3 lines · Andy Shevchenko
Quoted 1 line · Mika Westerberg

On Wed, May 18, 2022 at 11:46:06AM +0300, Andy Shevchenko wrote:

...

Quoted 9 lines
Quoted 6 lines
Quoted 1 line

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.

Quoted 2 lines

For both,

Acked-by Mika Westerberg <[email protected]>

Pushed to my review and testing queue, thanks!

Signature · 2 lines
With Best Regards,
Andy Shevchenko