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

[Thomas Gleixner <tglx@kernel.org>]

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