gs101 needs it's own suspend/resume callbacks to use the newly
added eint_fltcon_offset for saving & restoring fltcon0 & fltcon1
registers. It also differs to previous SoCs in that fltcon1
register doesn't always exist for each bank.
exynosautov920 also has dedicated logic for using eint_con_offset
and eint_mask_offset for saving & restoring it's registers.
Refactor the existing platform specific suspend/resume callback
so that each SoC variant has their own callback containing the
SoC specific logic.
Additionally we now call drvdata->suspend() & drvdata->resume()
from within the loop that iterates the banks in
samsung_pinctrl_suspend() and samsung_pinctrl_resume().
This simplifies the logic, and allows us to remove the
clk_enable() and clk_disable() from the callbacks.
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
Changes since v1:
* Split code refactor & gs101 parts into separate patches (Andre)
---
drivers/pinctrl/samsung/pinctrl-exynos-arm64.c | 28 ++--
drivers/pinctrl/samsung/pinctrl-exynos.c | 201 ++++++++++---------------
drivers/pinctrl/samsung/pinctrl-exynos.h | 6 +-
drivers/pinctrl/samsung/pinctrl-samsung.c | 11 +-
drivers/pinctrl/samsung/pinctrl-samsung.h | 8 +-
5 files changed, 111 insertions(+), 143 deletions(-)
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
index e28fe8177646..57c98d2451b5 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
@@ -1112,8 +1112,8 @@ static const struct samsung_pin_ctrl exynosautov920_pin_ctrl[] = {
.pin_banks = exynosautov920_pin_banks0,
.nr_banks = ARRAY_SIZE(exynosautov920_pin_banks0),
.eint_wkup_init = exynos_eint_wkup_init,
- .suspend = exynos_pinctrl_suspend,
- .resume = exynos_pinctrl_resume,
+ .suspend = exynosautov920_pinctrl_suspend,
+ .resume = exynosautov920_pinctrl_resume,
.retention_data = &exynosautov920_retention_data,
}, {
/* pin-controller instance 1 AUD data */
@@ -1124,43 +1124,43 @@ static const struct samsung_pin_ctrl exynosautov920_pin_ctrl[] = {
.pin_banks = exynosautov920_pin_banks2,
.nr_banks = ARRAY_SIZE(exynosautov920_pin_banks2),
.eint_gpio_init = exynos_eint_gpio_init,
- .suspend = exynos_pinctrl_suspend,
- .resume = exynos_pinctrl_resume,
+ .suspend = exynosautov920_pinctrl_suspend,
+ .resume = exynosautov920_pinctrl_resume,
}, {
/* pin-controller instance 3 HSI1 data */
.pin_banks = exynosautov920_pin_banks3,
.nr_banks = ARRAY_SIZE(exynosautov920_pin_banks3),
.eint_gpio_init = exynos_eint_gpio_init,
- .suspend = exynos_pinctrl_suspend,
- .resume = exynos_pinctrl_resume,
+ .suspend = exynosautov920_pinctrl_suspend,
+ .resume = exynosautov920_pinctrl_resume,
}, {
/* pin-controller instance 4 HSI2 data */
.pin_banks = exynosautov920_pin_banks4,
.nr_banks = ARRAY_SIZE(exynosautov920_pin_banks4),
.eint_gpio_init = exynos_eint_gpio_init,
- .suspend = exynos_pinctrl_suspend,
- .resume = exynos_pinctrl_resume,
+ .suspend = exynosautov920_pinctrl_suspend,
+ .resume = exynosautov920_pinctrl_resume,
}, {
/* pin-controller instance 5 HSI2UFS data */
.pin_banks = exynosautov920_pin_banks5,
.nr_banks = ARRAY_SIZE(exynosautov920_pin_banks5),
.eint_gpio_init = exynos_eint_gpio_init,
- .suspend = exynos_pinctrl_suspend,
- .resume = exynos_pinctrl_resume,
+ .suspend = exynosautov920_pinctrl_suspend,
+ .resume = exynosautov920_pinctrl_resume,
}, {
/* pin-controller instance 6 PERIC0 data */
.pin_banks = exynosautov920_pin_banks6,
.nr_banks = ARRAY_SIZE(exynosautov920_pin_banks6),
.eint_gpio_init = exynos_eint_gpio_init,
- .suspend = exynos_pinctrl_suspend,
- .resume = exynos_pinctrl_resume,
+ .suspend = exynosautov920_pinctrl_suspend,
+ .resume = exynosautov920_pinctrl_resume,
}, {
/* pin-controller instance 7 PERIC1 data */
.pin_banks = exynosautov920_pin_banks7,
.nr_banks = ARRAY_SIZE(exynosautov920_pin_banks7),
.eint_gpio_init = exynos_eint_gpio_init,
- .suspend = exynos_pinctrl_suspend,
- .resume = exynos_pinctrl_resume,
+ .suspend = exynosautov920_pinctrl_suspend,
+ .resume = exynosautov920_pinctrl_resume,
},
};
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index ac6dc22b37c9..d65a9fba0781 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -761,153 +761,118 @@ __init int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
return 0;
}
-static void exynos_pinctrl_suspend_bank(
- struct samsung_pinctrl_drv_data *drvdata,
- struct samsung_pin_bank *bank)
+static void exynos_set_wakeup(struct samsung_pin_bank *bank)
{
- struct exynos_eint_gpio_save *save = bank->soc_priv;
- const void __iomem *regs = bank->eint_base;
+ struct exynos_irq_chip *irq_chip = NULL;
- if (clk_enable(bank->drvdata->pclk)) {
- dev_err(bank->gpio_chip.parent,
- "unable to enable clock for saving state\n");
- return;
+ if (bank->eint_type == EINT_TYPE_WKUP) {
+ if (bank->irq_chip) {
+ irq_chip = bank->irq_chip;
+ irq_chip->set_eint_wakeup_mask(bank->drvdata, irq_chip);
+ }
}
-
- save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
- + bank->eint_offset);
- save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
- + 2 * bank->eint_offset);
- save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
- + 2 * bank->eint_offset + 4);
- save->eint_mask = readl(regs + bank->irq_chip->eint_mask
- + bank->eint_offset);
-
- clk_disable(bank->drvdata->pclk);
-
- pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
- pr_debug("%s: save fltcon0 %#010x\n", bank->name, save->eint_fltcon0);
- pr_debug("%s: save fltcon1 %#010x\n", bank->name, save->eint_fltcon1);
- pr_debug("%s: save mask %#010x\n", bank->name, save->eint_mask);
}
-static void exynosauto_pinctrl_suspend_bank(struct samsung_pinctrl_drv_data *drvdata,
- struct samsung_pin_bank *bank)
+void exynos_pinctrl_suspend(struct samsung_pin_bank *bank)
{
struct exynos_eint_gpio_save *save = bank->soc_priv;
const void __iomem *regs = bank->eint_base;
- if (clk_enable(bank->drvdata->pclk)) {
- dev_err(bank->gpio_chip.parent,
- "unable to enable clock for saving state\n");
- return;
+ exynos_set_wakeup(bank);
+
+ 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
+ + 2 * bank->eint_offset);
+ save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+ + 2 * bank->eint_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);
+ pr_debug("%s: save fltcon1 %#010x\n",
+ bank->name, save->eint_fltcon1);
+ pr_debug("%s: save mask %#010x\n",
+ bank->name, save->eint_mask);
}
-
- save->eint_con = readl(regs + bank->pctl_offset + bank->eint_con_offset);
- save->eint_mask = readl(regs + bank->pctl_offset + bank->eint_mask_offset);
-
- clk_disable(bank->drvdata->pclk);
-
- pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
- pr_debug("%s: save mask %#010x\n", bank->name, save->eint_mask);
}
-void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
+void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank)
{
- struct samsung_pin_bank *bank = drvdata->pin_banks;
- struct exynos_irq_chip *irq_chip = NULL;
- int i;
+ struct exynos_eint_gpio_save *save = bank->soc_priv;
+ const void __iomem *regs = bank->eint_base;
- for (i = 0; i < drvdata->nr_banks; ++i, ++bank) {
- if (bank->eint_type == EINT_TYPE_GPIO) {
- if (bank->eint_con_offset)
- exynosauto_pinctrl_suspend_bank(drvdata, bank);
- else
- exynos_pinctrl_suspend_bank(drvdata, bank);
- }
- else if (bank->eint_type == EINT_TYPE_WKUP) {
- if (!irq_chip) {
- irq_chip = bank->irq_chip;
- irq_chip->set_eint_wakeup_mask(drvdata,
- irq_chip);
- }
- }
+ exynos_set_wakeup(bank);
+
+ if (bank->eint_type == EINT_TYPE_GPIO) {
+ save->eint_con = readl(regs + bank->pctl_offset +
+ bank->eint_con_offset);
+ save->eint_mask = readl(regs + bank->pctl_offset +
+ bank->eint_mask_offset);
+ pr_debug("%s: save con %#010x\n",
+ bank->name, save->eint_con);
+ pr_debug("%s: save mask %#010x\n",
+ bank->name, save->eint_mask);
}
}
-static void exynos_pinctrl_resume_bank(
- struct samsung_pinctrl_drv_data *drvdata,
- struct samsung_pin_bank *bank)
+void exynos_pinctrl_resume(struct samsung_pin_bank *bank)
{
struct exynos_eint_gpio_save *save = bank->soc_priv;
void __iomem *regs = bank->eint_base;
- if (clk_enable(bank->drvdata->pclk)) {
- dev_err(bank->gpio_chip.parent,
- "unable to enable clock for restoring state\n");
- return;
+ 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+ + 2 * bank->eint_offset), save->eint_fltcon0);
+ pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
+ readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+ + 2 * bank->eint_offset + 4),
+ save->eint_fltcon1);
+ 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
+ + 2 * bank->eint_offset);
+ writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
+ + 2 * bank->eint_offset + 4);
+ writel(save->eint_mask, regs + bank->irq_chip->eint_mask
+ + bank->eint_offset);
}
-
- 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
- + 2 * bank->eint_offset), save->eint_fltcon0);
- pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
- readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
- + 2 * bank->eint_offset + 4), save->eint_fltcon1);
- 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
- + 2 * bank->eint_offset);
- writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
- + 2 * bank->eint_offset + 4);
- writel(save->eint_mask, regs + bank->irq_chip->eint_mask
- + bank->eint_offset);
-
- clk_disable(bank->drvdata->pclk);
}
-static void exynosauto_pinctrl_resume_bank(struct samsung_pinctrl_drv_data *drvdata,
- struct samsung_pin_bank *bank)
+void exynosautov920_pinctrl_resume(struct samsung_pin_bank *bank)
{
struct exynos_eint_gpio_save *save = bank->soc_priv;
void __iomem *regs = bank->eint_base;
- if (clk_enable(bank->drvdata->pclk)) {
- dev_err(bank->gpio_chip.parent,
- "unable to enable clock for restoring state\n");
- return;
+ if (bank->eint_type == EINT_TYPE_GPIO) {
+ /* exynosautov920 has eint_con_offset for all but one bank */
+ if (!bank->eint_con_offset)
+ exynos_pinctrl_resume(bank);
+
+ pr_debug("%s: con %#010x => %#010x\n", bank->name,
+ readl(regs + bank->pctl_offset + bank->eint_con_offset),
+ save->eint_con);
+ pr_debug("%s: mask %#010x => %#010x\n", bank->name,
+ readl(regs + bank->pctl_offset +
+ bank->eint_mask_offset), save->eint_mask);
+
+ writel(save->eint_con,
+ regs + bank->pctl_offset + bank->eint_con_offset);
+ writel(save->eint_mask,
+ regs + bank->pctl_offset + bank->eint_mask_offset);
}
-
- pr_debug("%s: con %#010x => %#010x\n", bank->name,
- readl(regs + bank->pctl_offset + bank->eint_con_offset), save->eint_con);
- pr_debug("%s: mask %#010x => %#010x\n", bank->name,
- readl(regs + bank->pctl_offset + bank->eint_mask_offset), save->eint_mask);
-
- writel(save->eint_con, regs + bank->pctl_offset + bank->eint_con_offset);
- writel(save->eint_mask, regs + bank->pctl_offset + bank->eint_mask_offset);
-
- clk_disable(bank->drvdata->pclk);
-}
-
-void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
-{
- struct samsung_pin_bank *bank = drvdata->pin_banks;
- int i;
-
- for (i = 0; i < drvdata->nr_banks; ++i, ++bank)
- if (bank->eint_type == EINT_TYPE_GPIO) {
- if (bank->eint_con_offset)
- exynosauto_pinctrl_resume_bank(drvdata, bank);
- else
- exynos_pinctrl_resume_bank(drvdata, bank);
- }
}
static void exynos_retention_enable(struct samsung_pinctrl_drv_data *drvdata)
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
index 33df21d5c9d6..35c2bc4ea488 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.h
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
@@ -221,8 +221,10 @@ struct exynos_muxed_weint_data {
int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d);
int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d);
-void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata);
-void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata);
+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);
struct samsung_retention_ctrl *
exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata,
const struct samsung_retention_data *data);
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index 963060920301..375634d8cc79 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -1349,6 +1349,9 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
const u8 *widths = bank->type->fld_width;
enum pincfg_type type;
+ if (drvdata->suspend)
+ drvdata->suspend(bank);
+
/* Registers without a powerdown config aren't lost */
if (!widths[PINCFG_TYPE_CON_PDN])
continue;
@@ -1373,8 +1376,6 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
clk_disable(drvdata->pclk);
- if (drvdata->suspend)
- drvdata->suspend(drvdata);
if (drvdata->retention_ctrl && drvdata->retention_ctrl->enable)
drvdata->retention_ctrl->enable(drvdata);
@@ -1406,9 +1407,6 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
return ret;
}
- if (drvdata->resume)
- drvdata->resume(drvdata);
-
for (i = 0; i < drvdata->nr_banks; i++) {
struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
void __iomem *reg = bank->pctl_base + bank->pctl_offset;
@@ -1416,6 +1414,9 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
const u8 *widths = bank->type->fld_width;
enum pincfg_type type;
+ if (drvdata->resume)
+ drvdata->resume(bank);
+
/* Registers without a powerdown config aren't lost */
if (!widths[PINCFG_TYPE_CON_PDN])
continue;
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 371e4f02bbfb..e939e5bb0347 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -285,8 +285,8 @@ struct samsung_pin_ctrl {
int (*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
int (*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
void (*pud_value_init)(struct samsung_pinctrl_drv_data *drvdata);
- void (*suspend)(struct samsung_pinctrl_drv_data *);
- void (*resume)(struct samsung_pinctrl_drv_data *);
+ void (*suspend)(struct samsung_pin_bank *bank);
+ void (*resume)(struct samsung_pin_bank *bank);
};
/**
@@ -335,8 +335,8 @@ struct samsung_pinctrl_drv_data {
struct samsung_retention_ctrl *retention_ctrl;
- void (*suspend)(struct samsung_pinctrl_drv_data *);
- void (*resume)(struct samsung_pinctrl_drv_data *);
+ void (*suspend)(struct samsung_pin_bank *bank);
+ void (*resume)(struct samsung_pin_bank *bank);
};
/**
--
2.48.1.711.g2feabab25a-googOn Sat, 2025-03-01 at 11:43 +0000, Peter Griffin wrote:
> gs101 needs it's own suspend/resume callbacks to use the newly
> added eint_fltcon_offset for saving & restoring fltcon0 & fltcon1
> registers. It also differs to previous SoCs in that fltcon1
> register doesn't always exist for each bank.
>
> exynosautov920 also has dedicated logic for using eint_con_offset
> and eint_mask_offset for saving & restoring it's registers.
>
> Refactor the existing platform specific suspend/resume callback
> so that each SoC variant has their own callback containing the
> SoC specific logic.
>
> Additionally we now call drvdata->suspend() & drvdata->resume()
> from within the loop that iterates the banks in
> samsung_pinctrl_suspend() and samsung_pinctrl_resume().
>
> This simplifies the logic, and allows us to remove the
> clk_enable() and clk_disable() from the callbacks.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> Changes since v1:
> * Split code refactor & gs101 parts into separate patches (Andre)
> ---
> drivers/pinctrl/samsung/pinctrl-exynos-arm64.c | 28 ++--
> drivers/pinctrl/samsung/pinctrl-exynos.c | 201 ++++++++++---------------
> drivers/pinctrl/samsung/pinctrl-exynos.h | 6 +-
> drivers/pinctrl/samsung/pinctrl-samsung.c | 11 +-
> drivers/pinctrl/samsung/pinctrl-samsung.h | 8 +-
> 5 files changed, 111 insertions(+), 143 deletions(-)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> index e28fe8177646..57c98d2451b5 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> @@ -1112,8 +1112,8 @@ static const struct samsung_pin_ctrl exynosautov920_pin_ctrl[] = {
> .pin_banks = exynosautov920_pin_banks0,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks0),
> .eint_wkup_init = exynos_eint_wkup_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> .retention_data = &exynosautov920_retention_data,
> }, {
> /* pin-controller instance 1 AUD data */
> @@ -1124,43 +1124,43 @@ static const struct samsung_pin_ctrl exynosautov920_pin_ctrl[] = {
> .pin_banks = exynosautov920_pin_banks2,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks2),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 3 HSI1 data */
> .pin_banks = exynosautov920_pin_banks3,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks3),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 4 HSI2 data */
> .pin_banks = exynosautov920_pin_banks4,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks4),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 5 HSI2UFS data */
> .pin_banks = exynosautov920_pin_banks5,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks5),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 6 PERIC0 data */
> .pin_banks = exynosautov920_pin_banks6,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks6),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 7 PERIC1 data */
> .pin_banks = exynosautov920_pin_banks7,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks7),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> },
> };
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> index ac6dc22b37c9..d65a9fba0781 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -761,153 +761,118 @@ __init int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
> return 0;
> }
>
> -static void exynos_pinctrl_suspend_bank(
> - struct samsung_pinctrl_drv_data *drvdata,
> - struct samsung_pin_bank *bank)
> +static void exynos_set_wakeup(struct samsung_pin_bank *bank)
> {
> - struct exynos_eint_gpio_save *save = bank->soc_priv;
> - const void __iomem *regs = bank->eint_base;
> + struct exynos_irq_chip *irq_chip = NULL;
This is a useless init and the value is overwritten right away.
>
> - if (clk_enable(bank->drvdata->pclk)) {
> - dev_err(bank->gpio_chip.parent,
> - "unable to enable clock for saving state\n");
> - return;
> + if (bank->eint_type == EINT_TYPE_WKUP) {
> + if (bank->irq_chip) {
> + irq_chip = bank->irq_chip;
> + irq_chip->set_eint_wakeup_mask(bank->drvdata, irq_chip);
> + }
> }
> -
> - save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
> - + bank->eint_offset);
> - save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset);
> - save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset + 4);
> - save->eint_mask = readl(regs + bank->irq_chip->eint_mask
> - + bank->eint_offset);
> -
> - clk_disable(bank->drvdata->pclk);
> -
> - pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
> - pr_debug("%s: save fltcon0 %#010x\n", bank->name, save->eint_fltcon0);
> - pr_debug("%s: save fltcon1 %#010x\n", bank->name, save->eint_fltcon1);
> - pr_debug("%s: save mask %#010x\n", bank->name, save->eint_mask);
> }
>
> -static void exynosauto_pinctrl_suspend_bank(struct samsung_pinctrl_drv_data *drvdata,
> - struct samsung_pin_bank *bank)
> +void exynos_pinctrl_suspend(struct samsung_pin_bank *bank)
> {
> struct exynos_eint_gpio_save *save = bank->soc_priv;
> const void __iomem *regs = bank->eint_base;
>
> - if (clk_enable(bank->drvdata->pclk)) {
> - dev_err(bank->gpio_chip.parent,
> - "unable to enable clock for saving state\n");
> - return;
> + exynos_set_wakeup(bank);
> +
> + if (bank->eint_type == EINT_TYPE_GPIO) {
Can you swap the order, and make the call to exynos_set_wakeup()
an else, to avoid some needless tests and make it clearer that
set_wakeup() is conditional as well, i.e.:
if (bank->eint_type == EINT_TYPE_GPIO) {
...
} else {
exynos_set_wakeup(bank);
}
What makes it hard to read is that as-is, it looks like the
set_wakeup() is unconditional, while it's also based on
bank->eint_type (which is also more obvious before your patch).
Or maybe even have exynos_set_wakeup() return a bool, as in
the follow-up gs101 patch, there are three tests of this type.
Other than that, I think I looks good.
Cheers,
Andre'
> + save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
> + + bank->eint_offset);
> + save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_offset);
> + save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_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);
> + pr_debug("%s: save fltcon1 %#010x\n",
> + bank->name, save->eint_fltcon1);
> + pr_debug("%s: save mask %#010x\n",
> + bank->name, save->eint_mask);
> }
> -
> - save->eint_con = readl(regs + bank->pctl_offset + bank->eint_con_offset);
> - save->eint_mask = readl(regs + bank->pctl_offset + bank->eint_mask_offset);
> -
> - clk_disable(bank->drvdata->pclk);
> -
> - pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
> - pr_debug("%s: save mask %#010x\n", bank->name, save->eint_mask);
> }
>
> -void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
> +void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank)
> {
> - struct samsung_pin_bank *bank = drvdata->pin_banks;
> - struct exynos_irq_chip *irq_chip = NULL;
> - int i;
> + struct exynos_eint_gpio_save *save = bank->soc_priv;
> + const void __iomem *regs = bank->eint_base;
>
> - for (i = 0; i < drvdata->nr_banks; ++i, ++bank) {
> - if (bank->eint_type == EINT_TYPE_GPIO) {
> - if (bank->eint_con_offset)
> - exynosauto_pinctrl_suspend_bank(drvdata, bank);
> - else
> - exynos_pinctrl_suspend_bank(drvdata, bank);
> - }
> - else if (bank->eint_type == EINT_TYPE_WKUP) {
> - if (!irq_chip) {
> - irq_chip = bank->irq_chip;
> - irq_chip->set_eint_wakeup_mask(drvdata,
> - irq_chip);
> - }
> - }
> + exynos_set_wakeup(bank);
> +
> + if (bank->eint_type == EINT_TYPE_GPIO) {
> + save->eint_con = readl(regs + bank->pctl_offset +
> + bank->eint_con_offset);
> + save->eint_mask = readl(regs + bank->pctl_offset +
> + bank->eint_mask_offset);
> + pr_debug("%s: save con %#010x\n",
> + bank->name, save->eint_con);
> + pr_debug("%s: save mask %#010x\n",
> + bank->name, save->eint_mask);
> }
> }
>
> -static void exynos_pinctrl_resume_bank(
> - struct samsung_pinctrl_drv_data *drvdata,
> - struct samsung_pin_bank *bank)
> +void exynos_pinctrl_resume(struct samsung_pin_bank *bank)
> {
> struct exynos_eint_gpio_save *save = bank->soc_priv;
> void __iomem *regs = bank->eint_base;
>
> - if (clk_enable(bank->drvdata->pclk)) {
> - dev_err(bank->gpio_chip.parent,
> - "unable to enable clock for restoring state\n");
> - return;
> + 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_offset), save->eint_fltcon0);
> + pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
> + readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_offset + 4),
> + save->eint_fltcon1);
> + 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_offset);
> + writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_offset + 4);
> + writel(save->eint_mask, regs + bank->irq_chip->eint_mask
> + + bank->eint_offset);
> }
> -
> - 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset), save->eint_fltcon0);
> - pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
> - readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset + 4), save->eint_fltcon1);
> - 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset);
> - writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset + 4);
> - writel(save->eint_mask, regs + bank->irq_chip->eint_mask
> - + bank->eint_offset);
> -
> - clk_disable(bank->drvdata->pclk);
> }
>
> -static void exynosauto_pinctrl_resume_bank(struct samsung_pinctrl_drv_data *drvdata,
> - struct samsung_pin_bank *bank)
> +void exynosautov920_pinctrl_resume(struct samsung_pin_bank *bank)
> {
> struct exynos_eint_gpio_save *save = bank->soc_priv;
> void __iomem *regs = bank->eint_base;
>
> - if (clk_enable(bank->drvdata->pclk)) {
> - dev_err(bank->gpio_chip.parent,
> - "unable to enable clock for restoring state\n");
> - return;
> + if (bank->eint_type == EINT_TYPE_GPIO) {
> + /* exynosautov920 has eint_con_offset for all but one bank */
> + if (!bank->eint_con_offset)
> + exynos_pinctrl_resume(bank);
> +
> + pr_debug("%s: con %#010x => %#010x\n", bank->name,
> + readl(regs + bank->pctl_offset + bank->eint_con_offset),
> + save->eint_con);
> + pr_debug("%s: mask %#010x => %#010x\n", bank->name,
> + readl(regs + bank->pctl_offset +
> + bank->eint_mask_offset), save->eint_mask);
> +
> + writel(save->eint_con,
> + regs + bank->pctl_offset + bank->eint_con_offset);
> + writel(save->eint_mask,
> + regs + bank->pctl_offset + bank->eint_mask_offset);
> }
> -
> - pr_debug("%s: con %#010x => %#010x\n", bank->name,
> - readl(regs + bank->pctl_offset + bank->eint_con_offset), save->eint_con);
> - pr_debug("%s: mask %#010x => %#010x\n", bank->name,
> - readl(regs + bank->pctl_offset + bank->eint_mask_offset), save->eint_mask);
> -
> - writel(save->eint_con, regs + bank->pctl_offset + bank->eint_con_offset);
> - writel(save->eint_mask, regs + bank->pctl_offset + bank->eint_mask_offset);
> -
> - clk_disable(bank->drvdata->pclk);
> -}
> -
> -void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
> -{
> - struct samsung_pin_bank *bank = drvdata->pin_banks;
> - int i;
> -
> - for (i = 0; i < drvdata->nr_banks; ++i, ++bank)
> - if (bank->eint_type == EINT_TYPE_GPIO) {
> - if (bank->eint_con_offset)
> - exynosauto_pinctrl_resume_bank(drvdata, bank);
> - else
> - exynos_pinctrl_resume_bank(drvdata, bank);
> - }
> }
>
> static void exynos_retention_enable(struct samsung_pinctrl_drv_data *drvdata)
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
> index 33df21d5c9d6..35c2bc4ea488 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.h
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
> @@ -221,8 +221,10 @@ struct exynos_muxed_weint_data {
>
> int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d);
> int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d);
> -void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata);
> -void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata);
> +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);
> struct samsung_retention_ctrl *
> exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata,
> const struct samsung_retention_data *data);
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 963060920301..375634d8cc79 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1349,6 +1349,9 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
> const u8 *widths = bank->type->fld_width;
> enum pincfg_type type;
>
> + if (drvdata->suspend)
> + drvdata->suspend(bank);
> +
> /* Registers without a powerdown config aren't lost */
> if (!widths[PINCFG_TYPE_CON_PDN])
> continue;
> @@ -1373,8 +1376,6 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
>
> clk_disable(drvdata->pclk);
>
> - if (drvdata->suspend)
> - drvdata->suspend(drvdata);
> if (drvdata->retention_ctrl && drvdata->retention_ctrl->enable)
> drvdata->retention_ctrl->enable(drvdata);
>
> @@ -1406,9 +1407,6 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
> return ret;
> }
>
> - if (drvdata->resume)
> - drvdata->resume(drvdata);
> -
> for (i = 0; i < drvdata->nr_banks; i++) {
> struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
> void __iomem *reg = bank->pctl_base + bank->pctl_offset;
> @@ -1416,6 +1414,9 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
> const u8 *widths = bank->type->fld_width;
> enum pincfg_type type;
>
> + if (drvdata->resume)
> + drvdata->resume(bank);
> +
> /* Registers without a powerdown config aren't lost */
> if (!widths[PINCFG_TYPE_CON_PDN])
> continue;
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> index 371e4f02bbfb..e939e5bb0347 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> @@ -285,8 +285,8 @@ struct samsung_pin_ctrl {
> int (*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
> int (*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
> void (*pud_value_init)(struct samsung_pinctrl_drv_data *drvdata);
> - void (*suspend)(struct samsung_pinctrl_drv_data *);
> - void (*resume)(struct samsung_pinctrl_drv_data *);
> + void (*suspend)(struct samsung_pin_bank *bank);
> + void (*resume)(struct samsung_pin_bank *bank);
> };
>
> /**
> @@ -335,8 +335,8 @@ struct samsung_pinctrl_drv_data {
>
> struct samsung_retention_ctrl *retention_ctrl;
>
> - void (*suspend)(struct samsung_pinctrl_drv_data *);
> - void (*resume)(struct samsung_pinctrl_drv_data *);
> + void (*suspend)(struct samsung_pin_bank *bank);
> + void (*resume)(struct samsung_pin_bank *bank);
> };
>
> /**
>
Hi André,
Thanks for the review feedback :)
On Thu, 6 Mar 2025 at 11:57, André Draszik <andre.draszik@linaro.org> wrote:
>
> On Sat, 2025-03-01 at 11:43 +0000, Peter Griffin wrote:
> > gs101 needs it's own suspend/resume callbacks to use the newly
> > added eint_fltcon_offset for saving & restoring fltcon0 & fltcon1
> > registers. It also differs to previous SoCs in that fltcon1
> > register doesn't always exist for each bank.
> >
> > exynosautov920 also has dedicated logic for using eint_con_offset
> > and eint_mask_offset for saving & restoring it's registers.
> >
> > Refactor the existing platform specific suspend/resume callback
> > so that each SoC variant has their own callback containing the
> > SoC specific logic.
> >
> > Additionally we now call drvdata->suspend() & drvdata->resume()
> > from within the loop that iterates the banks in
> > samsung_pinctrl_suspend() and samsung_pinctrl_resume().
> >
> > This simplifies the logic, and allows us to remove the
> > clk_enable() and clk_disable() from the callbacks.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> > Changes since v1:
> > * Split code refactor & gs101 parts into separate patches (Andre)
> > ---
> > drivers/pinctrl/samsung/pinctrl-exynos-arm64.c | 28 ++--
> > drivers/pinctrl/samsung/pinctrl-exynos.c | 201 ++++++++++---------------
> > drivers/pinctrl/samsung/pinctrl-exynos.h | 6 +-
> > drivers/pinctrl/samsung/pinctrl-samsung.c | 11 +-
> > drivers/pinctrl/samsung/pinctrl-samsung.h | 8 +-
> > 5 files changed, 111 insertions(+), 143 deletions(-)
> >
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > index e28fe8177646..57c98d2451b5 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > @@ -1112,8 +1112,8 @@ static const struct samsung_pin_ctrl exynosautov920_pin_ctrl[] = {
> > .pin_banks = exynosautov920_pin_banks0,
> > .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks0),
> > .eint_wkup_init = exynos_eint_wkup_init,
> > - .suspend = exynos_pinctrl_suspend,
> > - .resume = exynos_pinctrl_resume,
> > + .suspend = exynosautov920_pinctrl_suspend,
> > + .resume = exynosautov920_pinctrl_resume,
> > .retention_data = &exynosautov920_retention_data,
> > }, {
> > /* pin-controller instance 1 AUD data */
> > @@ -1124,43 +1124,43 @@ static const struct samsung_pin_ctrl exynosautov920_pin_ctrl[] = {
> > .pin_banks = exynosautov920_pin_banks2,
> > .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks2),
> > .eint_gpio_init = exynos_eint_gpio_init,
> > - .suspend = exynos_pinctrl_suspend,
> > - .resume = exynos_pinctrl_resume,
> > + .suspend = exynosautov920_pinctrl_suspend,
> > + .resume = exynosautov920_pinctrl_resume,
> > }, {
> > /* pin-controller instance 3 HSI1 data */
> > .pin_banks = exynosautov920_pin_banks3,
> > .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks3),
> > .eint_gpio_init = exynos_eint_gpio_init,
> > - .suspend = exynos_pinctrl_suspend,
> > - .resume = exynos_pinctrl_resume,
> > + .suspend = exynosautov920_pinctrl_suspend,
> > + .resume = exynosautov920_pinctrl_resume,
> > }, {
> > /* pin-controller instance 4 HSI2 data */
> > .pin_banks = exynosautov920_pin_banks4,
> > .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks4),
> > .eint_gpio_init = exynos_eint_gpio_init,
> > - .suspend = exynos_pinctrl_suspend,
> > - .resume = exynos_pinctrl_resume,
> > + .suspend = exynosautov920_pinctrl_suspend,
> > + .resume = exynosautov920_pinctrl_resume,
> > }, {
> > /* pin-controller instance 5 HSI2UFS data */
> > .pin_banks = exynosautov920_pin_banks5,
> > .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks5),
> > .eint_gpio_init = exynos_eint_gpio_init,
> > - .suspend = exynos_pinctrl_suspend,
> > - .resume = exynos_pinctrl_resume,
> > + .suspend = exynosautov920_pinctrl_suspend,
> > + .resume = exynosautov920_pinctrl_resume,
> > }, {
> > /* pin-controller instance 6 PERIC0 data */
> > .pin_banks = exynosautov920_pin_banks6,
> > .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks6),
> > .eint_gpio_init = exynos_eint_gpio_init,
> > - .suspend = exynos_pinctrl_suspend,
> > - .resume = exynos_pinctrl_resume,
> > + .suspend = exynosautov920_pinctrl_suspend,
> > + .resume = exynosautov920_pinctrl_resume,
> > }, {
> > /* pin-controller instance 7 PERIC1 data */
> > .pin_banks = exynosautov920_pin_banks7,
> > .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks7),
> > .eint_gpio_init = exynos_eint_gpio_init,
> > - .suspend = exynos_pinctrl_suspend,
> > - .resume = exynos_pinctrl_resume,
> > + .suspend = exynosautov920_pinctrl_suspend,
> > + .resume = exynosautov920_pinctrl_resume,
> > },
> > };
> >
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> > index ac6dc22b37c9..d65a9fba0781 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> > @@ -761,153 +761,118 @@ __init int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
> > return 0;
> > }
> >
> > -static void exynos_pinctrl_suspend_bank(
> > - struct samsung_pinctrl_drv_data *drvdata,
> > - struct samsung_pin_bank *bank)
> > +static void exynos_set_wakeup(struct samsung_pin_bank *bank)
> > {
> > - struct exynos_eint_gpio_save *save = bank->soc_priv;
> > - const void __iomem *regs = bank->eint_base;
> > + struct exynos_irq_chip *irq_chip = NULL;
>
> This is a useless init and the value is overwritten right away.
Will fix
>
> >
> > - if (clk_enable(bank->drvdata->pclk)) {
> > - dev_err(bank->gpio_chip.parent,
> > - "unable to enable clock for saving state\n");
> > - return;
> > + if (bank->eint_type == EINT_TYPE_WKUP) {
> > + if (bank->irq_chip) {
> > + irq_chip = bank->irq_chip;
> > + irq_chip->set_eint_wakeup_mask(bank->drvdata, irq_chip);
> > + }
> > }
> > -
> > - save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
> > - + bank->eint_offset);
> > - save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset);
> > - save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset + 4);
> > - save->eint_mask = readl(regs + bank->irq_chip->eint_mask
> > - + bank->eint_offset);
> > -
> > - clk_disable(bank->drvdata->pclk);
> > -
> > - pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
> > - pr_debug("%s: save fltcon0 %#010x\n", bank->name, save->eint_fltcon0);
> > - pr_debug("%s: save fltcon1 %#010x\n", bank->name, save->eint_fltcon1);
> > - pr_debug("%s: save mask %#010x\n", bank->name, save->eint_mask);
> > }
> >
> > -static void exynosauto_pinctrl_suspend_bank(struct samsung_pinctrl_drv_data *drvdata,
> > - struct samsung_pin_bank *bank)
> > +void exynos_pinctrl_suspend(struct samsung_pin_bank *bank)
> > {
> > struct exynos_eint_gpio_save *save = bank->soc_priv;
> > const void __iomem *regs = bank->eint_base;
> >
> > - if (clk_enable(bank->drvdata->pclk)) {
> > - dev_err(bank->gpio_chip.parent,
> > - "unable to enable clock for saving state\n");
> > - return;
> > + exynos_set_wakeup(bank);
> > +
> > + if (bank->eint_type == EINT_TYPE_GPIO) {
>
> Can you swap the order, and make the call to exynos_set_wakeup()
> an else, to avoid some needless tests and make it clearer that
> set_wakeup() is conditional as well, i.e.:
>
> if (bank->eint_type == EINT_TYPE_GPIO) {
> ...
> } else {
> exynos_set_wakeup(bank);
> }
>
> What makes it hard to read is that as-is, it looks like the
> set_wakeup() is unconditional, while it's also based on
> bank->eint_type (which is also more obvious before your patch).
I've addressed this and the rest of your review feedback in the v3
which I just sent.
Thanks,
Peter.
}
}
>
> Other than that, I think I looks good.
>
> Cheers,
> Andre'
>
> > + save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
> > + + bank->eint_offset);
> > + save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_offset);
> > + save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_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);
> > + pr_debug("%s: save fltcon1 %#010x\n",
> > + bank->name, save->eint_fltcon1);
> > + pr_debug("%s: save mask %#010x\n",
> > + bank->name, save->eint_mask);
> > }
> > -
> > - save->eint_con = readl(regs + bank->pctl_offset + bank->eint_con_offset);
> > - save->eint_mask = readl(regs + bank->pctl_offset + bank->eint_mask_offset);
> > -
> > - clk_disable(bank->drvdata->pclk);
> > -
> > - pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
> > - pr_debug("%s: save mask %#010x\n", bank->name, save->eint_mask);
> > }
> >
> > -void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
> > +void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank)
> > {
> > - struct samsung_pin_bank *bank = drvdata->pin_banks;
> > - struct exynos_irq_chip *irq_chip = NULL;
> > - int i;
> > + struct exynos_eint_gpio_save *save = bank->soc_priv;
> > + const void __iomem *regs = bank->eint_base;
> >
> > - for (i = 0; i < drvdata->nr_banks; ++i, ++bank) {
> > - if (bank->eint_type == EINT_TYPE_GPIO) {
> > - if (bank->eint_con_offset)
> > - exynosauto_pinctrl_suspend_bank(drvdata, bank);
> > - else
> > - exynos_pinctrl_suspend_bank(drvdata, bank);
> > - }
> > - else if (bank->eint_type == EINT_TYPE_WKUP) {
> > - if (!irq_chip) {
> > - irq_chip = bank->irq_chip;
> > - irq_chip->set_eint_wakeup_mask(drvdata,
> > - irq_chip);
> > - }
> > - }
> > + exynos_set_wakeup(bank);
> > +
> > + if (bank->eint_type == EINT_TYPE_GPIO) {
> > + save->eint_con = readl(regs + bank->pctl_offset +
> > + bank->eint_con_offset);
> > + save->eint_mask = readl(regs + bank->pctl_offset +
> > + bank->eint_mask_offset);
> > + pr_debug("%s: save con %#010x\n",
> > + bank->name, save->eint_con);
> > + pr_debug("%s: save mask %#010x\n",
> > + bank->name, save->eint_mask);
> > }
> > }
> >
> > -static void exynos_pinctrl_resume_bank(
> > - struct samsung_pinctrl_drv_data *drvdata,
> > - struct samsung_pin_bank *bank)
> > +void exynos_pinctrl_resume(struct samsung_pin_bank *bank)
> > {
> > struct exynos_eint_gpio_save *save = bank->soc_priv;
> > void __iomem *regs = bank->eint_base;
> >
> > - if (clk_enable(bank->drvdata->pclk)) {
> > - dev_err(bank->gpio_chip.parent,
> > - "unable to enable clock for restoring state\n");
> > - return;
> > + 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_offset), save->eint_fltcon0);
> > + pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
> > + readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_offset + 4),
> > + save->eint_fltcon1);
> > + 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_offset);
> > + writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_offset + 4);
> > + writel(save->eint_mask, regs + bank->irq_chip->eint_mask
> > + + bank->eint_offset);
> > }
> > -
> > - 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset), save->eint_fltcon0);
> > - pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
> > - readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset + 4), save->eint_fltcon1);
> > - 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset);
> > - writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset + 4);
> > - writel(save->eint_mask, regs + bank->irq_chip->eint_mask
> > - + bank->eint_offset);
> > -
> > - clk_disable(bank->drvdata->pclk);
> > }
> >
> > -static void exynosauto_pinctrl_resume_bank(struct samsung_pinctrl_drv_data *drvdata,
> > - struct samsung_pin_bank *bank)
> > +void exynosautov920_pinctrl_resume(struct samsung_pin_bank *bank)
> > {
> > struct exynos_eint_gpio_save *save = bank->soc_priv;
> > void __iomem *regs = bank->eint_base;
> >
> > - if (clk_enable(bank->drvdata->pclk)) {
> > - dev_err(bank->gpio_chip.parent,
> > - "unable to enable clock for restoring state\n");
> > - return;
> > + if (bank->eint_type == EINT_TYPE_GPIO) {
> > + /* exynosautov920 has eint_con_offset for all but one bank */
> > + if (!bank->eint_con_offset)
> > + exynos_pinctrl_resume(bank);
> > +
> > + pr_debug("%s: con %#010x => %#010x\n", bank->name,
> > + readl(regs + bank->pctl_offset + bank->eint_con_offset),
> > + save->eint_con);
> > + pr_debug("%s: mask %#010x => %#010x\n", bank->name,
> > + readl(regs + bank->pctl_offset +
> > + bank->eint_mask_offset), save->eint_mask);
> > +
> > + writel(save->eint_con,
> > + regs + bank->pctl_offset + bank->eint_con_offset);
> > + writel(save->eint_mask,
> > + regs + bank->pctl_offset + bank->eint_mask_offset);
> > }
> > -
> > - pr_debug("%s: con %#010x => %#010x\n", bank->name,
> > - readl(regs + bank->pctl_offset + bank->eint_con_offset), save->eint_con);
> > - pr_debug("%s: mask %#010x => %#010x\n", bank->name,
> > - readl(regs + bank->pctl_offset + bank->eint_mask_offset), save->eint_mask);
> > -
> > - writel(save->eint_con, regs + bank->pctl_offset + bank->eint_con_offset);
> > - writel(save->eint_mask, regs + bank->pctl_offset + bank->eint_mask_offset);
> > -
> > - clk_disable(bank->drvdata->pclk);
> > -}
> > -
> > -void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
> > -{
> > - struct samsung_pin_bank *bank = drvdata->pin_banks;
> > - int i;
> > -
> > - for (i = 0; i < drvdata->nr_banks; ++i, ++bank)
> > - if (bank->eint_type == EINT_TYPE_GPIO) {
> > - if (bank->eint_con_offset)
> > - exynosauto_pinctrl_resume_bank(drvdata, bank);
> > - else
> > - exynos_pinctrl_resume_bank(drvdata, bank);
> > - }
> > }
> >
> > static void exynos_retention_enable(struct samsung_pinctrl_drv_data *drvdata)
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
> > index 33df21d5c9d6..35c2bc4ea488 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos.h
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
> > @@ -221,8 +221,10 @@ struct exynos_muxed_weint_data {
> >
> > int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d);
> > int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d);
> > -void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata);
> > -void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata);
> > +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);
> > struct samsung_retention_ctrl *
> > exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata,
> > const struct samsung_retention_data *data);
> > diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> > index 963060920301..375634d8cc79 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> > @@ -1349,6 +1349,9 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
> > const u8 *widths = bank->type->fld_width;
> > enum pincfg_type type;
> >
> > + if (drvdata->suspend)
> > + drvdata->suspend(bank);
> > +
> > /* Registers without a powerdown config aren't lost */
> > if (!widths[PINCFG_TYPE_CON_PDN])
> > continue;
> > @@ -1373,8 +1376,6 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
> >
> > clk_disable(drvdata->pclk);
> >
> > - if (drvdata->suspend)
> > - drvdata->suspend(drvdata);
> > if (drvdata->retention_ctrl && drvdata->retention_ctrl->enable)
> > drvdata->retention_ctrl->enable(drvdata);
> >
> > @@ -1406,9 +1407,6 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
> > return ret;
> > }
> >
> > - if (drvdata->resume)
> > - drvdata->resume(drvdata);
> > -
> > for (i = 0; i < drvdata->nr_banks; i++) {
> > struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
> > void __iomem *reg = bank->pctl_base + bank->pctl_offset;
> > @@ -1416,6 +1414,9 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
> > const u8 *widths = bank->type->fld_width;
> > enum pincfg_type type;
> >
> > + if (drvdata->resume)
> > + drvdata->resume(bank);
> > +
> > /* Registers without a powerdown config aren't lost */
> > if (!widths[PINCFG_TYPE_CON_PDN])
> > continue;
> > diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> > index 371e4f02bbfb..e939e5bb0347 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> > @@ -285,8 +285,8 @@ struct samsung_pin_ctrl {
> > int (*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
> > int (*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
> > void (*pud_value_init)(struct samsung_pinctrl_drv_data *drvdata);
> > - void (*suspend)(struct samsung_pinctrl_drv_data *);
> > - void (*resume)(struct samsung_pinctrl_drv_data *);
> > + void (*suspend)(struct samsung_pin_bank *bank);
> > + void (*resume)(struct samsung_pin_bank *bank);
> > };
> >
> > /**
> > @@ -335,8 +335,8 @@ struct samsung_pinctrl_drv_data {
> >
> > struct samsung_retention_ctrl *retention_ctrl;
> >
> > - void (*suspend)(struct samsung_pinctrl_drv_data *);
> > - void (*resume)(struct samsung_pinctrl_drv_data *);
> > + void (*suspend)(struct samsung_pin_bank *bank);
> > + void (*resume)(struct samsung_pin_bank *bank);
> > };
> >
> > /**
> >
>
On Thu, 2025-03-06 at 11:57 +0000, André Draszik wrote:
> On Sat, 2025-03-01 at 11:43 +0000, Peter Griffin wrote:
> > [...]
> >
> > -static void exynos_pinctrl_resume_bank(
> > - struct samsung_pinctrl_drv_data *drvdata,
> > - struct samsung_pin_bank *bank)
> > +void exynos_pinctrl_resume(struct samsung_pin_bank *bank)
> > {
> > struct exynos_eint_gpio_save *save = bank->soc_priv;
> > void __iomem *regs = bank->eint_base;
> >
> > - if (clk_enable(bank->drvdata->pclk)) {
> > - dev_err(bank->gpio_chip.parent,
> > - "unable to enable clock for restoring state\n");
> > - return;
> > + 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_offset), save->eint_fltcon0);
> > + pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
> > + readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_offset + 4),
> > + save->eint_fltcon1);
The indent on this last line is a bit off.
> > + 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_offset);
> > + writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > + + 2 * bank->eint_offset + 4);
> > + writel(save->eint_mask, regs + bank->irq_chip->eint_mask
> > + + bank->eint_offset);
> > }
> > -
> > - 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset), save->eint_fltcon0);
> > - pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
> > - readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset + 4), save->eint_fltcon1);
> > - 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset);
> > - writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > - + 2 * bank->eint_offset + 4);
> > - writel(save->eint_mask, regs + bank->irq_chip->eint_mask
> > - + bank->eint_offset);
> > -
> > - clk_disable(bank->drvdata->pclk);
> > }
> >
> > -static void exynosauto_pinctrl_resume_bank(struct samsung_pinctrl_drv_data *drvdata,
> > - struct samsung_pin_bank *bank)
> > +void exynosautov920_pinctrl_resume(struct samsung_pin_bank *bank)
> > {
> > struct exynos_eint_gpio_save *save = bank->soc_priv;
> > void __iomem *regs = bank->eint_base;
> >
> > - if (clk_enable(bank->drvdata->pclk)) {
> > - dev_err(bank->gpio_chip.parent,
> > - "unable to enable clock for restoring state\n");
> > - return;
> > + if (bank->eint_type == EINT_TYPE_GPIO) {
> > + /* exynosautov920 has eint_con_offset for all but one bank */
> > + if (!bank->eint_con_offset)
> > + exynos_pinctrl_resume(bank);
> > +
> > + pr_debug("%s: con %#010x => %#010x\n", bank->name,
> > + readl(regs + bank->pctl_offset + bank->eint_con_offset),
> > + save->eint_con);
and here
Cheers,
Andre'
© 2016 - 2026 Red Hat, Inc.