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
