Hi Krzysztof,

Thanks for the review feedback.

On Tue, 18 Mar 2025 at 19:47, Krzysztof Kozlowski [off-list ref] wrote:

On 12/03/2025 22:58, Peter Griffin wrote:

quoted

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.

Yes drvdata->suspend() gets called slightly earlier after this patch.
I can mention that in the commit message

quoted

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.

The clk_enable() / clk_disable() has been removed from both the
drvdata->suspend() and drvdata->resume() callbacks

quoted

from each callback, and also avoids iterating the same loop again
in the next function.

...

quoted

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

+
              /* 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])

Yes exactly it was the above line, and trying to avoid iterating the
loop a second time.

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?

As you say it's just saving/restoring some registers so I don't
believe the ordering matters. But if you would like it to be kept
symmetrically reversed I could switch back to calling it in almost the
same place as before this patch (just moving it a couple lines up
before the clk_disable() and iterate the loop again.

for (i = 0; i < drvdata->nr_banks; i++)
    drvdata->suspend(bank);

and similar for drvdata->resume(). Then the ordering should be exactly
the same as prior to this patch.

Thanks,

Peter