Re: [PATCH v5 2/5] pinctrl: samsung: refactor drvdata suspend & resume callbacks
From: Krzysztof Kozlowski <krzk@kernel.org>
Date: 2025-03-18 19:47:55
Also in:
linux-gpio, linux-samsung-soc, lkml
On 12/03/2025 22:58, Peter Griffin wrote:
Move the call of drvdata->suspend()/resume into the loop which is iterating drvdata for each bank.
Side effect is that now each drvdata->suspend will be called before saving registers. Please mention it here and this lead me to one more comment.
This allows the clk_enable() and clk_disable() logic to be removed
For suspend path - yes. For resume path - nothing changed, because drvdata->resume(drvdata) was called with clock enabled.
from each callback, and also avoids iterating the same loop again in the next function.
...
quoted hunk ↗ jump to hunk
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c index 963060920301ec90affb2ee6d758d3d602ffb4a9..375634d8cc79d6533603e3eed562452181e2ee25 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);
Here suspend() is called before saving common register state (was *after*)...
quoted hunk ↗ jump to hunk
+ /* 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);
But this is not symmetrically reversed now - resume() is before restoring from saved state. Maybe this change is intentional, but then it should be expressed in commit msg and in commit why this was chosen. I guess you decided to do that way only because of code: if (!widths[PINCFG_TYPE_CON_PDN]) This code should be symmetrically reversed, otherwise it just raises questions. For saving register state, it does not really matter, but in general if we assume driver-specific suspend callback is run the last, then driver-specific resume callback should be first, no? Best regards, Krzysztof