Message ID | 20170224051556.16432-1-joel@jms.id.au |
---|---|
State | Rejected, archived |
Headers | show |
On Fri, Feb 24, 2017 at 3:45 PM, Joel Stanley <joel@jms.id.au> wrote: > The i2c core code provides i2c_lock_bus. This gives all i2c buses a > per-adapter lock, so the locking we perform in the driver is redundant. > > Tested on Zaius by using the UDC and EEPROM which are on the same i2c bus. > > Suggested-by: Andrew Jeffery <andrew@aj.id.au> > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > drivers/i2c/busses/i2c-aspeed.c | 27 ++------------------------- > 1 file changed, 2 insertions(+), 25 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index ece5aa3ecfd1..61da89407895 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -252,9 +252,6 @@ struct ast_i2c_bus { > struct clk *pclk; > int irq; > > - /* i2c transfer state. this is accessed from both process and IRQ > - * context, so is protected by cmd_lock */ > - spinlock_t cmd_lock; > bool send_start; > bool send_stop; /* last message of an xfer? */ > bool query_len; > @@ -358,10 +355,8 @@ static void ast_i2c_issue_cmd(struct ast_i2c_bus *bus, u32 cmd) > > static int ast_i2c_issue_oob_command(struct ast_i2c_bus *bus, u32 cmd) > { > - spin_lock_irq(&bus->cmd_lock); > init_completion(&bus->cmd_complete); > ast_i2c_issue_cmd(bus, cmd); > - spin_unlock_irq(&bus->cmd_lock); > return wait_for_completion_interruptible_timeout(&bus->cmd_complete, > bus->adap.timeout*HZ); > } > @@ -539,8 +534,9 @@ static void ast_i2c_master_xfer_done(struct ast_i2c_bus *bus) > /* queue the next message. If there's none left, we notify the > * waiter */ > next_msg_queued = ast_i2c_do_byte_xfer(bus); > - if (!next_msg_queued) > + if (!next_msg_queued) { > complete(&bus->cmd_complete); > + } This is obviously unrelated and unnecessary. > } > > #if IS_ENABLED(CONFIG_I2C_SLAVE) > @@ -554,7 +550,6 @@ static bool ast_i2c_slave_irq(struct ast_i2c_bus *bus) > enum i2c_slave_event event; > struct i2c_client *slave = bus->slave; > > - spin_lock(&bus->cmd_lock); > if (!slave) { > irq_handled = false; > goto out; > @@ -635,7 +630,6 @@ static bool ast_i2c_slave_irq(struct ast_i2c_bus *bus) > ast_i2c_write(bus, status_ack, I2C_INTR_STS_REG); > > out: > - spin_unlock(&bus->cmd_lock); > return irq_handled; > } > #endif > @@ -649,7 +643,6 @@ static bool ast_i2c_master_irq(struct ast_i2c_bus *bus) > AST_I2CD_INTR_TX_NAK; > u32 sts, cmd; > > - spin_lock(&bus->cmd_lock); > > cmd = ast_i2c_read(bus, I2C_CMD_REG); > sts = ast_i2c_read(bus, I2C_INTR_STS_REG); > @@ -708,8 +701,6 @@ static bool ast_i2c_master_irq(struct ast_i2c_bus *bus) > bus->msg, bus->cmd_pending); > } > > - spin_unlock(&bus->cmd_lock); > - > return true; > } > > @@ -734,13 +725,11 @@ static irqreturn_t ast_i2c_bus_irq(int irq, void *dev_id) > static int ast_i2c_do_msgs_xfer(struct ast_i2c_bus *bus, > struct i2c_msg *msgs, int num) > { > - unsigned long flags; > int i, ret = 0; > u32 err, cmd; > > for (i = 0; i < num; i++) { > > - spin_lock_irqsave(&bus->cmd_lock, flags); > bus->msg = &msgs[i]; > bus->msg_pos = 0; > bus->query_len = bus->msg->flags & I2C_M_RECV_LEN; > @@ -749,18 +738,15 @@ static int ast_i2c_do_msgs_xfer(struct ast_i2c_bus *bus, > init_completion(&bus->cmd_complete); > > ast_i2c_do_byte_xfer(bus); > - spin_unlock_irqrestore(&bus->cmd_lock, flags); > > ret = wait_for_completion_interruptible_timeout( > &bus->cmd_complete, > bus->adap.timeout * HZ); > > - spin_lock_irqsave(&bus->cmd_lock, flags); > err = bus->cmd_err; > cmd = bus->cmd_sent; > bus->cmd_sent = 0; > bus->msg = NULL; > - spin_unlock_irqrestore(&bus->cmd_lock, flags); > > if (!ret) { > dev_dbg(bus->dev, "controller timed out\n"); > @@ -831,14 +817,11 @@ static u32 ast_i2c_functionality(struct i2c_adapter *adap) > static int ast_i2c_reg_slave(struct i2c_client *client) > { > struct ast_i2c_bus *bus; > - unsigned long flags; > u32 addr_reg_val; > u32 func_ctrl_reg_val; > > bus = client->adapter->algo_data; > - spin_lock_irqsave(&bus->cmd_lock, flags); > if (bus->slave) { > - spin_unlock_irqrestore(&bus->cmd_lock, flags); > return -EINVAL; > } > > @@ -856,20 +839,16 @@ static int ast_i2c_reg_slave(struct i2c_client *client) > > bus->slave = client; > bus->slave_state = AST_I2C_SLAVE_STOP; > - spin_unlock_irqrestore(&bus->cmd_lock, flags); > return 0; > } > > static int ast_i2c_unreg_slave(struct i2c_client *client) > { > struct ast_i2c_bus *bus = client->adapter->algo_data; > - unsigned long flags; > u32 addr_reg_val; > u32 func_ctrl_reg_val; > > - spin_lock_irqsave(&bus->cmd_lock, flags); > if (!bus->slave) { > - spin_unlock_irqrestore(&bus->cmd_lock, flags); > return -EINVAL; > } > > @@ -880,7 +859,6 @@ static int ast_i2c_unreg_slave(struct i2c_client *client) > ast_i2c_write(bus, func_ctrl_reg_val, I2C_FUN_CTRL_REG); > > bus->slave = NULL; > - spin_unlock_irqrestore(&bus->cmd_lock, flags); > return 0; > } > #endif > @@ -933,7 +911,6 @@ static int ast_i2c_probe_bus(struct platform_device *pdev) > } > > /* Initialize the I2C adapter */ > - spin_lock_init(&bus->cmd_lock); > bus->adap.nr = bus_num; > bus->adap.owner = THIS_MODULE; > bus->adap.retries = 0; > -- > 2.11.0 >
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index ece5aa3ecfd1..61da89407895 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -252,9 +252,6 @@ struct ast_i2c_bus { struct clk *pclk; int irq; - /* i2c transfer state. this is accessed from both process and IRQ - * context, so is protected by cmd_lock */ - spinlock_t cmd_lock; bool send_start; bool send_stop; /* last message of an xfer? */ bool query_len; @@ -358,10 +355,8 @@ static void ast_i2c_issue_cmd(struct ast_i2c_bus *bus, u32 cmd) static int ast_i2c_issue_oob_command(struct ast_i2c_bus *bus, u32 cmd) { - spin_lock_irq(&bus->cmd_lock); init_completion(&bus->cmd_complete); ast_i2c_issue_cmd(bus, cmd); - spin_unlock_irq(&bus->cmd_lock); return wait_for_completion_interruptible_timeout(&bus->cmd_complete, bus->adap.timeout*HZ); } @@ -539,8 +534,9 @@ static void ast_i2c_master_xfer_done(struct ast_i2c_bus *bus) /* queue the next message. If there's none left, we notify the * waiter */ next_msg_queued = ast_i2c_do_byte_xfer(bus); - if (!next_msg_queued) + if (!next_msg_queued) { complete(&bus->cmd_complete); + } } #if IS_ENABLED(CONFIG_I2C_SLAVE) @@ -554,7 +550,6 @@ static bool ast_i2c_slave_irq(struct ast_i2c_bus *bus) enum i2c_slave_event event; struct i2c_client *slave = bus->slave; - spin_lock(&bus->cmd_lock); if (!slave) { irq_handled = false; goto out; @@ -635,7 +630,6 @@ static bool ast_i2c_slave_irq(struct ast_i2c_bus *bus) ast_i2c_write(bus, status_ack, I2C_INTR_STS_REG); out: - spin_unlock(&bus->cmd_lock); return irq_handled; } #endif @@ -649,7 +643,6 @@ static bool ast_i2c_master_irq(struct ast_i2c_bus *bus) AST_I2CD_INTR_TX_NAK; u32 sts, cmd; - spin_lock(&bus->cmd_lock); cmd = ast_i2c_read(bus, I2C_CMD_REG); sts = ast_i2c_read(bus, I2C_INTR_STS_REG); @@ -708,8 +701,6 @@ static bool ast_i2c_master_irq(struct ast_i2c_bus *bus) bus->msg, bus->cmd_pending); } - spin_unlock(&bus->cmd_lock); - return true; } @@ -734,13 +725,11 @@ static irqreturn_t ast_i2c_bus_irq(int irq, void *dev_id) static int ast_i2c_do_msgs_xfer(struct ast_i2c_bus *bus, struct i2c_msg *msgs, int num) { - unsigned long flags; int i, ret = 0; u32 err, cmd; for (i = 0; i < num; i++) { - spin_lock_irqsave(&bus->cmd_lock, flags); bus->msg = &msgs[i]; bus->msg_pos = 0; bus->query_len = bus->msg->flags & I2C_M_RECV_LEN; @@ -749,18 +738,15 @@ static int ast_i2c_do_msgs_xfer(struct ast_i2c_bus *bus, init_completion(&bus->cmd_complete); ast_i2c_do_byte_xfer(bus); - spin_unlock_irqrestore(&bus->cmd_lock, flags); ret = wait_for_completion_interruptible_timeout( &bus->cmd_complete, bus->adap.timeout * HZ); - spin_lock_irqsave(&bus->cmd_lock, flags); err = bus->cmd_err; cmd = bus->cmd_sent; bus->cmd_sent = 0; bus->msg = NULL; - spin_unlock_irqrestore(&bus->cmd_lock, flags); if (!ret) { dev_dbg(bus->dev, "controller timed out\n"); @@ -831,14 +817,11 @@ static u32 ast_i2c_functionality(struct i2c_adapter *adap) static int ast_i2c_reg_slave(struct i2c_client *client) { struct ast_i2c_bus *bus; - unsigned long flags; u32 addr_reg_val; u32 func_ctrl_reg_val; bus = client->adapter->algo_data; - spin_lock_irqsave(&bus->cmd_lock, flags); if (bus->slave) { - spin_unlock_irqrestore(&bus->cmd_lock, flags); return -EINVAL; } @@ -856,20 +839,16 @@ static int ast_i2c_reg_slave(struct i2c_client *client) bus->slave = client; bus->slave_state = AST_I2C_SLAVE_STOP; - spin_unlock_irqrestore(&bus->cmd_lock, flags); return 0; } static int ast_i2c_unreg_slave(struct i2c_client *client) { struct ast_i2c_bus *bus = client->adapter->algo_data; - unsigned long flags; u32 addr_reg_val; u32 func_ctrl_reg_val; - spin_lock_irqsave(&bus->cmd_lock, flags); if (!bus->slave) { - spin_unlock_irqrestore(&bus->cmd_lock, flags); return -EINVAL; } @@ -880,7 +859,6 @@ static int ast_i2c_unreg_slave(struct i2c_client *client) ast_i2c_write(bus, func_ctrl_reg_val, I2C_FUN_CTRL_REG); bus->slave = NULL; - spin_unlock_irqrestore(&bus->cmd_lock, flags); return 0; } #endif @@ -933,7 +911,6 @@ static int ast_i2c_probe_bus(struct platform_device *pdev) } /* Initialize the I2C adapter */ - spin_lock_init(&bus->cmd_lock); bus->adap.nr = bus_num; bus->adap.owner = THIS_MODULE; bus->adap.retries = 0;
The i2c core code provides i2c_lock_bus. This gives all i2c buses a per-adapter lock, so the locking we perform in the driver is redundant. Tested on Zaius by using the UDC and EEPROM which are on the same i2c bus. Suggested-by: Andrew Jeffery <andrew@aj.id.au> Signed-off-by: Joel Stanley <joel@jms.id.au> --- drivers/i2c/busses/i2c-aspeed.c | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-)