Re: [PATCH v2 3/4] pinctrl: samsung: add gs101 specific eint suspend/resume callbacks

[André Draszik]

On Sat, 2025-03-01 at 11:43 +0000, Peter Griffin wrote:
> gs101 differs to other SoCs in that fltcon1 register doesn't
> always exist. Additionally the offset of fltcon0 is not fixed
> and needs to use the newly added eint_fltcon_offset variable.
> 
> Signed-off-by: Peter Griffin <peter.griffin@xxxxxxxxxx>
> Fixes: 4a8be01a1a7a ("pinctrl: samsung: Add gs101 SoC pinctrl configuration")
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  drivers/pinctrl/samsung/pinctrl-exynos-arm64.c | 24 ++++-----
>  drivers/pinctrl/samsung/pinctrl-exynos.c       | 71 ++++++++++++++++++++++++++
>  drivers/pinctrl/samsung/pinctrl-exynos.h       |  2 +
>  3 files changed, 85 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> index 57c98d2451b5..fca447ebc5f5 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> @@ -1455,15 +1455,15 @@ static const struct samsung_pin_ctrl gs101_pin_ctrl[] __initconst = {
>  		.pin_banks	= gs101_pin_alive,
>  		.nr_banks	= ARRAY_SIZE(gs101_pin_alive),
>  		.eint_wkup_init = exynos_eint_wkup_init,
> -		.suspend	= exynos_pinctrl_suspend,
> -		.resume		= exynos_pinctrl_resume,
> +		.suspend	= gs101_pinctrl_suspend,
> +		.resume		= gs101_pinctrl_resume,
>  	}, {
>  		/* pin banks of gs101 pin-controller (FAR_ALIVE) */
>  		.pin_banks	= gs101_pin_far_alive,
>  		.nr_banks	= ARRAY_SIZE(gs101_pin_far_alive),
>  		.eint_wkup_init = exynos_eint_wkup_init,
> -		.suspend	= exynos_pinctrl_suspend,
> -		.resume		= exynos_pinctrl_resume,
> +		.suspend	= gs101_pinctrl_suspend,
> +		.resume		= gs101_pinctrl_resume,
>  	}, {
>  		/* pin banks of gs101 pin-controller (GSACORE) */
>  		.pin_banks	= gs101_pin_gsacore,
> @@ -1477,29 +1477,29 @@ static const struct samsung_pin_ctrl gs101_pin_ctrl[] __initconst = {
>  		.pin_banks	= gs101_pin_peric0,
>  		.nr_banks	= ARRAY_SIZE(gs101_pin_peric0),
>  		.eint_gpio_init = exynos_eint_gpio_init,
> -		.suspend	= exynos_pinctrl_suspend,
> -		.resume		= exynos_pinctrl_resume,
> +		.suspend	= gs101_pinctrl_suspend,
> +		.resume		= gs101_pinctrl_resume,
>  	}, {
>  		/* pin banks of gs101 pin-controller (PERIC1) */
>  		.pin_banks	= gs101_pin_peric1,
>  		.nr_banks	= ARRAY_SIZE(gs101_pin_peric1),
>  		.eint_gpio_init = exynos_eint_gpio_init,
> -		.suspend	= exynos_pinctrl_suspend,
> -		.resume	= exynos_pinctrl_resume,
> +		.suspend	= gs101_pinctrl_suspend,
> +		.resume		= gs101_pinctrl_resume,
>  	}, {
>  		/* pin banks of gs101 pin-controller (HSI1) */
>  		.pin_banks	= gs101_pin_hsi1,
>  		.nr_banks	= ARRAY_SIZE(gs101_pin_hsi1),
>  		.eint_gpio_init = exynos_eint_gpio_init,
> -		.suspend	= exynos_pinctrl_suspend,
> -		.resume		= exynos_pinctrl_resume,
> +		.suspend	= gs101_pinctrl_suspend,
> +		.resume		= gs101_pinctrl_resume,
>  	}, {
>  		/* pin banks of gs101 pin-controller (HSI2) */
>  		.pin_banks	= gs101_pin_hsi2,
>  		.nr_banks	= ARRAY_SIZE(gs101_pin_hsi2),
>  		.eint_gpio_init = exynos_eint_gpio_init,
> -		.suspend	= exynos_pinctrl_suspend,
> -		.resume		= exynos_pinctrl_resume,
> +		.suspend	= gs101_pinctrl_suspend,
> +		.resume		= gs101_pinctrl_resume,
>  	},
>  };
>  
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> index d65a9fba0781..ddc7245ec2e5 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -801,6 +801,41 @@ void exynos_pinctrl_suspend(struct samsung_pin_bank *bank)
>  	}
>  }
>  
> +void gs101_pinctrl_suspend(struct samsung_pin_bank *bank)
> +{
> +	struct exynos_eint_gpio_save *save = bank->soc_priv;
> +	const void __iomem *regs = bank->eint_base;
> +
> +	exynos_set_wakeup(bank);

As for patch 2, would be good to have this if / else, to make it more
obvious that this is conditional, too.

> +
> +	if (bank->eint_type == EINT_TYPE_GPIO) {
> +		save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
> +				       + bank->eint_offset);
> +
> +		save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET +
> +					   bank->eint_fltcon_offset);

If there's another version, maybe align style where the '+'
is placed (end of line vs start of line). It seems this file
generally uses + at start of line.

> +
> +		/* fltcon1 register only exists for pins 4-7 */
> +		if (bank->nr_pins > 4)
> +			save->eint_fltcon1 = readl(regs +
> +						EXYNOS_GPIO_EFLTCON_OFFSET +
> +						bank->eint_fltcon_offset + 4);
> +
> +		save->eint_mask = readl(regs + bank->irq_chip->eint_mask
> +					+ bank->eint_offset);
> +
> +		pr_debug("%s: save     con %#010x\n",
> +			 bank->name, save->eint_con);
> +		pr_debug("%s: save fltcon0 %#010x\n",
> +			 bank->name, save->eint_fltcon0);
> +		if (bank->nr_pins > 4)
> +			pr_debug("%s: save fltcon1 %#010x\n",
> +				 bank->name, save->eint_fltcon1);
> +		pr_debug("%s: save    mask %#010x\n",
> +			 bank->name, save->eint_mask);
> +	}
> +}
> +
>  void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank)
>  {
>  	struct exynos_eint_gpio_save *save = bank->soc_priv;
> @@ -820,6 +855,42 @@ void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank)
>  	}
>  }
>  
> +void gs101_pinctrl_resume(struct samsung_pin_bank *bank)
> +{
> +	struct exynos_eint_gpio_save *save = bank->soc_priv;
> +
> +	void __iomem *regs = bank->eint_base;
> +	void __iomem *eint_fltcfg0 = regs + EXYNOS_GPIO_EFLTCON_OFFSET
> +		     + bank->eint_fltcon_offset;
> +
> +	if (bank->eint_type == EINT_TYPE_GPIO) {
> +		pr_debug("%s:     con %#010x => %#010x\n", bank->name,
> +			 readl(regs + EXYNOS_GPIO_ECON_OFFSET
> +			       + bank->eint_offset), save->eint_con);
> +
> +		pr_debug("%s: fltcon0 %#010x => %#010x\n", bank->name,
> +			 readl(eint_fltcfg0), save->eint_fltcon0);
> +
> +		/* fltcon1 register only exists for pins 4-7 */
> +		if (bank->nr_pins > 4) {
> +			pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
> +				 readl(eint_fltcfg0 + 4), save->eint_fltcon1);
> +		}

If there's another version, braces are not needed here.

Cheers,
Andre'


> +		pr_debug("%s:    mask %#010x => %#010x\n", bank->name,
> +			 readl(regs + bank->irq_chip->eint_mask
> +			       + bank->eint_offset), save->eint_mask);
> +
> +		writel(save->eint_con, regs + EXYNOS_GPIO_ECON_OFFSET
> +		       + bank->eint_offset);
> +		writel(save->eint_fltcon0, eint_fltcfg0);
> +
> +		if (bank->nr_pins > 4)
> +			writel(save->eint_fltcon1, eint_fltcfg0 + 4);
> +		writel(save->eint_mask, regs + bank->irq_chip->eint_mask
> +		       + bank->eint_offset);
> +	}
> +}
> +
>  void exynos_pinctrl_resume(struct samsung_pin_bank *bank)
>  {
>  	struct exynos_eint_gpio_save *save = bank->soc_priv;
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
> index 35c2bc4ea488..773f161a82a3 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.h
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
> @@ -225,6 +225,8 @@ void exynosautov920_pinctrl_resume(struct samsung_pin_bank *bank);
>  void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank);
>  void exynos_pinctrl_suspend(struct samsung_pin_bank *bank);
>  void exynos_pinctrl_resume(struct samsung_pin_bank *bank);
> +void gs101_pinctrl_suspend(struct samsung_pin_bank *bank);
> +void gs101_pinctrl_resume(struct samsung_pin_bank *bank);
>  struct samsung_retention_ctrl *
>  exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata,
>  		      const struct samsung_retention_data *data);
>