[linux,dev-4.7] i2c: aspeed: drop the locks

Submitted by Joel Stanley on Feb. 24, 2017, 5:15 a.m.

Details

Message ID 20170224051556.16432-1-joel@jms.id.au
State Needs Review / ACK
Headers show

Commit Message

Joel Stanley Feb. 24, 2017, 5:15 a.m.
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(-)

Comments

Joel Stanley Feb. 24, 2017, 5:17 a.m.
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
>

Patch hide | download patch | download mbox

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;