All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] pinctrl: ingenic: Convert to immutable irq chip
@ 2022-06-07 11:05 Aidan MacDonald
  2022-06-07 16:26 ` Paul Cercueil
  2022-06-10 14:14 ` Andy Shevchenko
  0 siblings, 2 replies; 8+ messages in thread
From: Aidan MacDonald @ 2022-06-07 11:05 UTC (permalink / raw)
  To: paul, linus.walleij; +Cc: linux-mips, linux-gpio, linux-kernel

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);
 	girq->parent_handler = ingenic_gpio_irq_handler;
 	girq->num_parents = 1;
 	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip
  2022-06-07 11:05 [PATCH] pinctrl: ingenic: Convert to immutable irq chip Aidan MacDonald
@ 2022-06-07 16:26 ` Paul Cercueil
  2022-06-07 16:47   ` Aidan MacDonald
  2022-06-10 14:14 ` Andy Shevchenko
  1 sibling, 1 reply; 8+ messages in thread
From: Paul Cercueil @ 2022-06-07 16:26 UTC (permalink / raw)
  To: Aidan MacDonald; +Cc: linus.walleij, linux-mips, linux-gpio, linux-kernel

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

>  	girq->parent_handler = ingenic_gpio_irq_handler;
>  	girq->num_parents = 1;
>  	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
> --
> 2.35.1
> 



^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip
  2022-06-07 16:26 ` Paul Cercueil
@ 2022-06-07 16:47   ` Aidan MacDonald
  2022-06-09 10:00     ` Paul Cercueil
  0 siblings, 1 reply; 8+ messages in thread
From: Aidan MacDonald @ 2022-06-07 16:47 UTC (permalink / raw)
  To: Paul Cercueil; +Cc: linus.walleij, linux-mips, linux-gpio, linux-kernel


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?

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

(btw, I did remove jzgc->irq_chip -- or did I miss something?)

Best regards,
Aidan

>>  	girq->parent_handler = ingenic_gpio_irq_handler;
>>  	girq->num_parents = 1;
>>  	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
>> --
>> 2.35.1
>> 


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip
  2022-06-07 16:47   ` Aidan MacDonald
@ 2022-06-09 10:00     ` Paul Cercueil
  2022-06-09 12:08       ` Marc Zyngier
  0 siblings, 1 reply; 8+ messages in thread
From: Paul Cercueil @ 2022-06-09 10:00 UTC (permalink / raw)
  To: Aidan MacDonald; +Cc: linus.walleij, linux-mips, linux-gpio, linux-kernel

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.

> (btw, I did remove jzgc->irq_chip -- or did I miss something?)

Oops, missed that.

Cheers,
-Paul

> Best regards,
> Aidan
> 
>>>   	girq->parent_handler = ingenic_gpio_irq_handler;
>>>   	girq->num_parents = 1;
>>>   	girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
>>>  --
>>>  2.35.1
>>> 
> 



^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip
  2022-06-09 10:00     ` Paul Cercueil
@ 2022-06-09 12:08       ` Marc Zyngier
  2022-06-09 12:36         ` Paul Cercueil
  2022-06-09 17:45         ` Aidan MacDonald
  0 siblings, 2 replies; 8+ messages in thread
From: Marc Zyngier @ 2022-06-09 12:08 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Aidan MacDonald, linus.walleij, linux-mips, linux-gpio, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip
  2022-06-09 12:08       ` Marc Zyngier
@ 2022-06-09 12:36         ` Paul Cercueil
  2022-06-09 17:45         ` Aidan MacDonald
  1 sibling, 0 replies; 8+ messages in thread
From: Paul Cercueil @ 2022-06-09 12:36 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Aidan MacDonald, linus.walleij, linux-mips, linux-gpio, linux-kernel

Hi Marc,

Le jeu., juin 9 2022 at 13:08:53 +0100, Marc Zyngier <maz@kernel.org> a 
écrit :
> 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 
>>>>> \x7f\x7f\x7f\x7f++++++++++++++++++-------------
>>>>>   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 
>>>>> \x7f\x7f\x7f\x7firq_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 \x7f\x7f\x7f\x7firq_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 \x7f\x7f\x7f\x7firq_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 \x7f\x7fbe
>>> 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.

Works for me.

Cheers,
-Paul



^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip
  2022-06-09 12:08       ` Marc Zyngier
  2022-06-09 12:36         ` Paul Cercueil
@ 2022-06-09 17:45         ` Aidan MacDonald
  1 sibling, 0 replies; 8+ messages in thread
From: Aidan MacDonald @ 2022-06-09 17:45 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Paul Cercueil, linus.walleij, linux-mips, linux-gpio, linux-kernel


Marc Zyngier <maz@kernel.org> writes:

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

Thanks for the tip! I'll do that and send a v2.

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Convert to immutable irq chip
  2022-06-07 11:05 [PATCH] pinctrl: ingenic: Convert to immutable irq chip Aidan MacDonald
  2022-06-07 16:26 ` Paul Cercueil
@ 2022-06-10 14:14 ` Andy Shevchenko
  1 sibling, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2022-06-10 14:14 UTC (permalink / raw)
  To: Aidan MacDonald
  Cc: Paul Cercueil, Linus Walleij, open list:BROADCOM NVRAM DRIVER,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List

On Tue, Jun 7, 2022 at 5:53 PM Aidan MacDonald
<aidanmacdonald.0x0@gmail.com> wrote:
>
> Update the driver to use an immutable IRQ chip to fix this warning:
>
>     "not an immutable chip, please consider fixing it!"

...

>         int irq = irqd->hwirq;

Consider switching to irq_hw_number_t with irqd_to_hwirq() (in a
separate patch) as pointed out in the documentation.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-06-10 14:15 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-07 11:05 [PATCH] pinctrl: ingenic: Convert to immutable irq chip Aidan MacDonald
2022-06-07 16:26 ` Paul Cercueil
2022-06-07 16:47   ` Aidan MacDonald
2022-06-09 10:00     ` Paul Cercueil
2022-06-09 12:08       ` Marc Zyngier
2022-06-09 12:36         ` Paul Cercueil
2022-06-09 17:45         ` Aidan MacDonald
2022-06-10 14:14 ` Andy Shevchenko

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.