2019-08-29 18:38:54

by Lina Iyer

Subject: [PATCH RFC 07/14] genirq: Introduce irq_chip_get/set_parent_state calls

From: Maulik Shah <[email protected]>

On certain QTI chipsets some GPIOs are direct-connect interrupts
to the GIC.

Even when GPIOs are not used for interrupt generation and interrupt
line is disabled, it does not prevent interrupt to get pending at
GIC_ISPEND. When drivers call enable_irq unwanted interrupt occures.

Introduce irq_chip_get/set_parent_state calls to clear pending irq
which can get called within irq_enable of child irq chip to clear
any pending irq before enabling.

Signed-off-by: Maulik Shah <[email protected]>
---
include/linux/irq.h | 6 ++++++
kernel/irq/chip.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index fb301cf29148..7853eb9301f2 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -610,6 +610,12 @@ extern int irq_chip_pm_put(struct irq_data *data);
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
extern void handle_fasteoi_ack_irq(struct irq_desc *desc);
extern void handle_fasteoi_mask_irq(struct irq_desc *desc);
+extern int irq_chip_set_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool val);
+extern int irq_chip_get_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool *state);
extern void irq_chip_enable_parent(struct irq_data *data);
extern void irq_chip_disable_parent(struct irq_data *data);
extern void irq_chip_ack_parent(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index b76703b2c0af..6bb5b22bb0a7 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1297,6 +1297,50 @@ EXPORT_SYMBOL_GPL(handle_fasteoi_mask_irq);

#endif /* CONFIG_IRQ_FASTEOI_HIERARCHY_HANDLERS */

+/**
+ * irq_chip_set_parent_state - set the state of a parent interrupt.
+ * @data: Pointer to interrupt specific data
+ * @which: State to be restored (one of IRQCHIP_STATE_*)
+ * @val: Value corresponding to @which
+ *
+ */
+int irq_chip_set_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool val)
+{
+ data = data->parent_data;
+ if (!data)
+ return 0;
+
+ if (data->chip->irq_set_irqchip_state)
+ return data->chip->irq_set_irqchip_state(data, which, val);
+
+ return 0;
+}
+EXPORT_SYMBOL(irq_chip_set_parent_state);
+
+/**
+ * irq_chip_get_parent_state - get the state of a parent interrupt.
+ * @data: Pointer to interrupt specific data
+ * @which: one of IRQCHIP_STATE_* the caller wants to know
+ * @state: a pointer to a boolean where the state is to be stored
+ *
+ */
+int irq_chip_get_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool *state)
+{
+ data = data->parent_data;
+ if (!data)
+ return 0;
+
+ if (data->chip->irq_get_irqchip_state)
+ return data->chip->irq_get_irqchip_state(data, which, state);
+
+ return 0;
+}
+EXPORT_SYMBOL(irq_chip_get_parent_state);
+
/**
* irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
* NULL)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

2019-09-06 10:07:06

by Stephen Boyd

Subject: Re: [PATCH RFC 07/14] genirq: Introduce irq_chip_get/set_parent_state calls

Quoting Lina Iyer (2019-08-29 11:11:56)
> From: Maulik Shah <[email protected]>
>
> On certain QTI chipsets some GPIOs are direct-connect interrupts
> to the GIC.
>
> Even when GPIOs are not used for interrupt generation and interrupt
> line is disabled, it does not prevent interrupt to get pending at
> GIC_ISPEND. When drivers call enable_irq unwanted interrupt occures.

Inidicate functions with parenthesis like enable_irq().

>
> Introduce irq_chip_get/set_parent_state calls to clear pending irq
> which can get called within irq_enable of child irq chip to clear
> any pending irq before enabling.

This sentence is hard to read.

>
> index b76703b2c0af..6bb5b22bb0a7 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -1297,6 +1297,50 @@ EXPORT_SYMBOL_GPL(handle_fasteoi_mask_irq);
>
> #endif /* CONFIG_IRQ_FASTEOI_HIERARCHY_HANDLERS */
>
> +/**
> + * irq_chip_set_parent_state - set the state of a parent interrupt.
> + * @data: Pointer to interrupt specific data
> + * @which: State to be restored (one of IRQCHIP_STATE_*)
> + * @val: Value corresponding to @which
> + *
> + */
> +int irq_chip_set_parent_state(struct irq_data *data,
> + enum irqchip_irq_state which,
> + bool val)
> +{
> + data = data->parent_data;
> + if (!data)
> + return 0;
> +
> + if (data->chip->irq_set_irqchip_state)
> + return data->chip->irq_set_irqchip_state(data, which, val);
> +
> + return 0;

How about

if (!data || !data->chip->irq_set_irqchip_state)
return 0;

return data->chip->irq_set_irqchip_state(...)

> +}
> +EXPORT_SYMBOL(irq_chip_set_parent_state);
> +
> +/**
> + * irq_chip_get_parent_state - get the state of a parent interrupt.

Why is this indented so much?

> + * @data: Pointer to interrupt specific data
> + * @which: one of IRQCHIP_STATE_* the caller wants to know
> + * @state: a pointer to a boolean where the state is to be stored
> + *

Document return value?

> + */
> +int irq_chip_get_parent_state(struct irq_data *data,
> + enum irqchip_irq_state which,
> + bool *state)
> +{
> + data = data->parent_data;
> + if (!data)
> + return 0;
> +
> + if (data->chip->irq_get_irqchip_state)
> + return data->chip->irq_get_irqchip_state(data, which, state);
> +

Same comment here about collapsing logic.

> + return 0;
> +}
> +EXPORT_SYMBOL(irq_chip_get_parent_state);

Please make these symbols _GPL.

> +
> /**
> * irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
> * NULL)