| Message ID | 20210329015206.17437-5-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Accepted |
| Commit | bc72675228c781996d7f62849e84dd23145479d5 |
| Headers | show |
| Series | i2c: mpc: Refactor to improve responsiveness | expand |
On 11/04/21 8:16 am, Wolfram Sang wrote: > On Mon, Mar 29, 2021 at 02:52:04PM +1300, Chris Packham wrote: >> All the in-tree dts files that use one of the compatible strings from >> i2c-mpc.c provide an interrupt property. By making this mandatory we >> can simplify the code. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > After I applied this patch, cppcheck reports: > > CPPCHECK > drivers/i2c/busses/i2c-mpc.c:401:47: warning: Either the condition 'div?(int)div->fdr:-EINVAL' is redundant or there is possible null pointer dereference: div. [nullPointerRedundantCheck] > *real_clk = fsl_get_sys_freq() / prescaler / div->divider; > ^ > drivers/i2c/busses/i2c-mpc.c:402:13: note: Assuming that condition 'div?(int)div->fdr:-EINVAL' is not redundant > return div ? (int)div->fdr : -EINVAL; > ^ > drivers/i2c/busses/i2c-mpc.c:401:47: note: Null pointer dereference > *real_clk = fsl_get_sys_freq() / prescaler / div->divider; > ^ > Can you check this? I'd think we can fix it incrementally... > What are the arguments passed to cppcheck? I've tried two versions I have easy access to (1.82 and 1.86) neither report problems when invoked as `cppcheck drivers/i2c/busses/i2c-mpc.c` nor do they complain about this with `--enable=all`. Looking at the code I can see what it's complaining about, div should have a value since mpc_i2c_dividers_8xxx does not have a sentinel value so I think the div check is unnecessary.
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index 6a0d55e9e8e3..5b746a898e8e 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -123,37 +123,21 @@ static void mpc_i2c_fixup(struct mpc_i2c *i2c) static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing) { - unsigned long orig_jiffies = jiffies; u32 cmd_err; - int result = 0; + int result; - if (!i2c->irq) { - while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) { - schedule(); - if (time_after(jiffies, orig_jiffies + timeout)) { - dev_dbg(i2c->dev, "timeout\n"); - writeccr(i2c, 0); - result = -ETIMEDOUT; - break; - } - } - cmd_err = readb(i2c->base + MPC_I2C_SR); - writeb(0, i2c->base + MPC_I2C_SR); - } else { - /* Interrupt mode */ - result = wait_event_timeout(i2c->queue, + result = wait_event_timeout(i2c->queue, (i2c->interrupt & CSR_MIF), timeout); - if (unlikely(!(i2c->interrupt & CSR_MIF))) { - dev_dbg(i2c->dev, "wait timeout\n"); - writeccr(i2c, 0); - result = -ETIMEDOUT; - } - - cmd_err = i2c->interrupt; - i2c->interrupt = 0; + if (unlikely(!(i2c->interrupt & CSR_MIF))) { + dev_dbg(i2c->dev, "wait timeout\n"); + writeccr(i2c, 0); + result = -ETIMEDOUT; } + cmd_err = i2c->interrupt; + i2c->interrupt = 0; + if (result < 0) return result; @@ -694,13 +678,16 @@ static int fsl_i2c_probe(struct platform_device *op) } i2c->irq = irq_of_parse_and_map(op->dev.of_node, 0); - if (i2c->irq) { /* no i2c->irq implies polling */ - result = request_irq(i2c->irq, mpc_i2c_isr, - IRQF_SHARED, "i2c-mpc", i2c); - if (result < 0) { - dev_err(i2c->dev, "failed to attach interrupt\n"); - goto fail_request; - } + if (i2c->irq < 0) { + result = i2c->irq; + goto fail_map; + } + + result = request_irq(i2c->irq, mpc_i2c_isr, + IRQF_SHARED, "i2c-mpc", i2c); + if (result < 0) { + dev_err(i2c->dev, "failed to attach interrupt\n"); + goto fail_request; } /*
All the in-tree dts files that use one of the compatible strings from i2c-mpc.c provide an interrupt property. By making this mandatory we can simplify the code. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- drivers/i2c/busses/i2c-mpc.c | 51 ++++++++++++++---------------------- 1 file changed, 19 insertions(+), 32 deletions(-)