Re: [PATCH 06/17] i3c: renesas: Reset the controller on resume

[Claudiu Beznea <claudiu.beznea@kernel.org>]

On 5/22/26 22:15, Frank Li wrote:
> On Fri, May 22, 2026 at 01:18:04PM +0300, Claudiu Beznea wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Reset the controller on resume after enabling the clocks to follow the
>> same sequence as in probe and avoid potential ordering related failures.
>>
>> Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
> 
> Can you move these similar stuff to one helper function to avoid duplicate
> efforts later?

If you are talking about moving also the control of the reset signals in the 
renesas_i3c_reset() I can do that, but, FMPOV, it will complicate the code, 
especially the initialization and failure paths (see the above diff built on top 
of this series).

Moving the reset de-assert in the renesas_i3c_reset() will involve calling 
functions to assert back the resets in case of failure. FMPOV, that is a bit 
unbalanced (wrt the way the code looks) because we are calling deassert in one 
function and assert in another function. It is a bit difficult to follow.

Please see the above diff and let me know your thoughts.


diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 3b9807a89b54..5f45a024aa54 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -255,8 +255,7 @@ struct renesas_i3c_xferqueue {
  struct renesas_i3c {
         void __iomem *regs;
         struct clk *tclk;
-       struct reset_control *presetn;
-       struct reset_control *tresetn;
+       struct reset_control_bulk_data *resets;
         struct device *dev;
         int *irqs;
         struct renesas_i3c_xferqueue xferqueue;
@@ -264,6 +263,7 @@ struct renesas_i3c {
         unsigned long rate;
         unsigned int num_irqs;
         enum i3c_internal_state internal_state;
+       u32 num_resets;
         u32 free_pos;
         u32 dyn_addr;
         u32 i2c_STDBR;
@@ -492,16 +492,28 @@ static int renesas_i3c_reset(struct renesas_i3c *i3c)
         u32 val;
         int ret;

+       ret = reset_control_bulk_deassert(i3c->num_resets, i3c->resets);
+       if (ret)
+               return ret;
+
         PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
         ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
         if (ret)
-               return ret;
+               goto assert;

         renesas_writel(i3c->regs, BCTL, 0);
         renesas_set_bit(i3c->regs, RSTCTL, RSTCTL_RI3CRST);

-       return read_poll_timeout(renesas_readl, val, !(val & RSTCTL_RI3CRST),
-                                0, 1000, false, i3c->regs, RSTCTL);
+       ret = read_poll_timeout(renesas_readl, val, !(val & RSTCTL_RI3CRST),
+                               0, 1000, false, i3c->regs, RSTCTL);
+       if (ret)
+               goto assert;
+
+       return 0;
+
+assert:
+       reset_control_bulk_assert(i3c->num_resets, i3c->resets);
+       return ret;
  }

  static void renesas_i3c_hw_init(struct renesas_i3c *i3c)
@@ -1430,11 +1442,20 @@ static const struct renesas_i3c_irq_desc 
renesas_i3c_irqs[] = {
         { .name = "nack", .isr = renesas_i3c_tend_isr, .desc = "i3c-nack" },
  };

+static const char * const renesas_i3c_resets[] = { "tresetn", "presetn" };
+
  static void renesas_i3c_dont_use_autosuspend(void *data)
  {
         pm_runtime_dont_use_autosuspend(data);
  }

+static void renesas_i3c_resets_assert(void *data)
+{
+       struct renesas_i3c *i3c = data;
+
+       reset_control_bulk_assert(i3c->num_resets, i3c->resets);
+}
+
  static int renesas_i3c_probe(struct platform_device *pdev)
  {
         struct renesas_i3c *i3c;
@@ -1464,15 +1485,20 @@ static int renesas_i3c_probe(struct platform_device *pdev)
         if (ret)
                 return ret;

-       i3c->tresetn = 
devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev, "tresetn");
-       if (IS_ERR(i3c->tresetn))
-               return dev_err_probe(&pdev->dev, PTR_ERR(i3c->tresetn),
-                                    "Error: missing tresetn ctrl\n");
+       i3c->num_resets = ARRAY_SIZE(renesas_i3c_resets);
+       i3c->resets = devm_kmalloc_array(&pdev->dev, i3c->num_resets,
+                                        sizeof(*i3c->resets), GFP_KERNEL);
+       if (!i3c->resets)
+               return -ENOMEM;

-       i3c->presetn = 
devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev, "presetn");
-       if (IS_ERR(i3c->presetn))
-               return dev_err_probe(&pdev->dev, PTR_ERR(i3c->presetn),
-                                    "Error: missing presetn ctrl\n");
+       for (unsigned int i = 0; i < i3c->num_resets; i++)
+               i3c->resets[i].id = renesas_i3c_resets[i];
+
+       ret = devm_reset_control_bulk_get_optional_exclusive(&pdev->dev,
+                                                            i3c->num_resets,
+                                                            i3c->resets);
+       if (ret)
+               return ret;

         spin_lock_init(&i3c->xferqueue.lock);
         INIT_LIST_HEAD(&i3c->xferqueue.list);
@@ -1481,6 +1507,11 @@ static int renesas_i3c_probe(struct platform_device *pdev)
         if (ret)
                 return ret;

+       /* Add devm action for resets deasserted in renesas_i3c_reset(). */
+       ret = devm_add_action_or_reset(&pdev->dev, renesas_i3c_resets_assert, NULL);
+       if (ret)
+               return ret;
+
         i3c->num_irqs = ARRAY_SIZE(renesas_i3c_irqs);
         i3c->irqs = devm_kcalloc(&pdev->dev, i3c->num_irqs, sizeof(*i3c->irqs), 
GFP_KERNEL);
         if (!i3c->irqs)
@@ -1523,15 +1554,11 @@ static void renesas_i3c_remove(struct platform_device *pdev)
  static int renesas_i3c_suspend(struct device *dev)
  {
         struct renesas_i3c *i3c = dev_get_drvdata(dev);
-       struct reset_control_bulk_data resets[] = {
-               { .rstc = i3c->presetn },
-               { .rstc = i3c->tresetn },
-       };
         int ret;

         i2c_mark_adapter_suspended(&i3c->base.i2c);

-       ret = reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
+       ret = reset_control_bulk_assert(i3c->num_resets, i3c->resets);
         if (ret)
                 goto err_mark_resumed;

@@ -1542,7 +1569,7 @@ static int renesas_i3c_suspend(struct device *dev)
         return 0;

  err_resets_deassert:
-       reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
+       reset_control_bulk_deassert(i3c->num_resets, i3c->resets);
  err_mark_resumed:
         i2c_mark_adapter_resumed(&i3c->base.i2c);

@@ -1552,23 +1579,15 @@ static int renesas_i3c_suspend(struct device *dev)
  static int renesas_i3c_resume(struct device *dev)
  {
         struct renesas_i3c *i3c = dev_get_drvdata(dev);
-       struct reset_control_bulk_data resets[] = {
-               { .rstc = i3c->presetn },
-               { .rstc = i3c->tresetn },
-       };
         int ret;

         ret = pm_runtime_force_resume(dev);
         if (ret)
                 return ret;

-       ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
-       if (ret)
-               return ret;
-
         ret = renesas_i3c_reset(i3c);
         if (ret)
-               goto err_resets_asserted;
+               return ret;

         PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
         ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
@@ -1607,7 +1626,7 @@ static int renesas_i3c_resume(struct device *dev)
          * if a runtime path is triggered after a failed resume). Checked on
          * RZ/G3S.
          */
-       reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
+       reset_control_bulk_assert(i3c->num_resets, i3c->resets);
         return ret;
  }

-- 
Thank you,
Claudiu