Hi Andy,
Thank you for the review.
On Sat, Dec 28, 2024 at 11:35 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> Fri, Dec 27, 2024 at 11:51:47AM +0000, Prabhakar kirjoitti:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > 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
