Re: [PATCH v4] irqchip/irq-mst: Support polarity configuration

Marc Zyngier
2021-04-07T12:14:54Z

On Mon, 15 Mar 2021 13:18:48 +0000,
Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote:
>
> Support irq polarity configuration and save and restore the config
> when system suspend and resume.
>
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> ---
> drivers/irqchip/irq-mst-intc.c | 94 ++++++++++++++++++++++++++++++++--
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
> index 143657b0cf28..a2ab3f837b96 100644
> --- a/drivers/irqchip/irq-mst-intc.c
> +++ b/drivers/irqchip/irq-mst-intc.c
> @@ -13,15 +13,27 @@
> #include <linux/of_irq.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> +#include <linux/syscore_ops.h>
>
> -#define INTC_MASK 0x0
> -#define INTC_EOI 0x20
> +#define MST_INTC_MAX_IRQS 64
> +
> +#define INTC_MASK 0x0
> +#define INTC_REV_POLARITY 0x10
> +#define INTC_EOI 0x20
> +
> +#ifdef CONFIG_PM_SLEEP
> +static LIST_HEAD(mst_intc_list);
> +#endif
>
> struct mst_intc_chip_data {
> raw_spinlock_t lock;
> unsigned int irq_start, nr_irqs;
> void __iomem *base;
> bool no_eoi;
> +#ifdef CONFIG_PM_SLEEP
> + struct list_head entry;
> + u16 saved_polarity_conf[DIV_ROUND_UP(MST_INTC_MAX_IRQS, 16)];
> +#endif
> };
>
> static void mst_set_irq(struct irq_data *d, u32 offset)
> @@ -78,6 +90,20 @@ static void mst_intc_eoi_irq(struct irq_data *d)
> irq_chip_eoi_parent(d);
> }
>
> +static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type)
> +{
> + if (type != IRQ_TYPE_EDGE_RISING && type != IRQ_TYPE_EDGE_FALLING &&
> + type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_LEVEL_LOW)
> + return -EINVAL;

All of this amounts to checking for IRQ_TYPE_{NONE,EDGE_BOTH}. Maybe
that's a better option.

> +
> + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING)
> + mst_set_irq(data, INTC_REV_POLARITY);

What happens if the interrupt was set to LEVEL_LOW, then switched to
LEVEL_HIGH? You end up with a stuck reversed polarity.

> +
> + type = IRQ_TYPE_LEVEL_HIGH;
> +
> + return irq_chip_set_type_parent(data, type);

I'm going to fix this as below. Shout if you disagree.

Thanks,

M.

diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
index a2ab3f837b96..f6133ae28155 100644
--- a/drivers/irqchip/irq-mst-intc.c
+++ b/drivers/irqchip/irq-mst-intc.c
@@ -92,16 +92,20 @@ static void mst_intc_eoi_irq(struct irq_data *d)

static int mst_irq_chip_set_type(struct irq_data *data, unsigned int type)
{
- if (type != IRQ_TYPE_EDGE_RISING && type != IRQ_TYPE_EDGE_FALLING &&
- type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_LEVEL_LOW)
- return -EINVAL;
-
- if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING)
+ switch (type) {
+ case IRQ_TYPE_LEVEL_LOW:
+ case IRQ_TYPE_EDGE_FALLING:
mst_set_irq(data, INTC_REV_POLARITY);
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ case IRQ_TYPE_EDGE_RISING:
+ mst_clear_irq(data, INTC_REV_POLARITY);
+ break;
+ default:
+ return -EINVAL;
+ }

- type = IRQ_TYPE_LEVEL_HIGH;
-
- return irq_chip_set_type_parent(data, type);
+ return irq_chip_set_type_parent(data, IRQ_TYPE_LEVEL_HIGH);
}

static struct irq_chip mst_intc_chip = {

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