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

Hi Krzysztof,

Thanks for the review feedback.

On Tue, 11 Mar 2025 at 19:36, Krzysztof Kozlowski krzk@kernel.org wrote:

On 07/03/2025 11:29, 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.

Fixes: 4a8be01a1a7a ("pinctrl: samsung: Add gs101 SoC pinctrl configuration") Cc: stable@vger.kernel.org

It looks this depends on previous commit, right?

Yes that's right, it depends on the refactoring in the previous patch. To fix the bug (which is an Serror on suspend for gs101), we need the dedicated gs101 callback so it can have the knowledge that fltcon1 doesn't always exist and it's varying offset.

That's really not optimal, although I understand that if you re-order patches this code would be soon changed, just like you changed other suspend/resume callbacks in patch #2?

Originally it was just one patch, but the previous review feedback was to split the refactor into a dedicated patch, and then add gs101 specific parts separately. The refactoring was done primarily so we can fix this bug without affecting the other platforms, so I don't re-ordering the patches will help.

Thanks,

Peter

...

Reviewed-by: André Draszik andre.draszik@linaro.org Signed-off-by: Peter Griffin peter.griffin@linaro.org

---

Changes since v2:

• make it clear exynos_set_wakeup(bank) is conditional on bank type (Andre)
• align style where the '+' is placed (Andre)
• remove unnecessary braces (Andre)

---

...

...

+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;
  

• 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);
  

•         /* 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);
  

• } else if (bank->eint_type == EINT_TYPE_WKUP)
  

•         exynos_set_wakeup(bank);
  

Missing {}. Run checkpatch --strict.

Best regards, Krzysztof