Re: [PATCH 2/2] irqchip: ti-sci-inta: add runtime PM and system sleep support

Thomas Gleixner
2026-05-05T19:33:31Z

On Wed, Apr 29 2026 at 23:19, Rahul Sharma wrote:

Please use the proper subsystem prefix as documented:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

> Register runtime PM callbacks and enable runtime PM via
> devm_pm_runtime_enable() in probe.
>
> runtime_suspend is a no-op; IRQ routing context is preserved by TI SCI

s/IRQ/Interrupt/

A change log is written in prose and not an aggregation of random
acronyms. This is not twatter.

> firmware across power-gate cycles.
>
> runtime_resume restores VINT_ENABLE_SET for each active event bit,
> skipping IRQs with irqd_irq_masked set to avoid re-enabling
> intentionally disabled interrupts.
>
> System sleep reuses these callbacks via pm_runtime_force_suspend/resume
> as late/early sleep ops. This ensures MMIO writes in runtime_resume
> happen after genpd restores the power domain (dpm_resume_noirq),
> avoiding writes to a powered-off device.

TBH, I fails to decode the above word salad. Please check the above
linked documentation for hints how to structure change logs.

> +static int ti_sci_inta_runtime_suspend(struct device *dev)
> +{
> + return 0;

This clearly lacks a comment why this function is empty, while the
counterpart is not.

> +}
> +
> +static int ti_sci_inta_runtime_resume(struct device *dev)
> +{
> + struct ti_sci_inta_irq_domain *inta = dev_get_drvdata(dev);
> + struct ti_sci_inta_vint_desc *vint_desc;
> + int bit;
> +
> + mutex_lock(&inta->vint_mutex);

guard(mutex)(....);

> + list_for_each_entry(vint_desc, &inta->vint_list, list) {
> + for_each_set_bit(bit, vint_desc->event_map, MAX_EVENTS_PER_VINT) {
> + unsigned int virq;
> + struct irq_data *data;

See 'Variable declarations' in the linked document

> + virq = irq_find_mapping(vint_desc->domain,
> + vint_desc->events[bit].hwirq);

No line break required. You have 100 characters.

> + if (!virq)
> + continue;
> + data = irq_get_irq_data(virq);
> + if (!data || irqd_irq_masked(data))
> + continue;

This is a blatant abuse of the interrupt internals.

Why can't you keep track of the current state in

vint_desc->events[bit].XXXXX

and be done with it?

> + writeq_relaxed(BIT(bit), inta->base +
> + vint_desc->vint_id * 0x1000 +
> + VINT_ENABLE_SET_OFFSET);

Ditto.

> + }
> + }
> + mutex_unlock(&inta->vint_mutex);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ti_sci_inta_pm_ops = {
> + SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> + SET_RUNTIME_PM_OPS(ti_sci_inta_runtime_suspend,
> + ti_sci_inta_runtime_resume, NULL)
> +};
> +
> static const struct of_device_id ti_sci_inta_irq_domain_of_match[] = {
> { .compatible = "ti,sci-inta", },
> { /* sentinel */ },
> @@ -736,6 +784,7 @@ static struct platform_driver ti_sci_inta_irq_domain_driver = {
> .driver = {
> .name = "ti-sci-inta",
> .of_match_table = ti_sci_inta_irq_domain_of_match,
> + .pm = pm_ptr(&ti_sci_inta_pm_ops),

See 'Struct declarations and initializers' ....

Thanks,

tglx