Re: [PATCH 5/7] irqchip/renesas-rzg2l: cache registers on suspend/resume

[Thomas Gleixner]

On Mon, Oct 23 2023 at 13:22, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> Cache registers content when going to suspend and restore them in resume
> as these may be lost when switching to deep sleep states. With this
> driver data has been marked as static to be able to play with it
> in struct syscon_ops::{suspend, resume}.

I have no idea what you are trying to tell me here. Why do the
suspend/resume callbacks need a static data structure and cannot operate
on a pointer which wastes less builtin memory when the driver is not
used?

Also "play with it" is definitely not a technical term. See
Documentation/process/* which has lots of explanations how to write
proper change logs.

> Because IA55 input is from pin controller and IA55 resumes before pin
> controller we don't restore interrupt enable bits here but let the
> pinctrl to do it on IA55 behalf after pins are in proper state to
> avoid invalid interrupts.
> +
> +static struct rzg2l_irqc_priv {
>  	void __iomem *base;
>  	struct irq_fwspec fwspec[IRQC_NUM_IRQ];
>  	raw_spinlock_t lock;
> -};
> +	struct rzg2l_irqc_reg_cache cache;
> +} priv;

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

>  
>  static struct rzg2l_irqc_priv *irq_data_to_priv(struct irq_data *data)
>  {
> @@ -238,6 +251,37 @@ static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type)
>  	return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH);
>  }
>  
> +static int rzg2l_irqc_irq_suspend(void)
> +{
> +	struct rzg2l_irqc_reg_cache *cache = &priv.cache;
> +
> +	cache->iitsr = readl_relaxed(priv.base + IITSR);
> +	for (u8 i = 0; i < 2; i++)
> +		cache->titsr[i] = readl_relaxed(priv.base + TITSR(i));
> +
> +	return 0;
> +}
> +
> +static void rzg2l_irqc_irq_resume(void)
> +{
> +	struct rzg2l_irqc_reg_cache *cache = &priv.cache;
> +	u8 i;
> +
> +	/*
> +	 * Restore only interrupt type. TSSRx will be restored at the
> +	 * request of pin controller to avoid spurious interrupts due
> +	 * to invalid PIN states.
> +	 */
> +	for (i = 0; i < 2; i++)
> +		writel_relaxed(cache->titsr[i], priv.base + TITSR(i));
> +	writel_relaxed(cache->iitsr, priv.base + IITSR);
> +}
> +
> +static struct syscore_ops rzg2l_irqc_syscore_ops = {
> +	.suspend = rzg2l_irqc_irq_suspend,
> +	.resume = rzg2l_irqc_irq_resume,
> +};

Ditto.

>  static const struct irq_chip irqc_chip = {
>  	.name			= "rzg2l-irqc",
>  	.irq_eoi		= rzg2l_irqc_eoi,
> @@ -323,7 +367,6 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
>  	struct irq_domain *irq_domain, *parent_domain;
>  	struct platform_device *pdev;
>  	struct reset_control *resetn;
> -	struct rzg2l_irqc_priv *priv;

Make this pointer static at the top level and leave the rest of the code
alone and please give it a proper name. "priv" at the file level is
really non-descriptive.

Thanks,

        tglx