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