- optimizes and cleans up stm32-exti irq domain
- optimizes and enhances stm32gpio irq chip
Radoslaw Pietrzyk (2):
ARM: irqchip: stm32: Optimizes and cleans up stm32-exti irq_domain
ARM: pinctrl: stm32: Optimizes and enhances stm32gpio irqchip
drivers/irqchip/irq-stm32-exti.c | 10 +---------
drivers/pinctrl/stm32/pinctrl-stm32.c | 2 +-
2 files changed, 2 insertions(+), 10 deletions(-)
--
1.9.1
- removes unneeded irq_chip.irq_eoi callback
- adds irq_chip.irq_set_wake callback for possible
in the future GPIO wakeup
Signed-off-by: Radoslaw Pietrzyk <[email protected]>
---
drivers/pinctrl/stm32/pinctrl-stm32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 617df16..5b1740d 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -268,10 +268,10 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
static struct irq_chip stm32_gpio_irq_chip = {
.name = "stm32gpio",
- .irq_eoi = irq_chip_eoi_parent,
.irq_mask = irq_chip_mask_parent,
.irq_unmask = irq_chip_unmask_parent,
.irq_set_type = irq_chip_set_type_parent,
+ .irq_set_wake = irq_chip_set_wake_parent,
.irq_request_resources = stm32_gpio_irq_request_resources,
.irq_release_resources = stm32_gpio_irq_release_resources,
};
--
1.9.1
Maybe you could remove ARM from commit header.
On 02/21/2018 02:50 PM, Radoslaw Pietrzyk wrote:
> - removes unneeded irq_chip.irq_eoi callback
> - adds irq_chip.irq_set_wake callback for possible
> in the future GPIO wakeup
>
> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
> ---
> drivers/pinctrl/stm32/pinctrl-stm32.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 617df16..5b1740d 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -268,10 +268,10 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>
> static struct irq_chip stm32_gpio_irq_chip = {
> .name = "stm32gpio",
> - .irq_eoi = irq_chip_eoi_parent,
> .irq_mask = irq_chip_mask_parent,
> .irq_unmask = irq_chip_unmask_parent,
> .irq_set_type = irq_chip_set_type_parent,
> + .irq_set_wake = irq_chip_set_wake_parent,
> .irq_request_resources = stm32_gpio_irq_request_resources,
> .irq_release_resources = stm32_gpio_irq_release_resources,
> };
>
- allocates generic chip with handle_simple_irq
which simplifies irq_domain_ops.alloc callback
- removes ack register from generic chip which is not used for
handle_simple_irq as acking is done in a chained handler
- removes unneeded irq_domain_ops.xlate callback
Signed-off-by: Radoslaw Pietrzyk <[email protected]>
---
drivers/irqchip/irq-stm32-exti.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/irqchip/irq-stm32-exti.c b/drivers/irqchip/irq-stm32-exti.c
index 36f0fbe..42e74e3 100644
--- a/drivers/irqchip/irq-stm32-exti.c
+++ b/drivers/irqchip/irq-stm32-exti.c
@@ -176,16 +176,12 @@ static int stm32_irq_set_wake(struct irq_data *data, unsigned int on)
static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq,
unsigned int nr_irqs, void *data)
{
- struct irq_chip_generic *gc;
struct irq_fwspec *fwspec = data;
irq_hw_number_t hwirq;
hwirq = fwspec->param[0];
- gc = irq_get_domain_generic_chip(d, hwirq);
irq_map_generic_chip(d, virq, hwirq);
- irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc,
- handle_simple_irq, NULL, NULL);
return 0;
}
@@ -200,7 +196,6 @@ static void stm32_exti_free(struct irq_domain *d, unsigned int virq,
struct irq_domain_ops irq_exti_domain_ops = {
.map = irq_map_generic_chip,
- .xlate = irq_domain_xlate_onetwocell,
.alloc = stm32_exti_alloc,
.free = stm32_exti_free,
};
@@ -231,7 +226,7 @@ __init stm32_exti_init(const struct stm32_exti_bank **stm32_exti_banks,
}
ret = irq_alloc_domain_generic_chips(domain, IRQS_PER_BANK, 1, "exti",
- handle_edge_irq, clr, 0, 0);
+ handle_simple_irq, clr, 0, 0);
if (ret) {
pr_err("%pOF: Could not allocate generic interrupt chip.\n",
node);
@@ -246,13 +241,10 @@ __init stm32_exti_init(const struct stm32_exti_bank **stm32_exti_banks,
gc->reg_base = base;
gc->chip_types->type = IRQ_TYPE_EDGE_BOTH;
- gc->chip_types->chip.irq_ack = irq_gc_ack_set_bit;
gc->chip_types->chip.irq_mask = irq_gc_mask_clr_bit;
gc->chip_types->chip.irq_unmask = irq_gc_mask_set_bit;
gc->chip_types->chip.irq_set_type = stm32_irq_set_type;
gc->chip_types->chip.irq_set_wake = stm32_irq_set_wake;
- gc->chip_types->regs.ack = stm32_bank->pr_ofst;
- gc->chip_types->regs.mask = stm32_bank->imr_ofst;
gc->private = (void *)stm32_bank;
/* Determine number of irqs supported */
--
1.9.1
Hi Radoslaw,
On 02/21/2018 02:50 PM, Radoslaw Pietrzyk wrote:
> - removes unneeded irq_chip.irq_eoi callback
> - adds irq_chip.irq_set_wake callback for possible
> in the future GPIO wakeup
>
> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
> ---
> drivers/pinctrl/stm32/pinctrl-stm32.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 617df16..5b1740d 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -268,10 +268,10 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>
> static struct irq_chip stm32_gpio_irq_chip = {
> .name = "stm32gpio",
> - .irq_eoi = irq_chip_eoi_parent,
> .irq_mask = irq_chip_mask_parent,
> .irq_unmask = irq_chip_unmask_parent,
> .irq_set_type = irq_chip_set_type_parent,
> + .irq_set_wake = irq_chip_set_wake_parent,
Thanks, this one was in my backlog.
Acked-by: Alexandre TORGUE <[email protected]>
> .irq_request_resources = stm32_gpio_irq_request_resources,
> .irq_release_resources = stm32_gpio_irq_release_resources,
> };
>
Hi Radek
I've tested your change on stm32h743i-eval.dts board
and gpio-keys tests are not functional.
No interrupt occurs when we push or release button
(cat /proc/interrupt).
Board: stm32h743i-eval.dts
Test description: gpio-keys (gpioc 13 => exti 13), with gpio or
interrupt property.
comment below
On 02/21/2018 02:50 PM, radek wrote:
> - allocates generic chip with handle_simple_irq
> which simplifies irq_domain_ops.alloc callback
> - removes ack register from generic chip which is not used for
> handle_simple_irq as acking is done in a chained handler
> - removes unneeded irq_domain_ops.xlate callback
>
> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
> ---
> drivers/irqchip/irq-stm32-exti.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/irqchip/irq-stm32-exti.c b/drivers/irqchip/irq-stm32-exti.c
> index 36f0fbe..42e74e3 100644
> --- a/drivers/irqchip/irq-stm32-exti.c
> +++ b/drivers/irqchip/irq-stm32-exti.c
> @@ -176,16 +176,12 @@ static int stm32_irq_set_wake(struct irq_data *data, unsigned int on)
> static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq,
> unsigned int nr_irqs, void *data)
> {
> - struct irq_chip_generic *gc;
> struct irq_fwspec *fwspec = data;
> irq_hw_number_t hwirq;
>
> hwirq = fwspec->param[0];
> - gc = irq_get_domain_generic_chip(d, hwirq);
>
> irq_map_generic_chip(d, virq, hwirq);
> - irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc,
> - handle_simple_irq, NULL, NULL);
>
> return 0;
> }
> @@ -200,7 +196,6 @@ static void stm32_exti_free(struct irq_domain *d, unsigned int virq,
>
> struct irq_domain_ops irq_exti_domain_ops = {
> .map = irq_map_generic_chip,
> - .xlate = irq_domain_xlate_onetwocell,
> .alloc = stm32_exti_alloc,
> .free = stm32_exti_free,
> };
> @@ -231,7 +226,7 @@ __init stm32_exti_init(const struct stm32_exti_bank **stm32_exti_banks,
> }
>
> ret = irq_alloc_domain_generic_chips(domain, IRQS_PER_BANK, 1, "exti",
> - handle_edge_irq, clr, 0, 0);
> + handle_simple_irq, clr, 0, 0);
EXTI hardware block is trigged on pulse event rising or failing edge.
Why do you change to handle_simple_irq ?
> if (ret) {
> pr_err("%pOF: Could not allocate generic interrupt chip.\n",
> node);
> @@ -246,13 +241,10 @@ __init stm32_exti_init(const struct stm32_exti_bank **stm32_exti_banks,
>
> gc->reg_base = base;
> gc->chip_types->type = IRQ_TYPE_EDGE_BOTH;
> - gc->chip_types->chip.irq_ack = irq_gc_ack_set_bit;
> gc->chip_types->chip.irq_mask = irq_gc_mask_clr_bit;
> gc->chip_types->chip.irq_unmask = irq_gc_mask_set_bit;
> gc->chip_types->chip.irq_set_type = stm32_irq_set_type;
> gc->chip_types->chip.irq_set_wake = stm32_irq_set_wake;
> - gc->chip_types->regs.ack = stm32_bank->pr_ofst;
> - gc->chip_types->regs.mask = stm32_bank->imr_ofst;
> gc->private = (void *)stm32_bank;
>
> /* Determine number of irqs supported */
>
BR
Ludo
Hi Ludovic,
I have tested on stm32f429-disco and it works without a problem.
handle_edge_irq is not used in current implementation because it is
substituted by handle_simple_irq in an alloc callback either way which
causes that irq_ack callback is not invoked as well (acking is done in
chained handler).
2018-02-22 11:55 GMT+01:00 Ludovic BARRE <[email protected]>:
> Hi Radek
>
> I've tested your change on stm32h743i-eval.dts board
> and gpio-keys tests are not functional.
> No interrupt occurs when we push or release button
> (cat /proc/interrupt).
>
> Board: stm32h743i-eval.dts
> Test description: gpio-keys (gpioc 13 => exti 13), with gpio or interrupt
> property.
>
> comment below
>
>
> On 02/21/2018 02:50 PM, radek wrote:
>>
>> - allocates generic chip with handle_simple_irq
>> which simplifies irq_domain_ops.alloc callback
>> - removes ack register from generic chip which is not used for
>> handle_simple_irq as acking is done in a chained handler
>> - removes unneeded irq_domain_ops.xlate callback
>>
>> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
>> ---
>> drivers/irqchip/irq-stm32-exti.c | 10 +---------
>> 1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-stm32-exti.c
>> b/drivers/irqchip/irq-stm32-exti.c
>> index 36f0fbe..42e74e3 100644
>> --- a/drivers/irqchip/irq-stm32-exti.c
>> +++ b/drivers/irqchip/irq-stm32-exti.c
>> @@ -176,16 +176,12 @@ static int stm32_irq_set_wake(struct irq_data *data,
>> unsigned int on)
>> static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq,
>> unsigned int nr_irqs, void *data)
>> {
>> - struct irq_chip_generic *gc;
>> struct irq_fwspec *fwspec = data;
>> irq_hw_number_t hwirq;
>> hwirq = fwspec->param[0];
>> - gc = irq_get_domain_generic_chip(d, hwirq);
>> irq_map_generic_chip(d, virq, hwirq);
>> - irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc,
>> - handle_simple_irq, NULL, NULL);
>> return 0;
>> }
>> @@ -200,7 +196,6 @@ static void stm32_exti_free(struct irq_domain *d,
>> unsigned int virq,
>> struct irq_domain_ops irq_exti_domain_ops = {
>> .map = irq_map_generic_chip,
>> - .xlate = irq_domain_xlate_onetwocell,
>> .alloc = stm32_exti_alloc,
>> .free = stm32_exti_free,
>> };
>> @@ -231,7 +226,7 @@ __init stm32_exti_init(const struct stm32_exti_bank
>> **stm32_exti_banks,
>> }
>> ret = irq_alloc_domain_generic_chips(domain, IRQS_PER_BANK, 1,
>> "exti",
>> - handle_edge_irq, clr, 0, 0);
>> + handle_simple_irq, clr, 0,
>> 0);
>
>
> EXTI hardware block is trigged on pulse event rising or failing edge.
> Why do you change to handle_simple_irq ?
>
>> if (ret) {
>> pr_err("%pOF: Could not allocate generic interrupt
>> chip.\n",
>> node);
>> @@ -246,13 +241,10 @@ __init stm32_exti_init(const struct stm32_exti_bank
>> **stm32_exti_banks,
>> gc->reg_base = base;
>> gc->chip_types->type = IRQ_TYPE_EDGE_BOTH;
>> - gc->chip_types->chip.irq_ack = irq_gc_ack_set_bit;
>> gc->chip_types->chip.irq_mask = irq_gc_mask_clr_bit;
>> gc->chip_types->chip.irq_unmask = irq_gc_mask_set_bit;
>> gc->chip_types->chip.irq_set_type = stm32_irq_set_type;
>> gc->chip_types->chip.irq_set_wake = stm32_irq_set_wake;
>> - gc->chip_types->regs.ack = stm32_bank->pr_ofst;
>> - gc->chip_types->regs.mask = stm32_bank->imr_ofst;
>> gc->private = (void *)stm32_bank;
>> /* Determine number of irqs supported */
>>
>
> BR
> Ludo
Radoslaw Pietrzyk (2):
irqchip: stm32: Optimizes and cleans up stm32-exti irq_domain
pinctrl: stm32: Optimizes and enhances stm32gpio irqchip
drivers/irqchip/irq-stm32-exti.c | 13 -------------
drivers/pinctrl/stm32/pinctrl-stm32.c | 3 ++-
2 files changed, 2 insertions(+), 14 deletions(-)
--
1.9.1
- removes unneeded irq_chip.irq_eoi callback
- adds irq_chip.irq_set_wake callback for possible
in the future GPIO wakeup
- adds irq_chip.irq_ack callback
Signed-off-by: Radoslaw Pietrzyk <[email protected]>
---
drivers/pinctrl/stm32/pinctrl-stm32.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 617df16..6cbcff4 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -268,10 +268,11 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
static struct irq_chip stm32_gpio_irq_chip = {
.name = "stm32gpio",
- .irq_eoi = irq_chip_eoi_parent,
+ .irq_ack = irq_chip_ack_parent,
.irq_mask = irq_chip_mask_parent,
.irq_unmask = irq_chip_unmask_parent,
.irq_set_type = irq_chip_set_type_parent,
+ .irq_set_wake = irq_chip_set_wake_parent,
.irq_request_resources = stm32_gpio_irq_request_resources,
.irq_release_resources = stm32_gpio_irq_release_resources,
};
--
1.9.1
- discards setting handle_simple_irq handler for hierarchy interrupts
- removes acking in chained irq handler as this is done by
irq_chip itself inside handle_edge_irq
- removes unneeded irq_domain_ops.xlate callback
Signed-off-by: Radoslaw Pietrzyk <[email protected]>
---
drivers/irqchip/irq-stm32-exti.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/drivers/irqchip/irq-stm32-exti.c b/drivers/irqchip/irq-stm32-exti.c
index 36f0fbe..8013a87 100644
--- a/drivers/irqchip/irq-stm32-exti.c
+++ b/drivers/irqchip/irq-stm32-exti.c
@@ -79,13 +79,6 @@ static unsigned long stm32_exti_pending(struct irq_chip_generic *gc)
return irq_reg_readl(gc, stm32_bank->pr_ofst);
}
-static void stm32_exti_irq_ack(struct irq_chip_generic *gc, u32 mask)
-{
- const struct stm32_exti_bank *stm32_bank = gc->private;
-
- irq_reg_writel(gc, mask, stm32_bank->pr_ofst);
-}
-
static void stm32_irq_handler(struct irq_desc *desc)
{
struct irq_domain *domain = irq_desc_get_handler_data(desc);
@@ -106,7 +99,6 @@ static void stm32_irq_handler(struct irq_desc *desc)
for_each_set_bit(n, &pending, IRQS_PER_BANK) {
virq = irq_find_mapping(domain, irq_base + n);
generic_handle_irq(virq);
- stm32_exti_irq_ack(gc, BIT(n));
}
}
}
@@ -176,16 +168,12 @@ static int stm32_irq_set_wake(struct irq_data *data, unsigned int on)
static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq,
unsigned int nr_irqs, void *data)
{
- struct irq_chip_generic *gc;
struct irq_fwspec *fwspec = data;
irq_hw_number_t hwirq;
hwirq = fwspec->param[0];
- gc = irq_get_domain_generic_chip(d, hwirq);
irq_map_generic_chip(d, virq, hwirq);
- irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc,
- handle_simple_irq, NULL, NULL);
return 0;
}
@@ -200,7 +188,6 @@ static void stm32_exti_free(struct irq_domain *d, unsigned int virq,
struct irq_domain_ops irq_exti_domain_ops = {
.map = irq_map_generic_chip,
- .xlate = irq_domain_xlate_onetwocell,
.alloc = stm32_exti_alloc,
.free = stm32_exti_free,
};
--
1.9.1
Hi Ludovic,
Please check latest v2 patches series and let me know if it works on
stm32h7 as it does on stm32f4
2018-02-22 12:01 GMT+01:00 Radosław Pietrzyk <[email protected]>:
> Hi Ludovic,
> I have tested on stm32f429-disco and it works without a problem.
> handle_edge_irq is not used in current implementation because it is
> substituted by handle_simple_irq in an alloc callback either way which
> causes that irq_ack callback is not invoked as well (acking is done in
> chained handler).
>
> 2018-02-22 11:55 GMT+01:00 Ludovic BARRE <[email protected]>:
>> Hi Radek
>>
>> I've tested your change on stm32h743i-eval.dts board
>> and gpio-keys tests are not functional.
>> No interrupt occurs when we push or release button
>> (cat /proc/interrupt).
>>
>> Board: stm32h743i-eval.dts
>> Test description: gpio-keys (gpioc 13 => exti 13), with gpio or interrupt
>> property.
>>
>> comment below
>>
>>
>> On 02/21/2018 02:50 PM, radek wrote:
>>>
>>> - allocates generic chip with handle_simple_irq
>>> which simplifies irq_domain_ops.alloc callback
>>> - removes ack register from generic chip which is not used for
>>> handle_simple_irq as acking is done in a chained handler
>>> - removes unneeded irq_domain_ops.xlate callback
>>>
>>> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
>>> ---
>>> drivers/irqchip/irq-stm32-exti.c | 10 +---------
>>> 1 file changed, 1 insertion(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-stm32-exti.c
>>> b/drivers/irqchip/irq-stm32-exti.c
>>> index 36f0fbe..42e74e3 100644
>>> --- a/drivers/irqchip/irq-stm32-exti.c
>>> +++ b/drivers/irqchip/irq-stm32-exti.c
>>> @@ -176,16 +176,12 @@ static int stm32_irq_set_wake(struct irq_data *data,
>>> unsigned int on)
>>> static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq,
>>> unsigned int nr_irqs, void *data)
>>> {
>>> - struct irq_chip_generic *gc;
>>> struct irq_fwspec *fwspec = data;
>>> irq_hw_number_t hwirq;
>>> hwirq = fwspec->param[0];
>>> - gc = irq_get_domain_generic_chip(d, hwirq);
>>> irq_map_generic_chip(d, virq, hwirq);
>>> - irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc,
>>> - handle_simple_irq, NULL, NULL);
>>> return 0;
>>> }
>>> @@ -200,7 +196,6 @@ static void stm32_exti_free(struct irq_domain *d,
>>> unsigned int virq,
>>> struct irq_domain_ops irq_exti_domain_ops = {
>>> .map = irq_map_generic_chip,
>>> - .xlate = irq_domain_xlate_onetwocell,
>>> .alloc = stm32_exti_alloc,
>>> .free = stm32_exti_free,
>>> };
>>> @@ -231,7 +226,7 @@ __init stm32_exti_init(const struct stm32_exti_bank
>>> **stm32_exti_banks,
>>> }
>>> ret = irq_alloc_domain_generic_chips(domain, IRQS_PER_BANK, 1,
>>> "exti",
>>> - handle_edge_irq, clr, 0, 0);
>>> + handle_simple_irq, clr, 0,
>>> 0);
>>
>>
>> EXTI hardware block is trigged on pulse event rising or failing edge.
>> Why do you change to handle_simple_irq ?
>>
>>> if (ret) {
>>> pr_err("%pOF: Could not allocate generic interrupt
>>> chip.\n",
>>> node);
>>> @@ -246,13 +241,10 @@ __init stm32_exti_init(const struct stm32_exti_bank
>>> **stm32_exti_banks,
>>> gc->reg_base = base;
>>> gc->chip_types->type = IRQ_TYPE_EDGE_BOTH;
>>> - gc->chip_types->chip.irq_ack = irq_gc_ack_set_bit;
>>> gc->chip_types->chip.irq_mask = irq_gc_mask_clr_bit;
>>> gc->chip_types->chip.irq_unmask = irq_gc_mask_set_bit;
>>> gc->chip_types->chip.irq_set_type = stm32_irq_set_type;
>>> gc->chip_types->chip.irq_set_wake = stm32_irq_set_wake;
>>> - gc->chip_types->regs.ack = stm32_bank->pr_ofst;
>>> - gc->chip_types->regs.mask = stm32_bank->imr_ofst;
>>> gc->private = (void *)stm32_bank;
>>> /* Determine number of irqs supported */
>>>
>>
>> BR
>> Ludo
Radoslaw,
On Fri, 23 Feb 2018, Radoslaw Pietrzyk wrote:
> - discards setting handle_simple_irq handler for hierarchy interrupts
> - removes acking in chained irq handler as this is done by
> irq_chip itself inside handle_edge_irq
> - removes unneeded irq_domain_ops.xlate callback
if that's all functionally correct, then this is a nice cleanup. Though
from the above changelog its hard to tell because it merily tells WHAT the
patch does, but not WHY. The WHY is the important information for a
reviewer who is not familiar with the particular piece of code/hardware.
Can you please amend the changelog with proper explanations why a
particular piece of code is not needed or has to be changed to something
else?
Thanks,
tglx
I think the HW is fairly simple and straightforward comparing to other
irq chips so it's rather a logic that concerned me the most i.e. why
gpio irqs were handled in a "simple" manner whereas the rest of
interrupts in "edge" manner. According to specification all interrupts
are edge driven and that's how are they treated in set_type callback.
First I thought that all can be handled as simple but then I realized
it makes more sense in handling all of them as edge as they should
according to specification. I will prepare a v3 series with more
detailed explanation in it.
2018-02-23 9:42 GMT+01:00 Thomas Gleixner <[email protected]>:
> Radoslaw,
>
> On Fri, 23 Feb 2018, Radoslaw Pietrzyk wrote:
>
>> - discards setting handle_simple_irq handler for hierarchy interrupts
>> - removes acking in chained irq handler as this is done by
>> irq_chip itself inside handle_edge_irq
>> - removes unneeded irq_domain_ops.xlate callback
>
> if that's all functionally correct, then this is a nice cleanup. Though
> from the above changelog its hard to tell because it merily tells WHAT the
> patch does, but not WHY. The WHY is the important information for a
> reviewer who is not familiar with the particular piece of code/hardware.
>
> Can you please amend the changelog with proper explanations why a
> particular piece of code is not needed or has to be changed to something
> else?
>
> Thanks,
>
> tglx
hi Radoslaw
The gpio-keys tests it's now functional on H7, great.
However the gpio-key test only the bank1 (like stm32f429).
Like the H7 introduce the multi-bank management,
we must perform complementary test.
comment below about ack in handler
On 02/23/2018 09:31 AM, Radoslaw Pietrzyk wrote:
> - discards setting handle_simple_irq handler for hierarchy interrupts
> - removes acking in chained irq handler as this is done by
> irq_chip itself inside handle_edge_irq
> - removes unneeded irq_domain_ops.xlate callback
>
> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
> ---
> drivers/irqchip/irq-stm32-exti.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
> diff --git a/drivers/irqchip/irq-stm32-exti.c b/drivers/irqchip/irq-stm32-exti.c
> index 36f0fbe..8013a87 100644
> --- a/drivers/irqchip/irq-stm32-exti.c
> +++ b/drivers/irqchip/irq-stm32-exti.c
> @@ -79,13 +79,6 @@ static unsigned long stm32_exti_pending(struct irq_chip_generic *gc)
> return irq_reg_readl(gc, stm32_bank->pr_ofst);
> }
>
> -static void stm32_exti_irq_ack(struct irq_chip_generic *gc, u32 mask)
> -{
> - const struct stm32_exti_bank *stm32_bank = gc->private;
> -
> - irq_reg_writel(gc, mask, stm32_bank->pr_ofst);
> -}
> -
> static void stm32_irq_handler(struct irq_desc *desc)
> {
> struct irq_domain *domain = irq_desc_get_handler_data(desc);
> @@ -106,7 +99,6 @@ static void stm32_irq_handler(struct irq_desc *desc)
> for_each_set_bit(n, &pending, IRQS_PER_BANK) {
> virq = irq_find_mapping(domain, irq_base + n);
> generic_handle_irq(virq);
> - stm32_exti_irq_ack(gc, BIT(n));
the while loop " while ((pending = " has been introduce while original
upstream of this driver in V2 to V3
https://patchwork.ozlabs.org/patch/604190/
https://patchwork.ozlabs.org/patch/665242/
the "ack" is needed to acknowledge the irq which are handled and which
is not the original irq for which we enter.
> }
> }
> }
> @@ -176,16 +168,12 @@ static int stm32_irq_set_wake(struct irq_data *data, unsigned int on)
> static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq,
> unsigned int nr_irqs, void *data)
> {
> - struct irq_chip_generic *gc;
> struct irq_fwspec *fwspec = data;
> irq_hw_number_t hwirq;
>
> hwirq = fwspec->param[0];
> - gc = irq_get_domain_generic_chip(d, hwirq);
>
> irq_map_generic_chip(d, virq, hwirq);
> - irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc,
> - handle_simple_irq, NULL, NULL);
I see if it is not disturb the multi-bank.
>
> return 0;
> }
> @@ -200,7 +188,6 @@ static void stm32_exti_free(struct irq_domain *d, unsigned int virq,
>
> struct irq_domain_ops irq_exti_domain_ops = {
> .map = irq_map_generic_chip,
> - .xlate = irq_domain_xlate_onetwocell,
> .alloc = stm32_exti_alloc,
> .free = stm32_exti_free,
> };
>
BR
Ludo
On 02/23/2018 04:46 PM, Ludovic BARRE wrote:
> hi Radoslaw
>
> The gpio-keys tests it's now functional on H7, great.
> However the gpio-key test only the bank1 (like stm32f429).
> Like the H7 introduce the multi-bank management,
> we must perform complementary test.
>
> comment below about ack in handler
>
> On 02/23/2018 09:31 AM, Radoslaw Pietrzyk wrote:
>> - discards setting handle_simple_irq handler for hierarchy interrupts
>> - removes acking in chained irq handler as this is done by
>> irq_chip itself inside handle_edge_irq
>> - removes unneeded irq_domain_ops.xlate callback
>>
>> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
>> ---
>> drivers/irqchip/irq-stm32-exti.c | 13 -------------
>> 1 file changed, 13 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-stm32-exti.c
>> b/drivers/irqchip/irq-stm32-exti.c
>> index 36f0fbe..8013a87 100644
>> --- a/drivers/irqchip/irq-stm32-exti.c
>> +++ b/drivers/irqchip/irq-stm32-exti.c
>> @@ -79,13 +79,6 @@ static unsigned long stm32_exti_pending(struct
>> irq_chip_generic *gc)
>> return irq_reg_readl(gc, stm32_bank->pr_ofst);
>> }
>> -static void stm32_exti_irq_ack(struct irq_chip_generic *gc, u32 mask)
>> -{
>> - const struct stm32_exti_bank *stm32_bank = gc->private;
>> -
>> - irq_reg_writel(gc, mask, stm32_bank->pr_ofst);
>> -}
>> -
>> static void stm32_irq_handler(struct irq_desc *desc)
>> {
>> struct irq_domain *domain = irq_desc_get_handler_data(desc);
>> @@ -106,7 +99,6 @@ static void stm32_irq_handler(struct irq_desc *desc)
>> for_each_set_bit(n, &pending, IRQS_PER_BANK) {
>> virq = irq_find_mapping(domain, irq_base + n);
>> generic_handle_irq(virq);
>> - stm32_exti_irq_ack(gc, BIT(n));
>
> the while loop " while ((pending = " has been introduce while original
> upstream of this driver in V2 to V3
> https://patchwork.ozlabs.org/patch/604190/
> https://patchwork.ozlabs.org/patch/665242/
>
> the "ack" is needed to acknowledge the irq which are handled and which
> is not the original irq for which we enter.
>
>> }
>> }
>> }
>> @@ -176,16 +168,12 @@ static int stm32_irq_set_wake(struct irq_data
>> *data, unsigned int on)
>> static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq,
>> unsigned int nr_irqs, void *data)
>> {
>> - struct irq_chip_generic *gc;
>> struct irq_fwspec *fwspec = data;
>> irq_hw_number_t hwirq;
>> hwirq = fwspec->param[0];
>> - gc = irq_get_domain_generic_chip(d, hwirq);
>> irq_map_generic_chip(d, virq, hwirq);
>> - irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc,
>> - handle_simple_irq, NULL, NULL);
>
> I see if it is not disturb the multi-bank.
you are right, normally irq_domain_set_info is already set in
irq_map_generic_chip.
>
>> return 0;
>> }
>> @@ -200,7 +188,6 @@ static void stm32_exti_free(struct irq_domain *d,
>> unsigned int virq,
>> struct irq_domain_ops irq_exti_domain_ops = {
>> .map = irq_map_generic_chip,
>> - .xlate = irq_domain_xlate_onetwocell,
>> .alloc = stm32_exti_alloc,
>> .free = stm32_exti_free,
>> };
>>
>
> BR
> Ludo
I don't fully get it to be honest. If interrupt is pending it must
have been enabled (unmasked) and requires to be handled acked. It will
be acked by irq_chip.irq_ack handler within the edge handler.
Therefore this additional acking is meaningless.
2018-02-23 16:46 GMT+01:00 Ludovic BARRE <[email protected]>:
> hi Radoslaw
>
> The gpio-keys tests it's now functional on H7, great.
> However the gpio-key test only the bank1 (like stm32f429).
> Like the H7 introduce the multi-bank management,
> we must perform complementary test.
>
> comment below about ack in handler
>
>
> On 02/23/2018 09:31 AM, Radoslaw Pietrzyk wrote:
>>
>> - discards setting handle_simple_irq handler for hierarchy interrupts
>> - removes acking in chained irq handler as this is done by
>> irq_chip itself inside handle_edge_irq
>> - removes unneeded irq_domain_ops.xlate callback
>>
>> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
>> ---
>> drivers/irqchip/irq-stm32-exti.c | 13 -------------
>> 1 file changed, 13 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-stm32-exti.c
>> b/drivers/irqchip/irq-stm32-exti.c
>> index 36f0fbe..8013a87 100644
>> --- a/drivers/irqchip/irq-stm32-exti.c
>> +++ b/drivers/irqchip/irq-stm32-exti.c
>> @@ -79,13 +79,6 @@ static unsigned long stm32_exti_pending(struct
>> irq_chip_generic *gc)
>> return irq_reg_readl(gc, stm32_bank->pr_ofst);
>> }
>> -static void stm32_exti_irq_ack(struct irq_chip_generic *gc, u32 mask)
>> -{
>> - const struct stm32_exti_bank *stm32_bank = gc->private;
>> -
>> - irq_reg_writel(gc, mask, stm32_bank->pr_ofst);
>> -}
>> -
>> static void stm32_irq_handler(struct irq_desc *desc)
>> {
>> struct irq_domain *domain = irq_desc_get_handler_data(desc);
>> @@ -106,7 +99,6 @@ static void stm32_irq_handler(struct irq_desc *desc)
>> for_each_set_bit(n, &pending, IRQS_PER_BANK) {
>> virq = irq_find_mapping(domain, irq_base +
>> n);
>> generic_handle_irq(virq);
>> - stm32_exti_irq_ack(gc, BIT(n));
>
>
> the while loop " while ((pending = " has been introduce while original
> upstream of this driver in V2 to V3
> https://patchwork.ozlabs.org/patch/604190/
> https://patchwork.ozlabs.org/patch/665242/
>
> the "ack" is needed to acknowledge the irq which are handled and which is
> not the original irq for which we enter.
>
>> }
>> }
>> }
>> @@ -176,16 +168,12 @@ static int stm32_irq_set_wake(struct irq_data *data,
>> unsigned int on)
>> static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq,
>> unsigned int nr_irqs, void *data)
>> {
>> - struct irq_chip_generic *gc;
>> struct irq_fwspec *fwspec = data;
>> irq_hw_number_t hwirq;
>> hwirq = fwspec->param[0];
>> - gc = irq_get_domain_generic_chip(d, hwirq);
>> irq_map_generic_chip(d, virq, hwirq);
>> - irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc,
>> - handle_simple_irq, NULL, NULL);
>
>
> I see if it is not disturb the multi-bank.
>
>> return 0;
>> }
>> @@ -200,7 +188,6 @@ static void stm32_exti_free(struct irq_domain *d,
>> unsigned int virq,
>> struct irq_domain_ops irq_exti_domain_ops = {
>> .map = irq_map_generic_chip,
>> - .xlate = irq_domain_xlate_onetwocell,
>> .alloc = stm32_exti_alloc,
>> .free = stm32_exti_free,
>> };
>>
>
> BR
> Ludo
hi Radoslaw
Reviewed-by: Ludovic Barre <[email protected]>
Tested-by: Ludovic Barre <[email protected]>
BR
Ludo
On 02/23/2018 09:31 AM, Radoslaw Pietrzyk wrote:
> - removes unneeded irq_chip.irq_eoi callback
> - adds irq_chip.irq_set_wake callback for possible
> in the future GPIO wakeup
> - adds irq_chip.irq_ack callback
>
> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
> ---
> drivers/pinctrl/stm32/pinctrl-stm32.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 617df16..6cbcff4 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -268,10 +268,11 @@ static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
>
> static struct irq_chip stm32_gpio_irq_chip = {
> .name = "stm32gpio",
> - .irq_eoi = irq_chip_eoi_parent,
> + .irq_ack = irq_chip_ack_parent,
> .irq_mask = irq_chip_mask_parent,
> .irq_unmask = irq_chip_unmask_parent,
> .irq_set_type = irq_chip_set_type_parent,
> + .irq_set_wake = irq_chip_set_wake_parent,
> .irq_request_resources = stm32_gpio_irq_request_resources,
> .irq_release_resources = stm32_gpio_irq_release_resources,
> };
>
hi Radosław
On 02/23/2018 06:37 PM, Radosław Pietrzyk wrote:
> I don't fully get it to be honest. If interrupt is pending it must
> have been enabled (unmasked) and requires to be handled acked. It will
> be acked by irq_chip.irq_ack handler within the edge handler.
> Therefore this additional acking is meaningless.
>
Sorry, I did not see modification in pinctrl-stm32.c.
So it's ok for me
Acked-by: Ludovic Barre <[email protected]>
Tested-by: Ludovic Barre <[email protected]>
BR
Ludo
> 2018-02-23 16:46 GMT+01:00 Ludovic BARRE <[email protected]>:
>> hi Radoslaw
>>
>> The gpio-keys tests it's now functional on H7, great.
>> However the gpio-key test only the bank1 (like stm32f429).
>> Like the H7 introduce the multi-bank management,
>> we must perform complementary test.
>>
>> comment below about ack in handler
>>
>>
>> On 02/23/2018 09:31 AM, Radoslaw Pietrzyk wrote:
>>>
>>> - discards setting handle_simple_irq handler for hierarchy interrupts
>>> - removes acking in chained irq handler as this is done by
>>> irq_chip itself inside handle_edge_irq
>>> - removes unneeded irq_domain_ops.xlate callback
>>>
>>> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
>>> ---
>>> drivers/irqchip/irq-stm32-exti.c | 13 -------------
>>> 1 file changed, 13 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-stm32-exti.c
>>> b/drivers/irqchip/irq-stm32-exti.c
>>> index 36f0fbe..8013a87 100644
>>> --- a/drivers/irqchip/irq-stm32-exti.c
>>> +++ b/drivers/irqchip/irq-stm32-exti.c
>>> @@ -79,13 +79,6 @@ static unsigned long stm32_exti_pending(struct
>>> irq_chip_generic *gc)
>>> return irq_reg_readl(gc, stm32_bank->pr_ofst);
>>> }
>>> -static void stm32_exti_irq_ack(struct irq_chip_generic *gc, u32 mask)
>>> -{
>>> - const struct stm32_exti_bank *stm32_bank = gc->private;
>>> -
>>> - irq_reg_writel(gc, mask, stm32_bank->pr_ofst);
>>> -}
>>> -
>>> static void stm32_irq_handler(struct irq_desc *desc)
>>> {
>>> struct irq_domain *domain = irq_desc_get_handler_data(desc);
>>> @@ -106,7 +99,6 @@ static void stm32_irq_handler(struct irq_desc *desc)
>>> for_each_set_bit(n, &pending, IRQS_PER_BANK) {
>>> virq = irq_find_mapping(domain, irq_base +
>>> n);
>>> generic_handle_irq(virq);
>>> - stm32_exti_irq_ack(gc, BIT(n));
>>
>>
>> the while loop " while ((pending = " has been introduce while original
>> upstream of this driver in V2 to V3
>> https://patchwork.ozlabs.org/patch/604190/
>> https://patchwork.ozlabs.org/patch/665242/
>>
>> the "ack" is needed to acknowledge the irq which are handled and which is
>> not the original irq for which we enter.
>>
>>> }
>>> }
>>> }
>>> @@ -176,16 +168,12 @@ static int stm32_irq_set_wake(struct irq_data *data,
>>> unsigned int on)
>>> static int stm32_exti_alloc(struct irq_domain *d, unsigned int virq,
>>> unsigned int nr_irqs, void *data)
>>> {
>>> - struct irq_chip_generic *gc;
>>> struct irq_fwspec *fwspec = data;
>>> irq_hw_number_t hwirq;
>>> hwirq = fwspec->param[0];
>>> - gc = irq_get_domain_generic_chip(d, hwirq);
>>> irq_map_generic_chip(d, virq, hwirq);
>>> - irq_domain_set_info(d, virq, hwirq, &gc->chip_types->chip, gc,
>>> - handle_simple_irq, NULL, NULL);
>>
>>
>> I see if it is not disturb the multi-bank.
>>
>>> return 0;
>>> }
>>> @@ -200,7 +188,6 @@ static void stm32_exti_free(struct irq_domain *d,
>>> unsigned int virq,
>>> struct irq_domain_ops irq_exti_domain_ops = {
>>> .map = irq_map_generic_chip,
>>> - .xlate = irq_domain_xlate_onetwocell,
>>> .alloc = stm32_exti_alloc,
>>> .free = stm32_exti_free,
>>> };
>>>
>>
>> BR
>> Ludo
On Wed, Feb 21, 2018 at 2:50 PM, Radoslaw Pietrzyk
<[email protected]> wrote:
> - removes unneeded irq_chip.irq_eoi callback
> - adds irq_chip.irq_set_wake callback for possible
> in the future GPIO wakeup
>
> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
Patch applied with Alexandre's ACK, stripping ARM from the
commit message, thanks!
Yours,
Linus Walleij
I don't know if Alexandre agrees but It might be better to take v2
where irq_ack is added.
2018-03-01 22:04 GMT+01:00 Linus Walleij <[email protected]>:
> On Wed, Feb 21, 2018 at 2:50 PM, Radoslaw Pietrzyk
> <[email protected]> wrote:
>
>> - removes unneeded irq_chip.irq_eoi callback
>> - adds irq_chip.irq_set_wake callback for possible
>> in the future GPIO wakeup
>>
>> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
>
> Patch applied with Alexandre's ACK, stripping ARM from the
> commit message, thanks!
>
> Yours,
> Linus Walleij
On Fri, Feb 23, 2018 at 9:31 AM, Radoslaw Pietrzyk
<[email protected]> wrote:
> - removes unneeded irq_chip.irq_eoi callback
> - adds irq_chip.irq_set_wake callback for possible
> in the future GPIO wakeup
> - adds irq_chip.irq_ack callback
>
> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
This v2 patch applied with Ludovic's tags.
Yours,
Linus Walleij
Linus responded in v2 mail thread that he's gonna take it instead of v1.
2018-03-02 9:14 GMT+01:00 Alexandre Torgue <[email protected]>:
> Hi Radoslaw and Linus,
>
>
> On 03/01/2018 10:24 PM, Radosław Pietrzyk wrote:
>>
>> I don't know if Alexandre agrees but It might be better to take v2
>> where irq_ack is added.
>
>
>
> Sorry Linus, I agree with Rodoslaw, you need to take V2 as stm32-exti patch
> is linked to pinctrl v2 patch. I gonna add my acked-by.
>
> Regards
> Alex
>
>
>>
>> 2018-03-01 22:04 GMT+01:00 Linus Walleij <[email protected]>:
>>>
>>> On Wed, Feb 21, 2018 at 2:50 PM, Radoslaw Pietrzyk
>>> <[email protected]> wrote:
>>>
>>>> - removes unneeded irq_chip.irq_eoi callback
>>>> - adds irq_chip.irq_set_wake callback for possible
>>>> in the future GPIO wakeup
>>>>
>>>> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
>>>
>>>
>>> Patch applied with Alexandre's ACK, stripping ARM from the
>>> commit message, thanks!
>>>
>>> Yours,
>>> Linus Walleij
On 03/02/2018 09:37 AM, Radosław Pietrzyk wrote:
> Linus responded in v2 mail thread that he's gonna take it instead of v1.
I saw that too late :).
Regards
Alex
>
> 2018-03-02 9:14 GMT+01:00 Alexandre Torgue <[email protected]>:
>> Hi Radoslaw and Linus,
>>
>>
>> On 03/01/2018 10:24 PM, Radosław Pietrzyk wrote:
>>>
>>> I don't know if Alexandre agrees but It might be better to take v2
>>> where irq_ack is added.
>>
>>
>>
>> Sorry Linus, I agree with Rodoslaw, you need to take V2 as stm32-exti patch
>> is linked to pinctrl v2 patch. I gonna add my acked-by.
>>
>> Regards
>> Alex
>>
>>
>>>
>>> 2018-03-01 22:04 GMT+01:00 Linus Walleij <[email protected]>:
>>>>
>>>> On Wed, Feb 21, 2018 at 2:50 PM, Radoslaw Pietrzyk
>>>> <[email protected]> wrote:
>>>>
>>>>> - removes unneeded irq_chip.irq_eoi callback
>>>>> - adds irq_chip.irq_set_wake callback for possible
>>>>> in the future GPIO wakeup
>>>>>
>>>>> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
>>>>
>>>>
>>>> Patch applied with Alexandre's ACK, stripping ARM from the
>>>> commit message, thanks!
>>>>
>>>> Yours,
>>>> Linus Walleij
Hi Radoslaw and Linus,
On 03/01/2018 10:24 PM, Radosław Pietrzyk wrote:
> I don't know if Alexandre agrees but It might be better to take v2
> where irq_ack is added.
Sorry Linus, I agree with Rodoslaw, you need to take V2 as stm32-exti
patch is linked to pinctrl v2 patch. I gonna add my acked-by.
Regards
Alex
>
> 2018-03-01 22:04 GMT+01:00 Linus Walleij <[email protected]>:
>> On Wed, Feb 21, 2018 at 2:50 PM, Radoslaw Pietrzyk
>> <[email protected]> wrote:
>>
>>> - removes unneeded irq_chip.irq_eoi callback
>>> - adds irq_chip.irq_set_wake callback for possible
>>> in the future GPIO wakeup
>>>
>>> Signed-off-by: Radoslaw Pietrzyk <[email protected]>
>>
>> Patch applied with Alexandre's ACK, stripping ARM from the
>> commit message, thanks!
>>
>> Yours,
>> Linus Walleij
Radoslaw,
On 23/02/18 08:42, Thomas Gleixner wrote:
> Radoslaw,
>
> On Fri, 23 Feb 2018, Radoslaw Pietrzyk wrote:
>
>> - discards setting handle_simple_irq handler for hierarchy interrupts
>> - removes acking in chained irq handler as this is done by
>> irq_chip itself inside handle_edge_irq
>> - removes unneeded irq_domain_ops.xlate callback
>
> if that's all functionally correct, then this is a nice cleanup. Though
> from the above changelog its hard to tell because it merily tells WHAT the
> patch does, but not WHY. The WHY is the important information for a
> reviewer who is not familiar with the particular piece of code/hardware.
>
> Can you please amend the changelog with proper explanations why a
> particular piece of code is not needed or has to be changed to something
> else?
Any update on this? I'd like to queue this for 4.17, but Thomas'
comments should be addressed before that happens. Ca you please respin a
version with a better change log and the various review tags?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 14/03/18 11:46, Radosław Pietrzyk wrote:
> Hi Marc,
> We had a quite fruitful discussion in this mail thread regarding this
> topic and Ludovic acked it so recently I have asked Thomas if he still
> needs this v3 patch with detailed explanation especially as v2 version
> of stm32-gpio patch has been already taken by Linus. However if you
> require I can resend v3 of this patch only with this detailed explanation.
That'd be useful. The changelog is the only thing that will be left from
this discussion, so it'd better be complete and accurate. If you quickly
send a v3 for this single patch, I'll queue it right away.
Thanks,
M.
>
> 2018-03-14 12:09 GMT+01:00 Marc Zyngier <[email protected]
> <mailto:[email protected]>>:
>
> Radoslaw,
>
> On 23/02/18 08:42, Thomas Gleixner wrote:
> > Radoslaw,
> >
> > On Fri, 23 Feb 2018, Radoslaw Pietrzyk wrote:
> >
> >> - discards setting handle_simple_irq handler for hierarchy interrupts
> >> - removes acking in chained irq handler as this is done by
> >> irq_chip itself inside handle_edge_irq
> >> - removes unneeded irq_domain_ops.xlate callback
> >
> > if that's all functionally correct, then this is a nice cleanup. Though
> > from the above changelog its hard to tell because it merily tells WHAT the
> > patch does, but not WHY. The WHY is the important information for a
> > reviewer who is not familiar with the particular piece of code/hardware.
> >
> > Can you please amend the changelog with proper explanations why a
> > particular piece of code is not needed or has to be changed to something
> > else?
>
> Any update on this? I'd like to queue this for 4.17, but Thomas'
> comments should be addressed before that happens. Ca you please respin a
> version with a better change log and the various review tags?
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
>
>
--
Jazz is not dead. It just smells funny...
Hi Radoslaw
I preparing a patch serie which add support of stm32mp1.
Would you like, I add your patch (with commit message updated)
in my serie?
patch:
-irqchip: stm32: Optimizes and cleans up stm32-exti irq_domain
BR
Ludo
On 03/14/2018 01:04 PM, Marc Zyngier wrote:
> On 14/03/18 11:46, Radosław Pietrzyk wrote:
>> Hi Marc,
>> We had a quite fruitful discussion in this mail thread regarding this
>> topic and Ludovic acked it so recently I have asked Thomas if he still
>> needs this v3 patch with detailed explanation especially as v2 version
>> of stm32-gpio patch has been already taken by Linus. However if you
>> require I can resend v3 of this patch only with this detailed explanation.
>
> That'd be useful. The changelog is the only thing that will be left from
> this discussion, so it'd better be complete and accurate. If you quickly
> send a v3 for this single patch, I'll queue it right away.
>
> Thanks,
>
> M.
>
>>
>> 2018-03-14 12:09 GMT+01:00 Marc Zyngier <[email protected]
>> <mailto:[email protected]>>:
>>
>> Radoslaw,
>>
>> On 23/02/18 08:42, Thomas Gleixner wrote:
>> > Radoslaw,
>> >
>> > On Fri, 23 Feb 2018, Radoslaw Pietrzyk wrote:
>> >
>> >> - discards setting handle_simple_irq handler for hierarchy interrupts
>> >> - removes acking in chained irq handler as this is done by
>> >> irq_chip itself inside handle_edge_irq
>> >> - removes unneeded irq_domain_ops.xlate callback
>> >
>> > if that's all functionally correct, then this is a nice cleanup. Though
>> > from the above changelog its hard to tell because it merily tells WHAT the
>> > patch does, but not WHY. The WHY is the important information for a
>> > reviewer who is not familiar with the particular piece of code/hardware.
>> >
>> > Can you please amend the changelog with proper explanations why a
>> > particular piece of code is not needed or has to be changed to something
>> > else?
>>
>> Any update on this? I'd like to queue this for 4.17, but Thomas'
>> comments should be addressed before that happens. Ca you please respin a
>> version with a better change log and the various review tags?
>>
>> Thanks,
>>
>> M.
>> --
>> Jazz is not dead. It just smells funny...
>>
>>
>
>
ok, So I include your patch in my serie
- irqchip: stm32: Optimizes and cleans up stm32-exti irq_domain
BR
Ludo
On 04/19/2018 10:03 PM, Radosław Pietrzyk wrote:
> Sure, I don't mind. I didn't have time to resend v3 with more verbose
> description.
>
> 2018-04-19 15:24 GMT+02:00 Ludovic BARRE <[email protected]
> <mailto:[email protected]>>:
>
> Hi Radoslaw
>
> I preparing a patch serie which add support of stm32mp1.
> Would you like, I add your patch (with commit message updated)
> in my serie?
> patch:
> -irqchip: stm32: Optimizes and cleans up stm32-exti irq_domain
>
> BR
> Ludo
>
>
> On 03/14/2018 01:04 PM, Marc Zyngier wrote:
>
> On 14/03/18 11:46, Radosław Pietrzyk wrote:
>
> Hi Marc,
> We had a quite fruitful discussion in this mail thread
> regarding this
> topic and Ludovic acked it so recently I have asked Thomas
> if he still
> needs this v3 patch with detailed explanation especially as
> v2 version
> of stm32-gpio patch has been already taken by Linus. However
> if you
> require I can resend v3 of this patch only with this
> detailed explanation.
>
>
> That'd be useful. The changelog is the only thing that will be
> left from
> this discussion, so it'd better be complete and accurate. If you
> quickly
> send a v3 for this single patch, I'll queue it right away.
>
> Thanks,
>
> M.
>
>
> 2018-03-14 12:09 GMT+01:00 Marc Zyngier
> <[email protected] <mailto:[email protected]>
> <mailto:[email protected] <mailto:[email protected]>>>:
>
> Radoslaw,
>
> On 23/02/18 08:42, Thomas Gleixner wrote:
> > Radoslaw,
> >
> > On Fri, 23 Feb 2018, Radoslaw Pietrzyk wrote:
> >
> >> - discards setting handle_simple_irq handler for
> hierarchy interrupts
> >> - removes acking in chained irq handler as this is
> done by
> >> irq_chip itself inside handle_edge_irq
> >> - removes unneeded irq_domain_ops.xlate callback
> >
> > if that's all functionally correct, then this is a
> nice cleanup. Though
> > from the above changelog its hard to tell because it
> merily tells WHAT the
> > patch does, but not WHY. The WHY is the important
> information for a
> > reviewer who is not familiar with the particular
> piece of code/hardware.
> >
> > Can you please amend the changelog with proper
> explanations why a
> > particular piece of code is not needed or has to be
> changed to something
> > else?
>
> Any update on this? I'd like to queue this for 4.17,
> but Thomas'
> comments should be addressed before that happens. Ca
> you please respin a
> version with a better change log and the various
> review tags?
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
>
>
>
>
>