All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] pinctrl: nomadik: Make gpio irqchip immutable
@ 2022-09-17 13:19 Linus Walleij
  2022-09-17 16:27 ` Marc Zyngier
  0 siblings, 1 reply; 2+ messages in thread
From: Linus Walleij @ 2022-09-17 13:19 UTC (permalink / raw)
  To: linux-gpio; +Cc: Linus Walleij, Marc Zyngier

This makes the Nomadik GPIO irqchip immutable.

Tested on the Samsung Galaxy SIII mini GT-I8190.

Cc: Marc Zyngier <maz@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/nomadik/pinctrl-nomadik.c | 51 +++++++++++------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
index f5014d09d81a..1ee3b45dd6bc 100644
--- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c
+++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
@@ -244,7 +244,6 @@ enum nmk_gpio_slpm {
 
 struct nmk_gpio_chip {
 	struct gpio_chip chip;
-	struct irq_chip irqchip;
 	void __iomem *addr;
 	struct clk *clk;
 	unsigned int bank;
@@ -675,15 +674,11 @@ static void __nmk_gpio_set_wake(struct nmk_gpio_chip *nmk_chip,
 	__nmk_gpio_irq_modify(nmk_chip, offset, WAKE, on);
 }
 
-static int nmk_gpio_irq_maskunmask(struct irq_data *d, bool enable)
+static void nmk_gpio_irq_maskunmask(struct nmk_gpio_chip *nmk_chip,
+				    struct irq_data *d, bool enable)
 {
-	struct nmk_gpio_chip *nmk_chip;
 	unsigned long flags;
 
-	nmk_chip = irq_data_get_irq_chip_data(d);
-	if (!nmk_chip)
-		return -EINVAL;
-
 	clk_enable(nmk_chip->clk);
 	spin_lock_irqsave(&nmk_gpio_slpm_lock, flags);
 	spin_lock(&nmk_chip->lock);
@@ -696,18 +691,22 @@ static int nmk_gpio_irq_maskunmask(struct irq_data *d, bool enable)
 	spin_unlock(&nmk_chip->lock);
 	spin_unlock_irqrestore(&nmk_gpio_slpm_lock, flags);
 	clk_disable(nmk_chip->clk);
-
-	return 0;
 }
 
 static void nmk_gpio_irq_mask(struct irq_data *d)
 {
-	nmk_gpio_irq_maskunmask(d, false);
+	struct nmk_gpio_chip *nmk_chip = irq_data_get_irq_chip_data(d);
+
+	nmk_gpio_irq_maskunmask(nmk_chip, d, false);
+	gpiochip_disable_irq(&nmk_chip->chip, irqd_to_hwirq(d));
 }
 
 static void nmk_gpio_irq_unmask(struct irq_data *d)
 {
-	nmk_gpio_irq_maskunmask(d, true);
+	struct nmk_gpio_chip *nmk_chip = irq_data_get_irq_chip_data(d);
+
+	gpiochip_enable_irq(&nmk_chip->chip, irqd_to_hwirq(d));
+	nmk_gpio_irq_maskunmask(nmk_chip, d, true);
 }
 
 static int nmk_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
@@ -1078,13 +1077,25 @@ static struct nmk_gpio_chip *nmk_gpio_populate_chip(struct device_node *np,
 	return nmk_chip;
 }
 
+static const struct irq_chip nmk_irq_chip = {
+	.name = "nomadik-gpio",
+	.irq_ack = nmk_gpio_irq_ack,
+	.irq_mask = nmk_gpio_irq_mask,
+	.irq_unmask = nmk_gpio_irq_unmask,
+	.irq_set_type = nmk_gpio_irq_set_type,
+	.irq_set_wake = nmk_gpio_irq_set_wake,
+	.irq_startup = nmk_gpio_irq_startup,
+	.irq_shutdown = nmk_gpio_irq_shutdown,
+	.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
 static int nmk_gpio_probe(struct platform_device *dev)
 {
 	struct device_node *np = dev->dev.of_node;
 	struct nmk_gpio_chip *nmk_chip;
 	struct gpio_chip *chip;
 	struct gpio_irq_chip *girq;
-	struct irq_chip *irqchip;
 	bool supports_sleepmode;
 	int irq;
 	int ret;
@@ -1125,22 +1136,8 @@ static int nmk_gpio_probe(struct platform_device *dev)
 	chip->can_sleep = false;
 	chip->owner = THIS_MODULE;
 
-	irqchip = &nmk_chip->irqchip;
-	irqchip->irq_ack = nmk_gpio_irq_ack;
-	irqchip->irq_mask = nmk_gpio_irq_mask;
-	irqchip->irq_unmask = nmk_gpio_irq_unmask;
-	irqchip->irq_set_type = nmk_gpio_irq_set_type;
-	irqchip->irq_set_wake = nmk_gpio_irq_set_wake;
-	irqchip->irq_startup = nmk_gpio_irq_startup;
-	irqchip->irq_shutdown = nmk_gpio_irq_shutdown;
-	irqchip->flags = IRQCHIP_MASK_ON_SUSPEND;
-	irqchip->name = kasprintf(GFP_KERNEL, "nmk%u-%u-%u",
-				  dev->id,
-				  chip->base,
-				  chip->base + chip->ngpio - 1);
-
 	girq = &chip->irq;
-	girq->chip = irqchip;
+	gpio_irq_chip_set_chip(girq, &nmk_irq_chip);
 	girq->parent_handler = nmk_gpio_irq_handler;
 	girq->num_parents = 1;
 	girq->parents = devm_kcalloc(&dev->dev, 1,
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] pinctrl: nomadik: Make gpio irqchip immutable
  2022-09-17 13:19 [PATCH] pinctrl: nomadik: Make gpio irqchip immutable Linus Walleij
@ 2022-09-17 16:27 ` Marc Zyngier
  0 siblings, 0 replies; 2+ messages in thread
From: Marc Zyngier @ 2022-09-17 16:27 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-gpio

On Sat, 17 Sep 2022 14:19:07 +0100,
Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> This makes the Nomadik GPIO irqchip immutable.
> 
> Tested on the Samsung Galaxy SIII mini GT-I8190.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/pinctrl/nomadik/pinctrl-nomadik.c | 51 +++++++++++------------
>  1 file changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
> index f5014d09d81a..1ee3b45dd6bc 100644
> --- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c
> +++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
> @@ -244,7 +244,6 @@ enum nmk_gpio_slpm {
>  
>  struct nmk_gpio_chip {
>  	struct gpio_chip chip;
> -	struct irq_chip irqchip;
>  	void __iomem *addr;
>  	struct clk *clk;
>  	unsigned int bank;
> @@ -675,15 +674,11 @@ static void __nmk_gpio_set_wake(struct nmk_gpio_chip *nmk_chip,
>  	__nmk_gpio_irq_modify(nmk_chip, offset, WAKE, on);
>  }
>  
> -static int nmk_gpio_irq_maskunmask(struct irq_data *d, bool enable)
> +static void nmk_gpio_irq_maskunmask(struct nmk_gpio_chip *nmk_chip,
> +				    struct irq_data *d, bool enable)
>  {
> -	struct nmk_gpio_chip *nmk_chip;
>  	unsigned long flags;
>  
> -	nmk_chip = irq_data_get_irq_chip_data(d);
> -	if (!nmk_chip)
> -		return -EINVAL;
> -
>  	clk_enable(nmk_chip->clk);
>  	spin_lock_irqsave(&nmk_gpio_slpm_lock, flags);
>  	spin_lock(&nmk_chip->lock);
> @@ -696,18 +691,22 @@ static int nmk_gpio_irq_maskunmask(struct irq_data *d, bool enable)
>  	spin_unlock(&nmk_chip->lock);
>  	spin_unlock_irqrestore(&nmk_gpio_slpm_lock, flags);
>  	clk_disable(nmk_chip->clk);
> -
> -	return 0;
>  }
>  
>  static void nmk_gpio_irq_mask(struct irq_data *d)
>  {
> -	nmk_gpio_irq_maskunmask(d, false);
> +	struct nmk_gpio_chip *nmk_chip = irq_data_get_irq_chip_data(d);

As far as I can tell, the per-chip data for a GPIO chip points to the
gpio_chip, and not the driver-specific view that contains the generic
structure. Here you're lucky that they are the same address as the
gpio_chip is the first one. This is the same bug as the one fixed by
d1e972ace423 ("gpio: tegra186: Fix chip_data type confusion").

> +
> +	nmk_gpio_irq_maskunmask(nmk_chip, d, false);
> +	gpiochip_disable_irq(&nmk_chip->chip, irqd_to_hwirq(d));
>  }
>  
>  static void nmk_gpio_irq_unmask(struct irq_data *d)
>  {
> -	nmk_gpio_irq_maskunmask(d, true);
> +	struct nmk_gpio_chip *nmk_chip = irq_data_get_irq_chip_data(d);
> +
> +	gpiochip_enable_irq(&nmk_chip->chip, irqd_to_hwirq(d));
> +	nmk_gpio_irq_maskunmask(nmk_chip, d, true);
>  }
>  
>  static int nmk_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
> @@ -1078,13 +1077,25 @@ static struct nmk_gpio_chip *nmk_gpio_populate_chip(struct device_node *np,
>  	return nmk_chip;
>  }
>  
> +static const struct irq_chip nmk_irq_chip = {
> +	.name = "nomadik-gpio",

Maybe not, see below.

> +	.irq_ack = nmk_gpio_irq_ack,
> +	.irq_mask = nmk_gpio_irq_mask,
> +	.irq_unmask = nmk_gpio_irq_unmask,
> +	.irq_set_type = nmk_gpio_irq_set_type,
> +	.irq_set_wake = nmk_gpio_irq_set_wake,
> +	.irq_startup = nmk_gpio_irq_startup,
> +	.irq_shutdown = nmk_gpio_irq_shutdown,
> +	.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
> +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};
> +
>  static int nmk_gpio_probe(struct platform_device *dev)
>  {
>  	struct device_node *np = dev->dev.of_node;
>  	struct nmk_gpio_chip *nmk_chip;
>  	struct gpio_chip *chip;
>  	struct gpio_irq_chip *girq;
> -	struct irq_chip *irqchip;
>  	bool supports_sleepmode;
>  	int irq;
>  	int ret;
> @@ -1125,22 +1136,8 @@ static int nmk_gpio_probe(struct platform_device *dev)
>  	chip->can_sleep = false;
>  	chip->owner = THIS_MODULE;
>  
> -	irqchip = &nmk_chip->irqchip;
> -	irqchip->irq_ack = nmk_gpio_irq_ack;
> -	irqchip->irq_mask = nmk_gpio_irq_mask;
> -	irqchip->irq_unmask = nmk_gpio_irq_unmask;
> -	irqchip->irq_set_type = nmk_gpio_irq_set_type;
> -	irqchip->irq_set_wake = nmk_gpio_irq_set_wake;
> -	irqchip->irq_startup = nmk_gpio_irq_startup;
> -	irqchip->irq_shutdown = nmk_gpio_irq_shutdown;
> -	irqchip->flags = IRQCHIP_MASK_ON_SUSPEND;
> -	irqchip->name = kasprintf(GFP_KERNEL, "nmk%u-%u-%u",
> -				  dev->id,
> -				  chip->base,
> -				  chip->base + chip->ngpio - 1);
> -

Dropping this is unfortunately a userspace visible change. I'm 99%
sure nobody will ever care, but we've been burned by the remaining 1%,
and I'd rather you keep it using the irq_print_chip() callback with
something like:

static void nmk_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
{
	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
	struct nmk_gpio_chip *nmk_chip = container_of(chip,
						      struct nmk_gpio_chip,
						      chip);
	seq_printf(p, "nmk%u-%u-%u", nmk_chip->bank,
		   chip->base, chip->base + chip->ngpio - 1);
}

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-09-17 16:27 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-17 13:19 [PATCH] pinctrl: nomadik: Make gpio irqchip immutable Linus Walleij
2022-09-17 16:27 ` Marc Zyngier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.