Skip to contentSkip to conversation

i2c-powermac: Simplify pmac_i2c_adapter_to_bus

PATCH In review Jean Delvare 2 participants 5 messages Oct 14, 2009 – Nov 5, 2009 2 messages not in the archive

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

PATCH 2/3 i2c-powermac: Simplify pmac_i2c_adapter_to_bus

pmac_i2c_adapter_to_bus can be simplified. We no longer need to walk the list of buses, we can get the right bus directly.

The only case where it makes a difference is if the provided adapter is _not_ a pmac_i2c_bus, but it is not supposed to happen anyway. So we can also drop the error checks.

Cc Benjamin Herrenschmidt <[email protected]>
---
 arch/powerpc/platforms/powermac/low_i2c.c   |    7 +------
 drivers/macintosh/windfarm_lm75_sensor.c    |    2 --
 drivers/macintosh/windfarm_max6690_sensor.c |    2 --
 drivers/macintosh/windfarm_smu_sat.c        |    2 --
 sound/aoa/codecs/onyx.c                     |    2 --
 sound/aoa/codecs/tas.c                      |    2 --
 6 files changed, 1 insertion(+), 16 deletions(-)
arch/powerpc/platforms/powermac/low_i2c.c +1 -6
@@ -1020,12 +1020,7 @@ EXPORT_SYMBOL_GPL(pmac_i2c_get_adapter);
 
 struct pmac_i2c_bus *pmac_i2c_adapter_to_bus(struct i2c_adapter *adapter)
 {
-	struct pmac_i2c_bus *bus;
-
-	list_for_each_entry(bus, &pmac_i2c_busses, link)
-		if (&bus->adapter == adapter)
-			return bus;
-	return NULL;
+	return container_of(adapter, struct pmac_i2c_bus, adapter);
 }
 EXPORT_SYMBOL_GPL(pmac_i2c_adapter_to_bus);
 
drivers/macintosh/windfarm_lm75_sensor.c +0 -2
@@ -173,8 +173,6 @@ static int wf_lm75_attach(struct i2c_ada
 	DBG("wf_lm75: adapter %s detected\n", adapter->name);
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	DBG("wf_lm75: bus found, looking for device...\n");
drivers/macintosh/windfarm_max6690_sensor.c +0 -2
@@ -135,8 +135,6 @@ static int wf_max6690_attach(struct i2c_
 	const char *loc;
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	while ((dev = of_get_next_child(busnode, dev)) != NULL) {
drivers/macintosh/windfarm_smu_sat.c +0 -2
@@ -359,8 +359,6 @@ static int wf_sat_attach(struct i2c_adap
 	struct pmac_i2c_bus *bus;
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	while ((dev = of_get_next_child(busnode, dev)) != NULL)
sound/aoa/codecs/onyx.c +0 -2
@@ -1077,8 +1077,6 @@ static int onyx_i2c_attach(struct i2c_ad
 	struct pmac_i2c_bus *bus;
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	while ((dev = of_get_next_child(busnode, dev)) != NULL) {
sound/aoa/codecs/tas.c +0 -2
@@ -958,8 +958,6 @@ static int tas_i2c_attach(struct i2c_ada
 	struct pmac_i2c_bus *bus;
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	while ((dev = of_get_next_child(busnode, dev)) != NULL) {
Signature · 1 line
Jean Delvare

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

#2 lore raw
Quoted 6 lines · Jean Delvare

pmac_i2c_adapter_to_bus can be simplified. We no longer need to walk the list of buses, we can get the right bus directly.

The only case where it makes a difference is if the provided adapter is _not_ a pmac_i2c_bus, but it is not supposed to happen anyway. So we can also drop the error checks.

Allright, I suspect that's the one causing my crashes, I need to confirm it still since the crashes are ellusive.

The thing is, there are other i2c adapters on the machine that aren't pmac_i2c, for example, the ones creates by the radeon driver for i2c probing.

What happens then is that code such as the windfarm stuff does:

static int wf_sat_attach(struct i2c_adapter *adapter)

{
	struct device_node *busnode, *dev = NULL;
	struct pmac_i2c_bus *bus;

	bus = pmac_i2c_adapter_to_bus(adapter);
	busnode = pmac_i2c_get_bus_node(bus);

	while ((dev = of_get_next_child(busnode, dev)) != NULL)
		if (of_device_is_compatible(dev, "smu-sat"))
			wf_sat_create(adapter, dev);
	return 0;
}

That will not work when such an adapter is in the system with your new code since pmac_i2c_adapter_to_bus() will return crap, causing busnode to basically be random bits of unrelated memory.

Now, what would be nice would be to convert the SMU sat stuff to use some new style OF based i2c probing but that is out of scope right now, so we need to drop that patch of yours at least for now.

I'll dbl check later today that this is indeed what's happening

Cheers,
Ben.

Quoted 84 lines
Cc Benjamin Herrenschmidt <[email protected]>
---
 arch/powerpc/platforms/powermac/low_i2c.c   |    7 +------
 drivers/macintosh/windfarm_lm75_sensor.c    |    2 --
 drivers/macintosh/windfarm_max6690_sensor.c |    2 --
 drivers/macintosh/windfarm_smu_sat.c        |    2 --
 sound/aoa/codecs/onyx.c                     |    2 --
 sound/aoa/codecs/tas.c                      |    2 --
 6 files changed, 1 insertion(+), 16 deletions(-)
arch/powerpc/platforms/powermac/low_i2c.c +1 -6
@@ -1020,12 +1020,7 @@ EXPORT_SYMBOL_GPL(pmac_i2c_get_adapter);
 
 struct pmac_i2c_bus *pmac_i2c_adapter_to_bus(struct i2c_adapter *adapter)
 {
-	struct pmac_i2c_bus *bus;
-
-	list_for_each_entry(bus, &pmac_i2c_busses, link)
-		if (&bus->adapter == adapter)
-			return bus;
-	return NULL;
+	return container_of(adapter, struct pmac_i2c_bus, adapter);
 }
 EXPORT_SYMBOL_GPL(pmac_i2c_adapter_to_bus);
 
drivers/macintosh/windfarm_lm75_sensor.c +0 -2
@@ -173,8 +173,6 @@ static int wf_lm75_attach(struct i2c_ada
 	DBG("wf_lm75: adapter %s detected\n", adapter->name);
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	DBG("wf_lm75: bus found, looking for device...\n");
drivers/macintosh/windfarm_max6690_sensor.c +0 -2
@@ -135,8 +135,6 @@ static int wf_max6690_attach(struct i2c_
 	const char *loc;
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	while ((dev = of_get_next_child(busnode, dev)) != NULL) {
drivers/macintosh/windfarm_smu_sat.c +0 -2
@@ -359,8 +359,6 @@ static int wf_sat_attach(struct i2c_adap
 	struct pmac_i2c_bus *bus;
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	while ((dev = of_get_next_child(busnode, dev)) != NULL)
sound/aoa/codecs/onyx.c +0 -2
@@ -1077,8 +1077,6 @@ static int onyx_i2c_attach(struct i2c_ad
 	struct pmac_i2c_bus *bus;
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	while ((dev = of_get_next_child(busnode, dev)) != NULL) {
sound/aoa/codecs/tas.c +0 -2
@@ -958,8 +958,6 @@ static int tas_i2c_attach(struct i2c_ada
 	struct pmac_i2c_bus *bus;
 
 	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
 	busnode = pmac_i2c_get_bus_node(bus);
 
 	while ((dev = of_get_next_child(busnode, dev)) != NULL) {
#3 lore raw

Hi Ben,

Quoted 31 lines · Benjamin Herrenschmidt
Quoted 6 lines · Jean Delvare

pmac_i2c_adapter_to_bus can be simplified. We no longer need to walk the list of buses, we can get the right bus directly.

The only case where it makes a difference is if the provided adapter is _not_ a pmac_i2c_bus, but it is not supposed to happen anyway. So we can also drop the error checks.

Allright, I suspect that's the one causing my crashes, I need to confirm it still since the crashes are ellusive.

The thing is, there are other i2c adapters on the machine that aren't pmac_i2c, for example, the ones creates by the radeon driver for i2c probing.

What happens then is that code such as the windfarm stuff does:

static int wf_sat_attach(struct i2c_adapter *adapter)

{
	struct device_node *busnode, *dev = NULL;
	struct pmac_i2c_bus *bus;

	bus = pmac_i2c_adapter_to_bus(adapter);
	busnode = pmac_i2c_get_bus_node(bus);

	while ((dev = of_get_next_child(busnode, dev)) != NULL)
		if (of_device_is_compatible(dev, "smu-sat"))
			wf_sat_create(adapter, dev);
	return 0;
}
Quoted 3 lines

That will not work when such an adapter is in the system with your new code since pmac_i2c_adapter_to_bus() will return crap, causing busnode to basically be random bits of unrelated memory.

You are correct. Not sure how I missed this, as I am aware that the radeonfb driver has its own I2C adapters.

Quoted 4 lines

Now, what would be nice would be to convert the SMU sat stuff to use some new style OF based i2c probing but that is out of scope right now, so we need to drop that patch of yours at least for now.

I agree. Let's simply drop this one patch. It was only a clean-up and the rest of the patches are still working and useful without it.

Quoted 1 line

I'll dbl check later today that this is indeed what's happening

I'm fairly certain your analysis is correct. As long as some drivers still use i2c_driver.attach_adapter, we can't make any assumption about which adapter is passed.

I had 6 i2c-powermac patches in my local queue:

i2c-powermac-01-multiple-msg-not-supported.patch i2c-powermac-02-refactor-xfer.patch i2c-powermac-03-report-errors.patch i2c-powermac-04-include-adapter-in-bus.patch i2c-powermac-05-simplify-pmac_i2c_adapter_to_bus.patch i2c-powermac-06-no-temp-buffer-for-name.patch

I only have a formal ack from you for patch 01. Patches 01, 02 and 03 have been in linux-next for about a month now. I will delete patch 05 now. This leaves us with 4 patches still waiting for your ack (02, 03, 04 and 06.) I'm not sure which ones you tested so far?

Thanks,

Signature · 1 line
Jean Delvare

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

#5 lore raw
Quoted 8 lines · Jean Delvare

i2c-powermac-01-multiple-msg-not-supported.patch i2c-powermac-02-refactor-xfer.patch i2c-powermac-03-report-errors.patch i2c-powermac-04-include-adapter-in-bus.patch i2c-powermac-05-simplify-pmac_i2c_adapter_to_bus.patch i2c-powermac-06-no-temp-buffer-for-name.patch

Quoted 4 lines

I only have a formal ack from you for patch 01. Patches 01, 02 and 03 have been in linux-next for about a month now. I will delete patch 05 now. This leaves us with 4 patches still waiting for your ack (02, 03, 04 and 06.) I'm not sure which ones you tested so far?

I tested 4,5 and 6 though 5 will be dropped. 1 is acked. I think 2 and 3 slipped through the cracks. Where do I find your patch queue ? I'll test tomorrow.

Cheers,
Ben.

#6 lore raw
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 8 lines · Jean Delvare

i2c-powermac-01-multiple-msg-not-supported.patch i2c-powermac-02-refactor-xfer.patch i2c-powermac-03-report-errors.patch i2c-powermac-04-include-adapter-in-bus.patch i2c-powermac-05-simplify-pmac_i2c_adapter_to_bus.patch i2c-powermac-06-no-temp-buffer-for-name.patch

Quoted 8 lines
Quoted 4 lines

I only have a formal ack from you for patch 01. Patches 01, 02 and 03 have been in linux-next for about a month now. I will delete patch 05 now. This leaves us with 4 patches still waiting for your ack (02, 03, 04 and 06.) I'm not sure which ones you tested so far?

I tested 4,5 and 6 though 5 will be dropped. 1 is acked. I think 2 and 3 slipped through the cracks. Where do I find your patch queue ? I'll test tomorrow.

ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/

i2c-powermac-*.patch

Thanks,

Signature · 1 line
Jean Delvare