Re: [PATCH v2 2/2] irqchip: add MSM8x60 MPM wakeup interrupt controller driver

[Thomas Gleixner]

On Sun, May 31 2026 at 06:09, Herman van Hazendonk wrote:
> + *
> + *   1. Hierarchical irqdomain: for MPM pins that map to GIC SPIs (USB,
> + *      HDMI, ...). Consumers wire their interrupts through this
> + *      controller via interrupts-extended and the kernel manages
> + *      enable / mask / set_type / set_wake via the IRQ subsystem.
> + *
> + *   2. Raw-pin API: for MPM pins that do NOT correspond to a GIC IRQ
> + *      (SDC3_DAT1=21, SDC3_DAT3=22, SDC4_DAT1=23, SDC4_DAT3=24).
> + *      These are physical wake-signal lines monitored by MPM
> + *      directly. Consumers (mmci for SDC4 wake) call
> + *      msm8660_mpm_set_pin_wake() etc. The consumer API establishes
> + *      a device_link from consumer to producer so the MPM device
> + *      cannot disappear while a consumer holds a handle.

Why can't this be described in the device tree?

> +
> +struct msm8660_mpm {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct irq_domain *domain;
> +	struct msm8660_mpm_pin *pin_map;
> +	unsigned int pin_map_count;
> +	int parent_irq;
> +	struct mbox_client mbox_client;
> +	struct mbox_chan *mbox_chan;
> +};

https://docs.kernel.org/process/maintainer-tip.html#struct-declarations-and-initializers

Please read the rest of this document too.

> +
> +/*
> + * Singleton - there is only one MPM instance per SoC. msm8660_mpm_get()
> + * returns this. Updates are serialised through the binding lifecycle so
> + * a plain pointer is sufficient.
> + */
> +static struct msm8660_mpm *msm8660_mpm_global;
> +
> +static u32 msm8660_mpm_read(struct msm8660_mpm *mpm, unsigned int reg)
> +{
> +	return readl_relaxed(mpm->base + reg);
> +}
> +
> +static void msm8660_mpm_write(struct msm8660_mpm *mpm, unsigned int reg,
> +			      u32 val)

No line break required. You have 100 characters. All over the place.

> +static int msm8660_mpm_pin_to_hwirq(struct msm8660_mpm *mpm, int pin)
> +{
> +	int i;
> +
> +	for (i = 0; i < mpm->pin_map_count; i++) {

for (int i = 0; ....

> +		if (mpm->pin_map[i].pin == pin)
> +			return mpm->pin_map[i].hwirq;
> +	}
> +	return -ENOENT;
> +}
> +
> +/*
> + * IPC handler: MPM fires this IRQ when one or more enabled wake pins
> + * have pending activity. Read pending status, CLEAR the pending bits
> + * BEFORE dispatching the per-pin handlers so a fresh edge that arrives
> + * during dispatch cannot be wiped out by a later CLEAR write, then
> + * replay each pending pin through the irqdomain.
> + */
> +static irqreturn_t msm8660_mpm_irq(int irq, void *data)
> +{
> +	struct msm8660_mpm *mpm = data;
> +	unsigned long pending[MSM8660_MPM_REG_WIDTH];
> +	unsigned long enable[MSM8660_MPM_REG_WIDTH];
> +	int i, j;

See documented variable ordering and put the iterator variables into context.

> +static void msm8660_mpm_enable_hwirq(struct irq_data *d, bool enable)
> +{
> +	struct msm8660_mpm *mpm = irq_data_get_irq_chip_data(d);
> +	int pin;
> +	u32 val, mask;

See docs

> +static int msm8660_mpm_domain_alloc(struct irq_domain *domain,
> +				    unsigned int virq, unsigned int nr_irqs,
> +				    void *data)
> +{
> +	struct msm8660_mpm *mpm = domain->host_data;
> +	struct irq_fwspec *fwspec = data;
> +	struct irq_fwspec parent_fwspec;
> +	irq_hw_number_t hwirq;
> +	int i, ret;
> +
> +	if (fwspec->param_count != 2)
> +		return -EINVAL;
> +
> +	hwirq = fwspec->param[0];
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &msm8660_mpm_chip, mpm);

See bracket rules.

> +static void msm8660_mpm_remove(struct platform_device *pdev)
> +{
> +	struct msm8660_mpm *mpm = platform_get_drvdata(pdev);
> +
> +	/*
> +	 * Tear down in strict reverse order: drop the singleton so new
> +	 * consumers cannot grab a handle, free the IRQ so the handler

How is that serialized against a concurrent consumer request?

Also please look at:

   https://sashiko.dev/#/message/20260531043213.D18801F00893%40smtp.kernel.org

Thanks,

        tglx