Skip to contentSkip to conversation

i2c: Race fix for i2c-mpc.c

PATCH In review Sylvain Munaut 2 participants 4 messages May 16, 2005 – May 17, 2005

Subsystems (2)
I2C subsystem host drivers Freescale mpc I2C driver
lore
Share & tools
lore

Sylvain Munaut

author
#0 lore raw

PATCH 1/1 i2c: Race fix for i2c-mpc.c

i2c: Race fix for i2c-mpc.c

The problem was that the clock speed and driver data is initialized after the i2c adapter was added. This caused the i2c bus to start working at a wrong speed. (Mostly noticable on the second bus on mpc5200)

With this patch we've tried to keep the i2c adapter working perfectly all the time it is included in the system. Initialize before added, Remove garbage after deleleted.

Submitted-by Asier Llano Palacios
Signed-off-by Sylvain Munaut <[email protected]>

---

drivers/i2c/busses/i2c-mpc.c +4 -3
@@ -333,6 +333,9 @@
 	} else
 		i2c->irq = 0;
 
+	mpc_i2c_setclock(i2c);
+	ocp_set_drvdata(ocp, i2c);
+
 	i2c->adap = mpc_ops;
 	i2c_set_adapdata(&i2c->adap, i2c);
 

@@ -341,8 +344,6 @@
 		goto fail_add;
 	}
 
-	mpc_i2c_setclock(i2c);
-	ocp_set_drvdata(ocp, i2c);
 	return result;
 
       fail_add:

@@ -358,8 +359,8 @@
 static void __devexit mpc_i2c_remove(struct ocp_device *ocp)
 {
 	struct mpc_i2c *i2c = ocp_get_drvdata(ocp);
-	ocp_set_drvdata(ocp, NULL);
 	i2c_del_adapter(&i2c->adap);
+	ocp_set_drvdata(ocp, NULL);
 
 	if (ocp->def->irq != OCP_IRQ_NA)
 		free_irq(i2c->irq, i2c);
#1 lore raw

Sylvain,

Are you really still using the OCP side of the driver? Do we need a=20 similar fix for the platform driver side?

- kumar

Quoted 55 lines · Sylvain Munaut

i2c: Race fix for i2c-mpc.c

The problem was that the clock speed and driver data is
initialized after the i2c adapter was added. This caused the i2c bus to start working at a wrong speed. (Mostly
noticable on the second bus on mpc5200)

With this patch we've tried to keep the i2c adapter
working perfectly all the time it is included in the system. Initialize before added, Remove garbage after deleleted.

Submitted-by Asier Llano Palacios
Signed-off-by Sylvain Munaut <[email protected]>

---
--- linux-2.6/drivers/i2c/busses/i2c-mpc.c=A0=A0=A0=A0=A0 2005-05-16=20=

09:08:02.000000000 +0200
+++ linux-2.6/drivers/i2c/busses/i2c-mpc.c=A0=A0=A0=A0=A0 2005-05-16=20=

09:09:51.000000000 +0200
@@ -333,6 +333,9 @@
=A0=A0=A0=A0=A0=A0=A0 } else =A0=A0=A0=A0=A0=A0=A0 =A0=A0=A0=A0=A0=A0=A0 i2c->irq =3D 0; =A0

 +=A0=A0=A0=A0=A0=A0 mpc_i2c_setclock(i2c);
+=A0=A0=A0=A0=A0=A0 ocp_set_drvdata(ocp, i2c);

+
=A0=A0=A0=A0=A0=A0=A0 i2c->adap =3D mpc_ops; =A0=A0=A0=A0=A0=A0=A0 i2c_set_adapdata(&i2c->adap, i2c); =A0
@@ -341,8 +344,6 @@
=A0=A0=A0=A0=A0=A0=A0 =A0=A0=A0=A0=A0=A0=A0 goto fail_add; =A0=A0=A0=A0=A0=A0=A0 }
=A0
-=A0=A0=A0=A0=A0=A0 mpc_i2c_setclock(i2c);
-=A0=A0=A0=A0=A0=A0 ocp_set_drvdata(ocp, i2c);
=A0=A0=A0=A0=A0=A0=A0 return result;
=A0
=A0=A0=A0=A0=A0=A0 fail_add:
@@ -358,8 +359,8 @@
=A0static void __devexit mpc_i2c_remove(struct ocp_device *ocp) =A0{
=A0=A0=A0=A0=A0=A0=A0 struct mpc_i2c *i2c =3D ocp_get_drvdata(ocp);
-=A0=A0=A0=A0=A0=A0 ocp_set_drvdata(ocp, NULL);
=A0=A0=A0=A0=A0=A0=A0 i2c_del_adapter(&i2c->adap); +=A0=A0=A0=A0=A0=A0 ocp_set_drvdata(ocp, NULL);
=A0
=A0=A0=A0=A0=A0=A0=A0 if (ocp->def->irq !=3D OCP_IRQ_NA)
=A0=A0=A0=A0=A0=A0=A0 =A0=A0=A0=A0=A0=A0=A0 free_irq(i2c->irq, i2c);
_______________________________________________
Linuxppc-embedded mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-embedded

author
#2 lore raw
Quoted 3 lines · Kumar Gala

Sylvain,

Are you really still using the OCP side of the driver? Do we need a similar fix for the platform driver side?

/me hits himself with a hammer

Damn I included the wrong diff ... Sorry about that, the good one is in attachment.

No I don't use the OCP side but I changed both to stay coherent. I don't experience the problem myself, it's Asier who reported it and it apparently mostly shows up on the second i2c bus (where I have nothing on my hardware and anyway my bootloader init I2C beforehand ...).

But the patch looks correct, when a bus is added, it should be ready to be used.

Sylvain

---
i2c: Race fix for i2c-mpc.c

The problem was that the clock speed and driver data is initialized after the i2c adapter was added. This caused the i2c bus to start working at a wrong speed. (Mostly noticable on the second bus on mpc5200)

With this patch we've tried to keep the i2c adapter working perfectly all the time it is included in the system. Initialize before added, Remove garbage after deleleted.

Submitted-by Asier Llano Palacios
Signed-off-by Sylvain Munaut <[email protected]>

---

drivers/i2c/busses/i2c-mpc.c +8 -6
@@ -333,6 +333,9 @@
 	} else
 		i2c->irq = 0;
 
+	mpc_i2c_setclock(i2c);
+	ocp_set_drvdata(ocp, i2c);
+
 	i2c->adap = mpc_ops;
 	i2c_set_adapdata(&i2c->adap, i2c);
 

@@ -341,8 +344,6 @@
 		goto fail_add;
 	}
 
-	mpc_i2c_setclock(i2c);
-	ocp_set_drvdata(ocp, i2c);
 	return result;
 
       fail_add:

@@ -358,8 +359,8 @@
 static void __devexit mpc_i2c_remove(struct ocp_device *ocp)
 {
 	struct mpc_i2c *i2c = ocp_get_drvdata(ocp);
-	ocp_set_drvdata(ocp, NULL);
 	i2c_del_adapter(&i2c->adap);
+	ocp_set_drvdata(ocp, NULL);
 
 	if (ocp->def->irq != OCP_IRQ_NA)
 		free_irq(i2c->irq, i2c);

@@ -430,6 +431,9 @@
 			goto fail_irq;
 		}
 
+	mpc_i2c_setclock(i2c);
+	dev_set_drvdata(device, i2c);
+
 	i2c->adap = mpc_ops;
 	i2c_set_adapdata(&i2c->adap, i2c);
 	i2c->adap.dev.parent = &pdev->dev;

@@ -438,8 +442,6 @@
 		goto fail_add;
 	}
 
-	mpc_i2c_setclock(i2c);
-	dev_set_drvdata(device, i2c);
 	return result;
 
       fail_add:

@@ -456,8 +458,8 @@
 {
 	struct mpc_i2c *i2c = dev_get_drvdata(device);
 
-	dev_set_drvdata(device, NULL);
 	i2c_del_adapter(&i2c->adap);
+	dev_set_drvdata(device, NULL);
 
 	if (i2c->irq != 0)
 		free_irq(i2c->irq, i2c);
#3 lore raw

Sylvain,

Looks reasonable to me. Add a Signed-off-by: Kumar Gala=20 <[email protected]> line and sent to GregKH, lm-sensors guys. =20 CC me if you would.

- kumar

Quoted 4 lines · Sylvain Munaut
Quoted 3 lines · Kumar Gala

Sylvain,

Are you really still using the OCP side of the driver?=A0 Do we need =

a

Quoted 9 lines
Quoted 1 line

similar fix for the platform driver side?

/me hits himself with a hammer

Damn I included the wrong diff ... Sorry about that, the good one is
in attachment.

Quoted 7 lines

No I don't use the OCP side but I changed both to stay coherent. I=20 don't
experience the problem myself, it's Asier who reported it and it apparently mostly shows up on the second i2c bus (where I have nothing on my hardware and anyway my bootloader init I2C beforehand ...).

But the patch looks correct, when a bus is added, it should be ready =

to

Quoted 26 lines

be used.

=A0=A0=A0=A0=A0=A0=A0 Sylvain

---
i2c: Race fix for i2c-mpc.c

The problem was that the clock speed and driver data is
initialized after the i2c adapter was added. This caused the i2c bus to start working at a wrong speed. (Mostly
noticable on the second bus on mpc5200)

With this patch we've tried to keep the i2c adapter
working perfectly all the time it is included in the system. Initialize before added, Remove garbage after deleleted.

Submitted-by Asier Llano Palacios
Signed-off-by Sylvain Munaut <[email protected]>

---
<i2c-mpc-racefix.diff>=