diff mbox series

[v3,1/8] i2c: riic: Use dev_err_probe in probe and riic_init_hw functions

Message ID 20241227115154.56154-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State New
Delegated to: Geert Uytterhoeven
Headers show
Series i2c: riic: driver cleanup and improvements | expand

Commit Message

Lad, Prabhakar Dec. 27, 2024, 11:51 a.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Refactor error handling in the riic_i2c_probe() and riic_init_hw()
functions by replacing multiple dev_err() calls with dev_err_probe().

Additionally, update the riic_init_hw() function to use a local `dev`
pointer instead of `riic->adapter.dev` for dev_err_probe(), as the I2C
adapter is not initialized at this stage.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- Squashed dev_err_probe() change from patch #2 into patch #1
- Updated commit message
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

Comments

Andy Shevchenko Dec. 28, 2024, 11:32 p.m. UTC | #1 
Fri, Dec 27, 2024 at 11:51:47AM +0000, Prabhakar kirjoitti:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Refactor error handling in the riic_i2c_probe() and riic_init_hw()
> functions by replacing multiple dev_err() calls with dev_err_probe().
> 
> Additionally, update the riic_init_hw() function to use a local `dev`
> pointer instead of `riic->adapter.dev` for dev_err_probe(), as the I2C
> adapter is not initialized at this stage.

...

> +	if (brl > (0x1F + 3))
> +		return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
> +				     (unsigned long)t->bus_freq_hz);

There is nothing special about bus_freq_hz. Why casting?

...

>  		ret = devm_request_irq(dev, ret, riic_irqs[i].isr,

I hate code doing

		ret = foo(ret);

>  				       0, riic_irqs[i].name, riic);

> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to request irq %s\n",
> +					     riic_irqs[i].name);

While this following the original code, with the above change (introducing a
separate variable for IRQ) this might also print it.
Lad, Prabhakar Dec. 31, 2024, 2:20 p.m. UTC | #2 
Hi Andy,

Thank you for the review.

On Sat, Dec 28, 2024 at 11:35 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Fri, Dec 27, 2024 at 11:51:47AM +0000, Prabhakar kirjoitti:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Refactor error handling in the riic_i2c_probe() and riic_init_hw()
> > functions by replacing multiple dev_err() calls with dev_err_probe().
> >
> > Additionally, update the riic_init_hw() function to use a local `dev`
> > pointer instead of `riic->adapter.dev` for dev_err_probe(), as the I2C
> > adapter is not initialized at this stage.
>
> ...
>
> > +     if (brl > (0x1F + 3))
> > +             return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
> > +                                  (unsigned long)t->bus_freq_hz);
>
> There is nothing special about bus_freq_hz. Why casting?
>
Ok, I'll update it like below:

    if (brl > (0x1F + 3))
        return dev_err_probe(dev, -EINVAL, "invalid speed (%uHz). Too slow.\n",
                     t->bus_freq_hz);

> ...
>
> >               ret = devm_request_irq(dev, ret, riic_irqs[i].isr,
>
> I hate code doing
>
>                 ret = foo(ret);
>
> >                                      0, riic_irqs[i].name, riic);
>
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret, "failed to request irq %s\n",
> > +                                          riic_irqs[i].name);
>
> While this following the original code, with the above change (introducing a
> separate variable for IRQ) this might also print it.
>
Ok, I'll create a new patch for this and have something like below:


    for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
        int irq = platform_get_irq(pdev, riic_irqs[i].res_num);

        ret = devm_request_irq(dev, irq, riic_irqs[i].isr,
                       0, riic_irqs[i].name, riic);
        if (ret)
            return ret;
    }

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 9264adc97ca9..e1babd5077d4 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -356,11 +356,9 @@  static int riic_init_hw(struct riic_dev *riic)
 		rate /= 2;
 	}
 
-	if (brl > (0x1F + 3)) {
-		dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
-			(unsigned long)t->bus_freq_hz);
-		return -EINVAL;
-	}
+	if (brl > (0x1F + 3))
+		return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
+				     (unsigned long)t->bus_freq_hz);
 
 	brh = total_ticks - brl;
 
@@ -445,10 +443,9 @@  static int riic_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(riic->base);
 
 	riic->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(riic->clk)) {
-		dev_err(dev, "missing controller clock");
-		return PTR_ERR(riic->clk);
-	}
+	if (IS_ERR(riic->clk))
+		return dev_err_probe(dev, PTR_ERR(riic->clk),
+				     "missing controller clock");
 
 	riic->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
 	if (IS_ERR(riic->rstc))
@@ -470,10 +467,9 @@  static int riic_i2c_probe(struct platform_device *pdev)
 
 		ret = devm_request_irq(dev, ret, riic_irqs[i].isr,
 				       0, riic_irqs[i].name, riic);
-		if (ret) {
-			dev_err(dev, "failed to request irq %s\n", riic_irqs[i].name);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request irq %s\n",
+					     riic_irqs[i].name);
 	}
 
 	riic->info = of_device_get_match_data(dev);