| Message ID | 20241227115154.56154-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | i2c: riic: driver cleanup and improvements | expand |
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.
Fri, Dec 27, 2024 at 11:51:49AM +0000, Prabhakar kirjoitti: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Easier to read and ensures proper types. ... > #include <linux/pm_runtime.h> > #include <linux/reset.h> Does it include bits.h or equivalent (bitops.h, bitmap.h)?
Fri, Dec 27, 2024 at 11:51:53AM +0000, Prabhakar kirjoitti: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Replace the hardcoded `1000000000` with the predefined `NANO` macro for > clarity. Simplify the code by introducing a `ns_per_tick` variable to > store `NANO / rate`, reducing redundancy and improving readability. ... > - brl -= t->scl_fall_ns / (1000000000 / rate); > - brh -= t->scl_rise_ns / (1000000000 / rate); > + ns_per_tick = NANO / rate; So, why NANO and not NSEC_PER_SEC? > + brl -= t->scl_fall_ns / ns_per_tick; > + brh -= t->scl_rise_ns / ns_per_tick;
Fri, Dec 27, 2024 at 11:10:22PM +0100, Andi Shyti kirjoitti: ... > > i2c: riic: Use dev_err_probe in probe and riic_init_hw functions > > i2c: riic: Use local `dev` pointer in `dev_err_probe()` > > i2c: riic: Use BIT macro consistently > > i2c: riic: Use GENMASK() macro for bitmask definitions > > i2c: riic: Make use of devres helper to request deasserted reset line > > i2c: riic: Mark riic_irqs array as const > > i2c: riic: Use predefined macro and simplify clock tick calculation > > i2c: riic: Add `riic_bus_barrier()` to check bus availability > > merged to i2c/i2c-host. There are some comments, up to you how to proceed, they seem not to be any critical.
Hi Andy, On Sun, Dec 29, 2024 at 01:40:54AM +0200, Andy Shevchenko wrote: > Fri, Dec 27, 2024 at 11:10:22PM +0100, Andi Shyti kirjoitti: > > ... > > > > i2c: riic: Use dev_err_probe in probe and riic_init_hw functions > > > i2c: riic: Use local `dev` pointer in `dev_err_probe()` > > > i2c: riic: Use BIT macro consistently > > > i2c: riic: Use GENMASK() macro for bitmask definitions > > > i2c: riic: Make use of devres helper to request deasserted reset line > > > i2c: riic: Mark riic_irqs array as const > > > i2c: riic: Use predefined macro and simplify clock tick calculation > > > i2c: riic: Add `riic_bus_barrier()` to check bus availability > > > > merged to i2c/i2c-host. > > There are some comments, up to you how to proceed, they seem not to be any > critical. first of all, welcome back :-) I'd like the comments to be addressed, even if they are not critical. So that I'm going to remove this series for now until there are no more questions. Thanks for looking into this series, Andi
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
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Simplify and modernize the RIIC I2C driver with the following changes: 1. Refactor error handling in `riic_i2c_probe()` and `riic_init_hw()` by replacing `dev_err()` with `dev_err_probe()` and using a local `dev` pointer. 2. Use `BIT()` and `GENMASK()` macros for consistent and clear bit handling. 3. Manage reset lines with `devm_reset_control_get_exclusive()` to simplify resource handling. 4. Mark `riic_irqs` as `const` and simplify clock tick calculations with predefined macros. 5. Add `riic_bus_barrier()` to check bus availability and improve reliability. Lad Prabhakar (8): i2c: riic: Use dev_err_probe in probe and riic_init_hw functions i2c: riic: Use local `dev` pointer in `dev_err_probe()` i2c: riic: Use BIT macro consistently i2c: riic: Use GENMASK() macro for bitmask definitions i2c: riic: Make use of devres helper to request deasserted reset line i2c: riic: Mark riic_irqs array as const i2c: riic: Use predefined macro and simplify clock tick calculation i2c: riic: Add `riic_bus_barrier()` to check bus availability drivers/i2c/busses/i2c-riic.c | 123 ++++++++++++++++++---------------- 1 file changed, 65 insertions(+), 58 deletions(-)