Skip to contentSkip to conversation

Blackfin I2C/TWI driver: add missing pin mux operation

PATCH In review Bryan Wu 2 participants 3 messages Mar 12, 2008 – Mar 25, 2008 2 messages not in the archive I2C subsystem host drivers

lore
Share & tools
lore

Bryan Wu

author
#0 lore raw

PATCH 3/6 Blackfin I2C/TWI driver: add missing pin mux operation

From: Bryan Wu <[email protected]>

Blackfin TWI controller hardware pin should be requested from GPIO port controller Before BF54x, there is no need to do this. But as long as BF54x and BF52x are supported by this generic driver, the missing pin mux operation should be added.

Signed-off-by Bryan Wu <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
---
 drivers/i2c/busses/i2c-bfin-twi.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
drivers/i2c/busses/i2c-bfin-twi.c +29 -0
index 515fe08..674c053 100644
@@ -34,6 +34,7 @@
 #include <linux/platform_device.h>
 
 #include <asm/blackfin.h>
+#include <asm/portmux.h>
 #include <asm/irq.h>
 
 #define POLL_TIMEOUT       (2 * HZ)

@@ -63,6 +64,7 @@ struct bfin_twi_iface {
 	int			msg_num;
 	int			cur_msg;
 	void __iomem		*regs_base;
+	int			bus_num;
 };
 
 

@@ -89,6 +91,23 @@ DEFINE_TWI_REG(XMT_DATA16, 0x84)
 DEFINE_TWI_REG(RCV_DATA8, 0x88)
 DEFINE_TWI_REG(RCV_DATA16, 0x8C)
 
+static u16 pin_req[2][3] = {
+	{P_TWI0_SCL, P_TWI0_SDA, 0},
+	{P_TWI1_SCL, P_TWI1_SDA, 0},
+};
+
+static int setup_pin_mux(struct bfin_twi_iface *iface, int action)
+{
+	int rc = 0;
+
+	if (action)
+		rc = peripheral_request_list(pin_req[iface->bus_num], "i2c-bfin-twi");
+	else
+		peripheral_free_list(pin_req[iface->bus_num]);
+
+	return rc;
+}
+
 static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 {
 	unsigned short twi_int_status = read_INT_STAT(iface);

@@ -640,6 +659,7 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
 		goto out_error_no_irq;
 	}
 
+	iface->bus_num = pdev->id;
 	init_timer(&(iface->timeout_timer));
 	iface->timeout_timer.function = bfin_twi_timeout;
 	iface->timeout_timer.data = (unsigned long)iface;

@@ -653,6 +673,12 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
 	p_adap->class = I2C_CLASS_ALL;
 	p_adap->dev.parent = &pdev->dev;
 
+	rc = setup_pin_mux(iface, 1);
+	if (rc) {
+		dev_err(&pdev->dev, "Can't setup Pin Mux!\n");
+		goto out_error_pin_mux;
+	}
+
 	rc = request_irq(iface->irq, bfin_twi_interrupt_entry,
 		IRQF_DISABLED, pdev->name, iface);
 	if (rc) {

@@ -690,6 +716,8 @@ out_error_add_adapter:
 	free_irq(iface->irq, iface);
 out_error_req_irq:
 out_error_no_irq:
+	setup_pin_mux(iface, 0);
+out_error_pin_mux:
 	iounmap(iface->regs_base);
 out_error_ioremap:
 out_error_get_res:

@@ -706,6 +734,7 @@ static int i2c_bfin_twi_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&(iface->adap));
 	free_irq(iface->irq, iface);
+	setup_pin_mux(iface, 0);
 
 	return 0;
 }
Signature · 7 lines
1.5.4.3


_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): 1205479360-25240-4-git-send-email-cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#2 lore raw

Hi Bryan,

Quoted 34 lines · Bryan Wu

From: Bryan Wu <[email protected]>

Blackfin TWI controller hardware pin should be requested from GPIO port controller Before BF54x, there is no need to do this. But as long as BF54x and BF52x are supported by this generic driver, the missing pin mux operation should be added.

Signed-off-by Bryan Wu <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
---
 drivers/i2c/busses/i2c-bfin-twi.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
drivers/i2c/busses/i2c-bfin-twi.c +2 -0
index 515fe08..674c053 100644
@@ -34,6 +34,7 @@
 #include <linux/platform_device.h>
 
 #include <asm/blackfin.h>
+#include <asm/portmux.h>
 #include <asm/irq.h>
 
 #define POLL_TIMEOUT       (2 * HZ)

@@ -63,6 +64,7 @@ struct bfin_twi_iface {
 	int			msg_num;
 	int			cur_msg;
 	void __iomem		*regs_base;
+	int			bus_num;
 };

drivers/i2c/busses/i2c-bfin-twi.c DEFINE_TWI_REG(XMT_DATA16, 0x84) · line 97

Quoted 7 lines
drivers/i2c/busses/i2c-bfin-twi.c +4 -0
@@ -89,6 +91,23 @@ DEFINE_TWI_REG(XMT_DATA16, 0x84)
 DEFINE_TWI_REG(RCV_DATA8, 0x88)
 DEFINE_TWI_REG(RCV_DATA16, 0x8C)
 
+static u16 pin_req[2][3] = {
+	{P_TWI0_SCL, P_TWI0_SDA, 0},
+	{P_TWI1_SCL, P_TWI1_SDA, 0},
+};

Could be const.

drivers/i2c/busses/i2c-bfin-twi.c DEFINE_TWI_REG(XMT_DATA16, 0x84) · line 109

Quoted 12 lines
drivers/i2c/busses/i2c-bfin-twi.c +12 -0
+
+static int setup_pin_mux(struct bfin_twi_iface *iface, int action)
+{
+	int rc = 0;
+
+	if (action)
+		rc = peripheral_request_list(pin_req[iface->bus_num], "i2c-bfin-twi");
+	else
+		peripheral_free_list(pin_req[iface->bus_num]);
+
+	return rc;
+}

It didn't strike me the first time I reviewed this patch, but this function doesn't make much sense. Calling peripheral_request_list() and peripheral_free_list() directly would make the code more simple. But well, that's your driver, so that's your call. I can only hope that gcc properly optimizes away the additional costs.

drivers/i2c/busses/i2c-bfin-twi.c static int i2c_bfin_twi_probe(struct platform_device *pdev) · line 662

Quoted 9 lines
drivers/i2c/busses/i2c-bfin-twi.c +2 -0
+
 static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 {
 	unsigned short twi_int_status = read_INT_STAT(iface);

@@ -640,6 +659,7 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
 		goto out_error_no_irq;
 	}
 
+	iface->bus_num = pdev->id;

I fail to see why you duplicate this value... why don't you use pdev->id directly?

drivers/i2c/busses/i2c-bfin-twi.c static int i2c_bfin_twi_probe(struct platform_device *pdev) · line 678

Quoted 10 lines
drivers/i2c/busses/i2c-bfin-twi.c +3 -0
 	init_timer(&(iface->timeout_timer));
 	iface->timeout_timer.function = bfin_twi_timeout;
 	iface->timeout_timer.data = (unsigned long)iface;

@@ -653,6 +673,12 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
 	p_adap->class = I2C_CLASS_ALL;
 	p_adap->dev.parent = &pdev->dev;
 
+	rc = setup_pin_mux(iface, 1);
+	if (rc) {
+		dev_err(&pdev->dev, "Can't setup Pin Mux!\n");

s/Pin Mux/pin mux/

Quoted 23 lines
drivers/i2c/busses/i2c-bfin-twi.c +6 -0
+		goto out_error_pin_mux;
+	}
+
 	rc = request_irq(iface->irq, bfin_twi_interrupt_entry,
 		IRQF_DISABLED, pdev->name, iface);
 	if (rc) {

@@ -690,6 +716,8 @@ out_error_add_adapter:
 	free_irq(iface->irq, iface);
 out_error_req_irq:
 out_error_no_irq:
+	setup_pin_mux(iface, 0);
+out_error_pin_mux:
 	iounmap(iface->regs_base);
 out_error_ioremap:
 out_error_get_res:

@@ -706,6 +734,7 @@ static int i2c_bfin_twi_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&(iface->adap));
 	free_irq(iface->irq, iface);
+	setup_pin_mux(iface, 0);
 
 	return 0;
 }
Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw
Quoted 116 lines · Jean Delvare

Hi Bryan,

Quoted 34 lines · Bryan Wu

From: Bryan Wu <[email protected]>

Blackfin TWI controller hardware pin should be requested from GPIO port controller Before BF54x, there is no need to do this. But as long as BF54x and BF52x are supported by this generic driver, the missing pin mux operation should be added.

Signed-off-by Bryan Wu <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
---
 drivers/i2c/busses/i2c-bfin-twi.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
drivers/i2c/busses/i2c-bfin-twi.c +2 -0
index 515fe08..674c053 100644
@@ -34,6 +34,7 @@
 #include <linux/platform_device.h>
 
 #include <asm/blackfin.h>
+#include <asm/portmux.h>
 #include <asm/irq.h>
 
 #define POLL_TIMEOUT       (2 * HZ)

@@ -63,6 +64,7 @@ struct bfin_twi_iface {
      int                     msg_num;
      int                     cur_msg;
      void __iomem            *regs_base;
+     int                     bus_num;
 };

drivers/i2c/busses/i2c-bfin-twi.c DEFINE_TWI_REG(XMT_DATA16, 0x84) · line 97

Quoted 7 lines
drivers/i2c/busses/i2c-bfin-twi.c +4 -0
@@ -89,6 +91,23 @@ DEFINE_TWI_REG(XMT_DATA16, 0x84)
 DEFINE_TWI_REG(RCV_DATA8, 0x88)
 DEFINE_TWI_REG(RCV_DATA16, 0x8C)
 
+static u16 pin_req[2][3] = {
+     {P_TWI0_SCL, P_TWI0_SDA, 0},
+     {P_TWI1_SCL, P_TWI1_SDA, 0},
+};

Could be const.

drivers/i2c/busses/i2c-bfin-twi.c DEFINE_TWI_REG(XMT_DATA16, 0x84) · line 109

Quoted 12 lines
drivers/i2c/busses/i2c-bfin-twi.c +12 -0
+
+static int setup_pin_mux(struct bfin_twi_iface *iface, int action)
+{
+     int rc = 0;
+
+     if (action)
+             rc = peripheral_request_list(pin_req[iface->bus_num], "i2c-bfin-twi");
+     else
+             peripheral_free_list(pin_req[iface->bus_num]);
+
+     return rc;
+}

It didn't strike me the first time I reviewed this patch, but this function doesn't make much sense. Calling peripheral_request_list() and peripheral_free_list() directly would make the code more simple. But well, that's your driver, so that's your call. I can only hope that gcc properly optimizes away the additional costs.

drivers/i2c/busses/i2c-bfin-twi.c static int i2c_bfin_twi_probe(struct platform_device *pdev) · line 662

Quoted 9 lines
drivers/i2c/busses/i2c-bfin-twi.c +2 -0
+
 static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 {
      unsigned short twi_int_status = read_INT_STAT(iface);

@@ -640,6 +659,7 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
              goto out_error_no_irq;
      }
 
+     iface->bus_num = pdev->id;

I fail to see why you duplicate this value... why don't you use pdev->id directly?

drivers/i2c/busses/i2c-bfin-twi.c static int i2c_bfin_twi_probe(struct platform_device *pdev) · line 678

Quoted 10 lines
drivers/i2c/busses/i2c-bfin-twi.c +3 -0
      init_timer(&(iface->timeout_timer));
      iface->timeout_timer.function = bfin_twi_timeout;
      iface->timeout_timer.data = (unsigned long)iface;

@@ -653,6 +673,12 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
      p_adap->class = I2C_CLASS_ALL;
      p_adap->dev.parent = &pdev->dev;
 
+     rc = setup_pin_mux(iface, 1);
+     if (rc) {
+             dev_err(&pdev->dev, "Can't setup Pin Mux!\n");

s/Pin Mux/pin mux/

Quoted 17 lines
drivers/i2c/busses/i2c-bfin-twi.c +5 -0
+             goto out_error_pin_mux;
+     }
+
      rc = request_irq(iface->irq, bfin_twi_interrupt_entry,
              IRQF_DISABLED, pdev->name, iface);
      if (rc) {

@@ -690,6 +716,8 @@ out_error_add_adapter:
      free_irq(iface->irq, iface);
 out_error_req_irq:
 out_error_no_irq:
+     setup_pin_mux(iface, 0);
+out_error_pin_mux:
      iounmap(iface->regs_base);
 out_error_ioremap:
 out_error_get_res:

@@ -706,6 +734,7 @@ static int i2c_bfin_twi_remove(struct platform_device *pdev)
Quoted 8 lines

drivers/i2c/busses/i2c-bfin-twi.c static int i2c_bfin_twi_remove(struct platform_device *pdev) · line 740

Quoted 6 lines
drivers/i2c/busses/i2c-bfin-twi.c +1 -0
      i2c_del_adapter(&(iface->adap));
      free_irq(iface->irq, iface);
+     setup_pin_mux(iface, 0);
 
      return 0;
 }

You are right, I will update this patch. It will be more simple and better.

Thanks
-Bryan

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c