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

[Claudiu Beznea]

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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > ---
>
> 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