[PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

The legacy i2c binding model is going away soon, so convert the AOA
codec drivers to the new model or they'll break.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Tested-by: Johannes Berg <johannes@sipsolutions.net>
Tested-by: Andreas Schwab <schwab@linux-m68k.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Takashi Iwai <tiwai@suse.de>
---
Takashi, please push this patch to Linus quickly, as this is blocking
the removal of the legacy i2c binding model, which is scheduled for
2.6.30.

 sound/aoa/codecs/onyx.c |   76 ++++++++++++++++++++++++++++++++---------------
 sound/aoa/codecs/tas.c  |   66 +++++++++++++++++++++++++---------------
 2 files changed, 95 insertions(+), 47 deletions(-)

--- linux-2.6.30-rc2.orig/sound/aoa/codecs/onyx.c	2009-04-15 09:55:20.000000000 +0200
+++ linux-2.6.30-rc2/sound/aoa/codecs/onyx.c	2009-04-15 14:07:50.000000000 +0200
@@ -47,7 +47,7 @@ MODULE_DESCRIPTION("pcm3052 (onyx) codec
 struct onyx {
 	/* cache registers 65 to 80, they are write-only! */
 	u8			cache[16];
-	struct i2c_client	i2c;
+	struct i2c_client	*i2c;
 	struct aoa_codec	codec;
 	u32			initialised:1,
 				spdif_locked:1,
@@ -72,7 +72,7 @@ static int onyx_read_register(struct ony
 		*value = onyx->cache[reg-FIRSTREGISTER];
 		return 0;
 	}
-	v = i2c_smbus_read_byte_data(&onyx->i2c, reg);
+	v = i2c_smbus_read_byte_data(onyx->i2c, reg);
 	if (v < 0)
 		return -1;
 	*value = (u8)v;
@@ -84,7 +84,7 @@ static int onyx_write_register(struct on
 {
 	int result;
 
-	result = i2c_smbus_write_byte_data(&onyx->i2c, reg, value);
+	result = i2c_smbus_write_byte_data(onyx->i2c, reg, value);
 	if (!result)
 		onyx->cache[reg-FIRSTREGISTER] = value;
 	return result;
@@ -996,12 +996,45 @@ static void onyx_exit_codec(struct aoa_c
 	onyx->codec.soundbus_dev->detach_codec(onyx->codec.soundbus_dev, onyx);
 }
 
-static struct i2c_driver onyx_driver;
-
 static int onyx_create(struct i2c_adapter *adapter,
 		       struct device_node *node,
 		       int addr)
 {
+	struct i2c_board_info info;
+	struct i2c_client *client;
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "aoa_codec_onyx", I2C_NAME_SIZE);
+	info.addr = addr;
+	info.platform_data = node;
+	client = i2c_new_device(adapter, &info);
+	if (!client)
+		return -ENODEV;
+
+	/*
+	 * We know the driver is already loaded, so the device should be
+	 * already bound. If not it means binding failed, which suggests
+	 * the device doesn't really exist and should be deleted.
+	 * Ideally this would be replaced by better checks _before_
+	 * instantiating the device.
+	 */
+	if (!client->driver) {
+		i2c_unregister_device(client);
+		return -ENODEV;
+	}
+
+	/*
+	 * Let i2c-core delete that device on driver removal.
+	 * This is safe because i2c-core holds the core_lock mutex for us.
+	 */
+	list_add_tail(&client->detected, &client->driver->clients);
+	return 0;
+}
+
+static int onyx_i2c_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct device_node *node = client->dev.platform_data;
 	struct onyx *onyx;
 	u8 dummy;
 
@@ -1011,20 +1044,12 @@ static int onyx_create(struct i2c_adapte
 		return -ENOMEM;
 
 	mutex_init(&onyx->mutex);
-	onyx->i2c.driver = &onyx_driver;
-	onyx->i2c.adapter = adapter;
-	onyx->i2c.addr = addr & 0x7f;
-	strlcpy(onyx->i2c.name, "onyx audio codec", I2C_NAME_SIZE);
-
-	if (i2c_attach_client(&onyx->i2c)) {
-		printk(KERN_ERR PFX "failed to attach to i2c\n");
-		goto fail;
-	}
+	onyx->i2c = client;
+	i2c_set_clientdata(client, onyx);
 
 	/* we try to read from register ONYX_REG_CONTROL
 	 * to check if the codec is present */
 	if (onyx_read_register(onyx, ONYX_REG_CONTROL, &dummy) != 0) {
-		i2c_detach_client(&onyx->i2c);
 		printk(KERN_ERR PFX "failed to read control register\n");
 		goto fail;
 	}
@@ -1036,14 +1061,14 @@ static int onyx_create(struct i2c_adapte
 	onyx->codec.node = of_node_get(node);
 
 	if (aoa_codec_register(&onyx->codec)) {
-		i2c_detach_client(&onyx->i2c);
 		goto fail;
 	}
 	printk(KERN_DEBUG PFX "created and attached onyx instance\n");
 	return 0;
  fail:
+	i2c_set_clientdata(client, NULL);
 	kfree(onyx);
-	return -EINVAL;
+	return -ENODEV;
 }
 
 static int onyx_i2c_attach(struct i2c_adapter *adapter)
@@ -1080,28 +1105,33 @@ static int onyx_i2c_attach(struct i2c_ad
 	return onyx_create(adapter, NULL, 0x47);
 }
 
-static int onyx_i2c_detach(struct i2c_client *client)
+static int onyx_i2c_remove(struct i2c_client *client)
 {
-	struct onyx *onyx = container_of(client, struct onyx, i2c);
-	int err;
+	struct onyx *onyx = i2c_get_clientdata(client);
 
-	if ((err = i2c_detach_client(client)))
-		return err;
 	aoa_codec_unregister(&onyx->codec);
 	of_node_put(onyx->codec.node);
 	if (onyx->codec_info)
 		kfree(onyx->codec_info);
+	i2c_set_clientdata(client, onyx);
 	kfree(onyx);
 	return 0;
 }
 
+static const struct i2c_device_id onyx_i2c_id[] = {
+	{ "aoa_codec_onyx", 0 },
+	{ }
+};
+
 static struct i2c_driver onyx_driver = {
 	.driver = {
 		.name = "aoa_codec_onyx",
 		.owner = THIS_MODULE,
 	},
 	.attach_adapter = onyx_i2c_attach,
-	.detach_client = onyx_i2c_detach,
+	.probe = onyx_i2c_probe,
+	.remove = onyx_i2c_remove,
+	.id_table = onyx_i2c_id,
 };
 
 static int __init onyx_init(void)
--- linux-2.6.30-rc2.orig/sound/aoa/codecs/tas.c	2009-04-15 09:55:20.000000000 +0200
+++ linux-2.6.30-rc2/sound/aoa/codecs/tas.c	2009-04-15 09:56:39.000000000 +0200
@@ -82,7 +82,7 @@ MODULE_DESCRIPTION("tas codec driver for
 
 struct tas {
 	struct aoa_codec	codec;
-	struct i2c_client	i2c;
+	struct i2c_client	*i2c;
 	u32			mute_l:1, mute_r:1 ,
 				controls_created:1 ,
 				drc_enabled:1,
@@ -108,9 +108,9 @@ static struct tas *codec_to_tas(struct a
 static inline int tas_write_reg(struct tas *tas, u8 reg, u8 len, u8 *data)
 {
 	if (len == 1)
-		return i2c_smbus_write_byte_data(&tas->i2c, reg, *data);
+		return i2c_smbus_write_byte_data(tas->i2c, reg, *data);
 	else
-		return i2c_smbus_write_i2c_block_data(&tas->i2c, reg, len, data);
+		return i2c_smbus_write_i2c_block_data(tas->i2c, reg, len, data);
 }
 
 static void tas3004_set_drc(struct tas *tas)
@@ -882,12 +882,34 @@ static void tas_exit_codec(struct aoa_co
 }
 
 
-static struct i2c_driver tas_driver;
-
 static int tas_create(struct i2c_adapter *adapter,
 		       struct device_node *node,
 		       int addr)
 {
+	struct i2c_board_info info;
+	struct i2c_client *client;
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "aoa_codec_tas", I2C_NAME_SIZE);
+	info.addr = addr;
+	info.platform_data = node;
+
+	client = i2c_new_device(adapter, &info);
+	if (!client)
+		return -ENODEV;
+
+	/*
+	 * Let i2c-core delete that device on driver removal.
+	 * This is safe because i2c-core holds the core_lock mutex for us.
+	 */
+	list_add_tail(&client->detected, &client->driver->clients);
+	return 0;
+}
+
+static int tas_i2c_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device_node *node = client->dev.platform_data;
 	struct tas *tas;
 
 	tas = kzalloc(sizeof(struct tas), GFP_KERNEL);
@@ -896,17 +918,11 @@ static int tas_create(struct i2c_adapter
 		return -ENOMEM;
 
 	mutex_init(&tas->mtx);
-	tas->i2c.driver = &tas_driver;
-	tas->i2c.adapter = adapter;
-	tas->i2c.addr = addr;
+	tas->i2c = client;
+	i2c_set_clientdata(client, tas);
+
 	/* seems that half is a saner default */
 	tas->drc_range = TAS3004_DRC_MAX / 2;
-	strlcpy(tas->i2c.name, "tas audio codec", I2C_NAME_SIZE);
-
-	if (i2c_attach_client(&tas->i2c)) {
-		printk(KERN_ERR PFX "failed to attach to i2c\n");
-		goto fail;
-	}
 
 	strlcpy(tas->codec.name, "tas", MAX_CODEC_NAME_LEN);
 	tas->codec.owner = THIS_MODULE;
@@ -915,14 +931,12 @@ static int tas_create(struct i2c_adapter
 	tas->codec.node = of_node_get(node);
 
 	if (aoa_codec_register(&tas->codec)) {
-		goto detach;
+		goto fail;
 	}
 	printk(KERN_DEBUG
 	       "snd-aoa-codec-tas: tas found, addr 0x%02x on %s\n",
-	       addr, node->full_name);
+	       (unsigned int)client->addr, node->full_name);
 	return 0;
- detach:
-	i2c_detach_client(&tas->i2c);
  fail:
 	mutex_destroy(&tas->mtx);
 	kfree(tas);
@@ -970,14 +984,11 @@ static int tas_i2c_attach(struct i2c_ada
 	return -ENODEV;
 }
 
-static int tas_i2c_detach(struct i2c_client *client)
+static int tas_i2c_remove(struct i2c_client *client)
 {
-	struct tas *tas = container_of(client, struct tas, i2c);
-	int err;
+	struct tas *tas = i2c_get_clientdata(client);
 	u8 tmp = TAS_ACR_ANALOG_PDOWN;
 
-	if ((err = i2c_detach_client(client)))
-		return err;
 	aoa_codec_unregister(&tas->codec);
 	of_node_put(tas->codec.node);
 
@@ -989,13 +1000,20 @@ static int tas_i2c_detach(struct i2c_cli
 	return 0;
 }
 
+static const struct i2c_device_id tas_i2c_id[] = {
+	{ "aoa_codec_tas", 0 },
+	{ }
+};
+
 static struct i2c_driver tas_driver = {
 	.driver = {
 		.name = "aoa_codec_tas",
 		.owner = THIS_MODULE,
 	},
 	.attach_adapter = tas_i2c_attach,
-	.detach_client = tas_i2c_detach,
+	.probe = tas_i2c_probe,
+	.remove = tas_i2c_remove,
+	.id_table = tas_i2c_id,
 };
 
 static int __init tas_init(void)


-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Mon, 2009-04-20 at 22:54 +0200, Jean Delvare wrote:
> The legacy i2c binding model is going away soon, so convert the AOA
> codec drivers to the new model or they'll break.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Tested-by: Johannes Berg <johannes@sipsolutions.net>
> Tested-by: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> ---
> Takashi, please push this patch to Linus quickly, as this is blocking
> the removal of the legacy i2c binding model, which is scheduled for
> 2.6.30.

Have you taken care of snd-powermac similarly?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

Hi Johannes,

On Mon, 20 Apr 2009 23:04:52 +0200, Johannes Berg wrote:
> On Mon, 2009-04-20 at 22:54 +0200, Jean Delvare wrote:
> > The legacy i2c binding model is going away soon, so convert the AOA
> > codec drivers to the new model or they'll break.
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Tested-by: Johannes Berg <johannes@sipsolutions.net>
> > Tested-by: Andreas Schwab <schwab@linux-m68k.org>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > ---
> > Takashi, please push this patch to Linus quickly, as this is blocking
> > the removal of the legacy i2c binding model, which is scheduled for
> > 2.6.30.
> 
> Have you taken care of snd-powermac similarly?

Yes I did, see my patch "keywest: Convert to new-style i2c driver" to
the alsa-devel and linuxppc-dev mailing lists:
http://ozlabs.org/pipermail/linuxppc-dev/2009-April/070884.html

Would you be able to test this patch? This would be very welcome. I can
resend it to you if it helps.

Thanks,
-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

Hi,

> > Have you taken care of snd-powermac similarly?
> 
> Yes I did, see my patch "keywest: Convert to new-style i2c driver" to
> the alsa-devel and linuxppc-dev mailing lists:
> http://ozlabs.org/pipermail/linuxppc-dev/2009-April/070884.html
> 
> Would you be able to test this patch? This would be very welcome. I can
> resend it to you if it helps.

Cool. No, was just wondering, I can't test it, none of my machines can
use that (which is why I wrote aoa, heh)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Takashi Iwai]

At Mon, 20 Apr 2009 22:54:25 +0200,
Jean Delvare wrote:
> 
> The legacy i2c binding model is going away soon, so convert the AOA
> codec drivers to the new model or they'll break.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Tested-by: Johannes Berg <johannes@sipsolutions.net>
> Tested-by: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> ---
> Takashi, please push this patch to Linus quickly, as this is blocking
> the removal of the legacy i2c binding model, which is scheduled for
> 2.6.30.

Applied now.  Thanks.


Takashi

[PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

Hi all,

The legacy i2c model is going away soon, the remaining drivers that
still use it need to be converted very quickly. There are 3 sound
drivers remaining:

sound/aoa/codecs/onyx.c
sound/aoa/codecs/tas.c
sound/ppc/keywest.c

I've given a try to the former two, patch below. I could only
build-test it, so I would appreciate if anyone with supported hardware
could test the patch. I also welcome reviews and comments, of course. I
am not familiar with PowerPC so I might as well have done it wrong.

Basically the idea is to move the I2C device instantiation from the
codec drivers (onyx, tas) to the I2C adapter driver (i2c-powermac.)
This follows the Linux device driver model, requires slightly less
code, runs faster and and lets the required chip drivers be loaded by
udev or similar automatically.

The last driver to convert, keywest, is a mystery to me. I have a hard
time understanding how it interacts with tumbler and daca. I have the
feeling that it is essentially a glue module to workaround the legacy
i2c model limitations, so probably it could go away entirely now, but
I'm not sure how to do that in practice. Maybe tumbler and daca would
be made separate i2c drivers, I'm not sure.

One thing in particular which I find strange is that tumbler has
references to the TAS3004 device, much like the tas codec driver. It is
unclear to me whether tas is a replacement for tumbler, or if both
drivers work together, or if they are for separate hardware. I would
appreciate clarifications about this.

Thanks.

* * * * *

The legacy i2c binding model is going away soon, so convert the AOA
codec drivers to the new model or they'll break.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/i2c/busses/i2c-powermac.c |   76 +++++++++++++++++++++++++++++++
 sound/aoa/codecs/onyx.c           |   77 +++++++-------------------------
 sound/aoa/codecs/tas.c            |   89 +++++++++----------------------------
 3 files changed, 116 insertions(+), 126 deletions(-)

--- linux-2.6.30-rc1.orig/drivers/i2c/busses/i2c-powermac.c	2009-04-08 08:52:48.000000000 +0200
+++ linux-2.6.30-rc1/drivers/i2c/busses/i2c-powermac.c	2009-04-08 13:48:31.000000000 +0200
@@ -204,7 +204,7 @@ static int __devexit i2c_powermac_remove
 static int __devinit i2c_powermac_probe(struct platform_device *dev)
 {
 	struct pmac_i2c_bus *bus = dev->dev.platform_data;
-	struct device_node *parent = NULL;
+	struct device_node *parent = NULL, *busnode, *devnode;
 	struct i2c_adapter *adapter;
 	char name[32];
 	const char *basename;
@@ -212,6 +212,7 @@ static int __devinit i2c_powermac_probe(
 
 	if (bus == NULL)
 		return -EINVAL;
+	busnode = pmac_i2c_get_bus_node(bus);
 
 	/* Ok, now we need to make up a name for the interface that will
 	 * match what we used to do in the past, that is basically the
@@ -289,6 +290,79 @@ static int __devinit i2c_powermac_probe(
 		}
 	}
 
+	devnode = NULL;
+	while ((devnode = of_get_next_child(busnode, devnode)) != NULL) {
+		struct i2c_board_info info;
+		const u32 *addr;
+
+		/* Instantiate I2C sound device if present */
+		if (of_device_is_compatible(devnode, "pcm3052")) {
+			printk(KERN_DEBUG "i2c-powermac: found pcm3052\n");
+			addr = of_get_property(devnode, "reg", NULL);
+			if (!addr)
+				continue;
+
+			memset(&info, 0, sizeof(struct i2c_board_info));
+			info.addr = (*addr) >> 1;
+			strlcpy(info.type, "aoa_codec_onyx", I2C_NAME_SIZE);
+			info.platform_data = devnode;
+			i2c_new_device(adapter, &info);
+			continue;
+		}
+
+		if (of_device_is_compatible(devnode, "tas3004")) {
+			printk(KERN_DEBUG "i2c-powermac: found tas3004\n");
+			addr = of_get_property(devnode, "reg", NULL);
+			if (!addr)
+				continue;
+
+			memset(&info, 0, sizeof(struct i2c_board_info));
+			info.addr = ((*addr) >> 1) & 0x7f;
+			strlcpy(info.type, "aoa_codec_tas", I2C_NAME_SIZE);
+			info.platform_data = devnode;
+			i2c_new_device(adapter, &info);
+			continue;
+		}
+
+		/* older machines have no 'codec' node with a 'compatible'
+		 * property that says 'tas3004', they just have a 'deq'
+		 * node without any such property... */
+		if (strcmp(devnode->name, "deq") == 0) {
+			printk(KERN_DEBUG "i2c-powermac: found 'deq' node\n");
+			addr = of_get_property(devnode, "i2c-address", NULL);
+			if (!addr)
+				continue;
+
+			memset(&info, 0, sizeof(struct i2c_board_info));
+			info.addr = ((*addr) >> 1) & 0x7f;
+			/* now, if the address doesn't match any of the two
+			 * that a tas3004 can have, we cannot handle this.
+			 * I doubt it ever happens but hey. */
+			if (info.addr != 0x34 && info.addr != 0x35)
+				continue;
+			strlcpy(info.type, "aoa_codec_tas", I2C_NAME_SIZE);
+			info.platform_data = devnode;
+			i2c_new_device(adapter, &info);
+			continue;
+		}
+
+		/* if that didn't work, try desperate mode for older
+		 * machines that have stuff missing from the device tree */
+		if (of_device_is_compatible(busnode, "k2-i2c")) {
+			unsigned short probe_onyx[] = {
+				0x46, 0x47, I2C_CLIENT_END
+			};
+
+			printk(KERN_DEBUG "i2c-powermac: found k2-i2c, "
+			       "checking if onyx chip is on it\n");
+
+			memset(&info, 0, sizeof(struct i2c_board_info));
+			strlcpy(info.type, "aoa_codec_onyx", I2C_NAME_SIZE);
+			info.platform_data = devnode;
+			i2c_new_probed_device(adapter, &info, probe_onyx);
+		}
+	}
+
 	return rc;
 }
 
--- linux-2.6.30-rc1.orig/sound/aoa/codecs/onyx.c	2009-03-26 15:08:13.000000000 +0100
+++ linux-2.6.30-rc1/sound/aoa/codecs/onyx.c	2009-04-08 13:50:11.000000000 +0200
@@ -47,7 +47,7 @@ MODULE_DESCRIPTION("pcm3052 (onyx) codec
 struct onyx {
 	/* cache registers 65 to 80, they are write-only! */
 	u8			cache[16];
-	struct i2c_client	i2c;
+	struct i2c_client	*i2c;
 	struct aoa_codec	codec;
 	u32			initialised:1,
 				spdif_locked:1,
@@ -72,7 +72,7 @@ static int onyx_read_register(struct ony
 		*value = onyx->cache[reg-FIRSTREGISTER];
 		return 0;
 	}
-	v = i2c_smbus_read_byte_data(&onyx->i2c, reg);
+	v = i2c_smbus_read_byte_data(onyx->i2c, reg);
 	if (v < 0)
 		return -1;
 	*value = (u8)v;
@@ -84,7 +84,7 @@ static int onyx_write_register(struct on
 {
 	int result;
 
-	result = i2c_smbus_write_byte_data(&onyx->i2c, reg, value);
+	result = i2c_smbus_write_byte_data(onyx->i2c, reg, value);
 	if (!result)
 		onyx->cache[reg-FIRSTREGISTER] = value;
 	return result;
@@ -998,10 +998,10 @@ static void onyx_exit_codec(struct aoa_c
 
 static struct i2c_driver onyx_driver;
 
-static int onyx_create(struct i2c_adapter *adapter,
-		       struct device_node *node,
-		       int addr)
+static int onyx_i2c_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
 {
+	struct device_node *node = client->dev.platform_data;
 	struct onyx *onyx;
 	u8 dummy;
 
@@ -1011,20 +1011,12 @@ static int onyx_create(struct i2c_adapte
 		return -ENOMEM;
 
 	mutex_init(&onyx->mutex);
-	onyx->i2c.driver = &onyx_driver;
-	onyx->i2c.adapter = adapter;
-	onyx->i2c.addr = addr & 0x7f;
-	strlcpy(onyx->i2c.name, "onyx audio codec", I2C_NAME_SIZE);
-
-	if (i2c_attach_client(&onyx->i2c)) {
-		printk(KERN_ERR PFX "failed to attach to i2c\n");
-		goto fail;
-	}
+	onyx->i2c = client;
+	i2c_set_clientdata(client, onyx);
 
 	/* we try to read from register ONYX_REG_CONTROL
 	 * to check if the codec is present */
 	if (onyx_read_register(onyx, ONYX_REG_CONTROL, &dummy) != 0) {
-		i2c_detach_client(&onyx->i2c);
 		printk(KERN_ERR PFX "failed to read control register\n");
 		goto fail;
 	}
@@ -1036,7 +1028,6 @@ static int onyx_create(struct i2c_adapte
 	onyx->codec.node = of_node_get(node);
 
 	if (aoa_codec_register(&onyx->codec)) {
-		i2c_detach_client(&onyx->i2c);
 		goto fail;
 	}
 	printk(KERN_DEBUG PFX "created and attached onyx instance\n");
@@ -1046,47 +1037,10 @@ static int onyx_create(struct i2c_adapte
 	return -EINVAL;
 }
 
-static int onyx_i2c_attach(struct i2c_adapter *adapter)
+static int onyx_i2c_remove(struct i2c_client *client)
 {
-	struct device_node *busnode, *dev = NULL;
-	struct pmac_i2c_bus *bus;
-
-	bus = pmac_i2c_adapter_to_bus(adapter);
-	if (bus == NULL)
-		return -ENODEV;
-	busnode = pmac_i2c_get_bus_node(bus);
+	struct onyx *onyx = i2c_get_clientdata(client);
 
-	while ((dev = of_get_next_child(busnode, dev)) != NULL) {
-		if (of_device_is_compatible(dev, "pcm3052")) {
-			const u32 *addr;
-			printk(KERN_DEBUG PFX "found pcm3052\n");
-			addr = of_get_property(dev, "reg", NULL);
-			if (!addr)
-				return -ENODEV;
-			return onyx_create(adapter, dev, (*addr)>>1);
-		}
-	}
-
-	/* if that didn't work, try desperate mode for older
-	 * machines that have stuff missing from the device tree */
-
-	if (!of_device_is_compatible(busnode, "k2-i2c"))
-		return -ENODEV;
-
-	printk(KERN_DEBUG PFX "found k2-i2c, checking if onyx chip is on it\n");
-	/* probe both possible addresses for the onyx chip */
-	if (onyx_create(adapter, NULL, 0x46) == 0)
-		return 0;
-	return onyx_create(adapter, NULL, 0x47);
-}
-
-static int onyx_i2c_detach(struct i2c_client *client)
-{
-	struct onyx *onyx = container_of(client, struct onyx, i2c);
-	int err;
-
-	if ((err = i2c_detach_client(client)))
-		return err;
 	aoa_codec_unregister(&onyx->codec);
 	of_node_put(onyx->codec.node);
 	if (onyx->codec_info)
@@ -1095,13 +1049,20 @@ static int onyx_i2c_detach(struct i2c_cl
 	return 0;
 }
 
+static const struct i2c_device_id onyx_i2c_id[] = {
+	{ "aoa_codec_onyx", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, onyx_i2c_id);
+
 static struct i2c_driver onyx_driver = {
 	.driver = {
 		.name = "aoa_codec_onyx",
 		.owner = THIS_MODULE,
 	},
-	.attach_adapter = onyx_i2c_attach,
-	.detach_client = onyx_i2c_detach,
+	.probe = onyx_i2c_probe,
+	.remove = onyx_i2c_remove,
+	.id_table = onyx_i2c_id,
 };
 
 static int __init onyx_init(void)
--- linux-2.6.30-rc1.orig/sound/aoa/codecs/tas.c	2009-03-24 13:44:06.000000000 +0100
+++ linux-2.6.30-rc1/sound/aoa/codecs/tas.c	2009-04-08 13:50:22.000000000 +0200
@@ -82,7 +82,7 @@ MODULE_DESCRIPTION("tas codec driver for
 
 struct tas {
 	struct aoa_codec	codec;
-	struct i2c_client	i2c;
+	struct i2c_client	*i2c;
 	u32			mute_l:1, mute_r:1 ,
 				controls_created:1 ,
 				drc_enabled:1,
@@ -108,9 +108,9 @@ static struct tas *codec_to_tas(struct a
 static inline int tas_write_reg(struct tas *tas, u8 reg, u8 len, u8 *data)
 {
 	if (len == 1)
-		return i2c_smbus_write_byte_data(&tas->i2c, reg, *data);
+		return i2c_smbus_write_byte_data(tas->i2c, reg, *data);
 	else
-		return i2c_smbus_write_i2c_block_data(&tas->i2c, reg, len, data);
+		return i2c_smbus_write_i2c_block_data(tas->i2c, reg, len, data);
 }
 
 static void tas3004_set_drc(struct tas *tas)
@@ -884,10 +884,10 @@ static void tas_exit_codec(struct aoa_co
 
 static struct i2c_driver tas_driver;
 
-static int tas_create(struct i2c_adapter *adapter,
-		       struct device_node *node,
-		       int addr)
+static int tas_i2c_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
 {
+	struct device_node *node = client->dev.platform_data;
 	struct tas *tas;
 
 	tas = kzalloc(sizeof(struct tas), GFP_KERNEL);
@@ -896,17 +896,11 @@ static int tas_create(struct i2c_adapter
 		return -ENOMEM;
 
 	mutex_init(&tas->mtx);
-	tas->i2c.driver = &tas_driver;
-	tas->i2c.adapter = adapter;
-	tas->i2c.addr = addr;
+	tas->i2c = client;
+	i2c_set_clientdata(client, tas);
+
 	/* seems that half is a saner default */
 	tas->drc_range = TAS3004_DRC_MAX / 2;
-	strlcpy(tas->i2c.name, "tas audio codec", I2C_NAME_SIZE);
-
-	if (i2c_attach_client(&tas->i2c)) {
-		printk(KERN_ERR PFX "failed to attach to i2c\n");
-		goto fail;
-	}
 
 	strlcpy(tas->codec.name, "tas", MAX_CODEC_NAME_LEN);
 	tas->codec.owner = THIS_MODULE;
@@ -915,69 +909,23 @@ static int tas_create(struct i2c_adapter
 	tas->codec.node = of_node_get(node);
 
 	if (aoa_codec_register(&tas->codec)) {
-		goto detach;
+		goto fail;
 	}
 	printk(KERN_DEBUG
 	       "snd-aoa-codec-tas: tas found, addr 0x%02x on %s\n",
-	       addr, node->full_name);
+	       (unsigned int)client->addr, node->full_name);
 	return 0;
- detach:
-	i2c_detach_client(&tas->i2c);
  fail:
 	mutex_destroy(&tas->mtx);
 	kfree(tas);
 	return -EINVAL;
 }
 
-static int tas_i2c_attach(struct i2c_adapter *adapter)
-{
-	struct device_node *busnode, *dev = NULL;
-	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) {
-		if (of_device_is_compatible(dev, "tas3004")) {
-			const u32 *addr;
-			printk(KERN_DEBUG PFX "found tas3004\n");
-			addr = of_get_property(dev, "reg", NULL);
-			if (!addr)
-				continue;
-			return tas_create(adapter, dev, ((*addr) >> 1) & 0x7f);
-		}
-		/* older machines have no 'codec' node with a 'compatible'
-		 * property that says 'tas3004', they just have a 'deq'
-		 * node without any such property... */
-		if (strcmp(dev->name, "deq") == 0) {
-			const u32 *_addr;
-			u32 addr;
-			printk(KERN_DEBUG PFX "found 'deq' node\n");
-			_addr = of_get_property(dev, "i2c-address", NULL);
-			if (!_addr)
-				continue;
-			addr = ((*_addr) >> 1) & 0x7f;
-			/* now, if the address doesn't match any of the two
-			 * that a tas3004 can have, we cannot handle this.
-			 * I doubt it ever happens but hey. */
-			if (addr != 0x34 && addr != 0x35)
-				continue;
-			return tas_create(adapter, dev, addr);
-		}
-	}
-	return -ENODEV;
-}
-
-static int tas_i2c_detach(struct i2c_client *client)
+static int tas_i2c_remove(struct i2c_client *client)
 {
-	struct tas *tas = container_of(client, struct tas, i2c);
-	int err;
+	struct tas *tas = i2c_get_clientdata(client);
 	u8 tmp = TAS_ACR_ANALOG_PDOWN;
 
-	if ((err = i2c_detach_client(client)))
-		return err;
 	aoa_codec_unregister(&tas->codec);
 	of_node_put(tas->codec.node);
 
@@ -989,13 +937,20 @@ static int tas_i2c_detach(struct i2c_cli
 	return 0;
 }
 
+static const struct i2c_device_id tas_i2c_id[] = {
+	{ "aoa_codec_tas", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tas_i2c_id);
+
 static struct i2c_driver tas_driver = {
 	.driver = {
 		.name = "aoa_codec_tas",
 		.owner = THIS_MODULE,
 	},
-	.attach_adapter = tas_i2c_attach,
-	.detach_client = tas_i2c_detach,
+	.probe = tas_i2c_probe,
+	.remove = tas_i2c_remove,
+	.id_table = tas_i2c_id,
 };
 
 static int __init tas_init(void)


-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 4981 bytes --]

Hi!

> Basically the idea is to move the I2C device instantiation from the
> codec drivers (onyx, tas) to the I2C adapter driver (i2c-powermac.)
> This follows the Linux device driver model, requires slightly less
> code, runs faster and and lets the required chip drivers be loaded by
> udev or similar automatically.

Sounds weird -- how do you handle codecs that could be on different
buses? This seems to imply that any probing may potentially need to be
duplicated across any bus driver they could possibly connected to. But
that's not really relevant to this patch.

> The last driver to convert, keywest, is a mystery to me. I have a hard
> time understanding how it interacts with tumbler and daca. I have the
> feeling that it is essentially a glue module to workaround the legacy
> i2c model limitations, so probably it could go away entirely now, but
> I'm not sure how to do that in practice. Maybe tumbler and daca would
> be made separate i2c drivers, I'm not sure.

Sorry, I'm not familiar with this code.

> One thing in particular which I find strange is that tumbler has
> references to the TAS3004 device, much like the tas codec driver. It is
> unclear to me whether tas is a replacement for tumbler, or if both
> drivers work together, or if they are for separate hardware. I would
> appreciate clarifications about this.

Well... tumbler also drives the very similar tas3001 codec.

However, I need to start with a little more background.

Apple machines can contain various codecs, for example the tas3004,
which has various outputs. Due to the way Apple wires up the codec, you
need platform fabric to identify which outputs are connected where, cf.
sound/aoa/fabrics/layout.c. Note all machines have line-in for instance.

The aoa driver, which I wrote, currently supports only i2s buses for
actually transferring data, but I think there are some other ways to get
data to the codec -- I'm not too familiar with the old machines.

Now, aoa will currently automatically load from the layout fabric
module, and then pull in the modules for the codecs it _knows_ to be
present on the bus. Therefore, it seems that your patch makes things
less efficient because you probe for all those codecs, and there's no
machine that has all of them. The aoa fabric only loads the modules for
those it knows to be present, and they then probe (and in reality the
probing never fails because they really are there).

Now, since aoa needs information on how the entire system is glued
together (the fabric I was talking about with the line-in example), it
has to infer that from platform data, in this case the device tree.
Because I do not have any older machines, am lazy and snd-powermac works
for the old machines, snd-powermac with its "tumbler" is a driver for
the same tas3004 codec, but on a different, older, fabric.

Once upon a time the plan was to get rid of snd-powermac entirely and
port all its functionality into subdrivers of aoa, but that clearly
never happened. No fairy-tale happy ending here, quite obviously.

Now, looking at your patch, I think it will break snd-powermac. See, if
snd_aoa_codec_tas is automatically loaded on a system with an _old_
fabric that aoa knows nothing about, snd-powermac can no longer be
loaded. (Incidentally, snd-powermac cannot be auto-loaded at all
currently, while aoa can via the fabric driver's device-tree binding)

Therefore, probing the codecs in i2c-powermac and automatically loading
the corresponding aoa module will break sound on old machines.

I would think that if you removed the MODULE_DEVICE_TABLE from your
patch, it may continue to work because the aoa fabric driver loads the
modules as before, and on old machines nothing loads automatically and
snd-powermac can be loaded manually.

However, it will still be less efficient because you will be probing
_all_ those codecs, notably the tas family, even on machines that are
known to not have it (machines that have onyx). Putting that mutual
exclusion information into i2c-powermac would be misplaced, imho.

Note also that there's one more codec (topaz) which isn't currently
supported.

In conclusion, I think that the old/existing/legacy i2c binding model
was much better suited to platform knowledge about the way machines are
put together, and the new code is, as far as I can tell, less efficient
-- contrary to your assertion.


Since I'm away from all machines I could test this with I have no data
on any that or the module device table thing I pointed out for now.

Anyway, some more technical comments on your patch:
 * I realise you just copied things around but it would be nice to clean
   up the coding style, especially comment style, a little while at it.
   (yeah, it's my fault)

 * aoa_codec_* is the module name, I see no reason to use that as the
   i2c name, that should be the codec's name instead (aka pcm3052 etc)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

Hi Johannes,

Thanks for the fast answer.

On Wed, 08 Apr 2009 17:51:39 +0200, Johannes Berg wrote:
> > Basically the idea is to move the I2C device instantiation from the
> > codec drivers (onyx, tas) to the I2C adapter driver (i2c-powermac.)
> > This follows the Linux device driver model, requires slightly less
> > code, runs faster and and lets the required chip drivers be loaded by
> > udev or similar automatically.
> 
> Sounds weird -- how do you handle codecs that could be on different
> buses? This seems to imply that any probing may potentially need to be
> duplicated across any bus driver they could possibly connected to. But
> that's not really relevant to this patch.

Yes, "probing" would be duplicated if we had to support more than one
bus. But as far as I can see, the onyx and tas devices can only be
found on i2c-powermac buses, so this shouldn't be an issue. And there
isn't really any probing going on, it's really only a matter of walking
a small subset of the device tree (the children of the I2C bus) and
instantiating I2C devices.

Out of curiosity, are there systems with more than one system I2C buses
(supported by i2c-powermac)?

If there were a significant amount of duplicated code, it could
certainly be addressed one way or another, but it wasn't my top
priority, and actually didn't seem that necessary. As a matter of fact,
my patch removes (slightly) more code than it adds.

> > The last driver to convert, keywest, is a mystery to me. I have a hard
> > time understanding how it interacts with tumbler and daca. I have the
> > feeling that it is essentially a glue module to workaround the legacy
> > i2c model limitations, so probably it could go away entirely now, but
> > I'm not sure how to do that in practice. Maybe tumbler and daca would
> > be made separate i2c drivers, I'm not sure.
> 
> Sorry, I'm not familiar with this code.
> 
> > One thing in particular which I find strange is that tumbler has
> > references to the TAS3004 device, much like the tas codec driver. It is
> > unclear to me whether tas is a replacement for tumbler, or if both
> > drivers work together, or if they are for separate hardware. I would
> > appreciate clarifications about this.
> 
> Well... tumbler also drives the very similar tas3001 codec.
> 
> However, I need to start with a little more background.
> 
> Apple machines can contain various codecs, for example the tas3004,
> which has various outputs. Due to the way Apple wires up the codec, you
> need platform fabric to identify which outputs are connected where, cf.
> sound/aoa/fabrics/layout.c. Note all machines have line-in for instance.
> 
> The aoa driver, which I wrote, currently supports only i2s buses for
> actually transferring data, but I think there are some other ways to get
> data to the codec -- I'm not too familiar with the old machines.
> 
> Now, aoa will currently automatically load from the layout fabric
> module, and then pull in the modules for the codecs it _knows_ to be
> present on the bus. Therefore, it seems that your patch makes things
> less efficient because you probe for all those codecs, and there's no
> machine that has all of them. The aoa fabric only loads the modules for
> those it knows to be present, and they then probe (and in reality the
> probing never fails because they really are there).

Can you please point me to the layout fabric / aoa fabric? I'd like to
understand how it works. Then I may be able to rewrite my patch
completely differently.

There are basically two ways to instantiate I2C devices in the new
model. The first method is to declare the I2C devices as platform data
and let i2c-core instantiate them. The second method is to have the i2c
bus driver instantiate the clients. My patch uses the second method
because I didn't know how to use the first method. However using the
first method would solve the issues you raised. But I need some help
from someone more familiar with the powermac platform initialization
code to get it right.

> Now, since aoa needs information on how the entire system is glued
> together (the fabric I was talking about with the line-in example), it
> has to infer that from platform data, in this case the device tree.
> Because I do not have any older machines, am lazy and snd-powermac works
> for the old machines, snd-powermac with its "tumbler" is a driver for
> the same tas3004 codec, but on a different, older, fabric.

I think I've found that one by now (snd_pmac_detect in
sound/ppc/pmac.c, right?), thanks for the clarification.

BTW, that code doesn't seem significantly different from what my patch
is adding to i2c-powermac.

> Once upon a time the plan was to get rid of snd-powermac entirely and
> port all its functionality into subdrivers of aoa, but that clearly
> never happened. No fairy-tale happy ending here, quite obviously.
> 
> Now, looking at your patch, I think it will break snd-powermac. See, if
> snd_aoa_codec_tas is automatically loaded on a system with an _old_
> fabric that aoa knows nothing about, snd-powermac can no longer be
> loaded. (Incidentally, snd-powermac cannot be auto-loaded at all
> currently, while aoa can via the fabric driver's device-tree binding)

Hmm, OK, I expected the code I moved from the aoa drivers to
i2c-powermac to only match the subset of machines actually supported by
aoa. If that's not the case then indeed it is incorrect.

> Therefore, probing the codecs in i2c-powermac and automatically loading
> the corresponding aoa module will break sound on old machines.

Does this mean that manually loading snd_aoa_codec_tas today on an old
system with a TAS would break sound too?

> I would think that if you removed the MODULE_DEVICE_TABLE from your
> patch, it may continue to work because the aoa fabric driver loads the
> modules as before, and on old machines nothing loads automatically and
> snd-powermac can be loaded manually.
> 
> However, it will still be less efficient because you will be probing
> _all_ those codecs, notably the tas family, even on machines that are
> known to not have it (machines that have onyx). Putting that mutual
> exclusion information into i2c-powermac would be misplaced, imho.
> 
> Note also that there's one more codec (topaz) which isn't currently
> supported.
> 
> In conclusion, I think that the old/existing/legacy i2c binding model
> was much better suited to platform knowledge about the way machines are
> put together, and the new code is, as far as I can tell, less efficient
> -- contrary to your assertion.

My comment about efficiency was related to the fact that the sound chip
detection code would run only on the i2c-power mac bus, rather than all
I2C buses on the system. Admittedly I had missed the larger picture of
all detections running on all powermac systems, which wasn't the case
so far.

Anyway, the key point of my patch is to get rid of the legacy i2c
binding model, not efficiency.

> Since I'm away from all machines I could test this with I have no data
> on any that or the module device table thing I pointed out for now.
> 
> Anyway, some more technical comments on your patch:
>  * I realise you just copied things around but it would be nice to clean
>    up the coding style, especially comment style, a little while at it.
>    (yeah, it's my fault)

I can fix anything you like, just tell me what :)

>  * aoa_codec_* is the module name, I see no reason to use that as the
>    i2c name, that should be the codec's name instead (aka pcm3052 etc)

The names are totally arbitrary and we can change them if you like.
Keep in mind though that we should avoid using mere chip names for
non-generic drivers. The aoa drivers are powermac-specific, we don't
want the names we pick to collide with another driver, that's why I
chose to keep the aoa prefix.

-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 5101 bytes --]

Hi Jean!

> Yes, "probing" would be duplicated if we had to support more than one
> bus. But as far as I can see, the onyx and tas devices can only be
> found on i2c-powermac buses, so this shouldn't be an issue. And there
> isn't really any probing going on, it's really only a matter of walking
> a small subset of the device tree (the children of the I2C bus) and
> instantiating I2C devices.

Right, it was just an unrelated thought, it's not related to this in
particular.

> > Now, aoa will currently automatically load from the layout fabric
> > module, and then pull in the modules for the codecs it _knows_ to be
> > present on the bus. Therefore, it seems that your patch makes things
> > less efficient because you probe for all those codecs, and there's no
> > machine that has all of them. The aoa fabric only loads the modules for
> > those it knows to be present, and they then probe (and in reality the
> > probing never fails because they really are there).
> 
> Can you please point me to the layout fabric / aoa fabric? I'd like to
> understand how it works. Then I may be able to rewrite my patch
> completely differently.

That's in sound/aoa/fabric/layout.c

> There are basically two ways to instantiate I2C devices in the new
> model. The first method is to declare the I2C devices as platform data
> and let i2c-core instantiate them. The second method is to have the i2c
> bus driver instantiate the clients. My patch uses the second method
> because I didn't know how to use the first method. However using the
> first method would solve the issues you raised. But I need some help
> from someone more familiar with the powermac platform initialization
> code to get it right.

Interesting. Does it need to be _platform_ data, or could
sound/aoa/fabric/layout.c declare the devices instead?

> > Now, since aoa needs information on how the entire system is glued
> > together (the fabric I was talking about with the line-in example), it
> > has to infer that from platform data, in this case the device tree.
> > Because I do not have any older machines, am lazy and snd-powermac works
> > for the old machines, snd-powermac with its "tumbler" is a driver for
> > the same tas3004 codec, but on a different, older, fabric.
> 
> I think I've found that one by now (snd_pmac_detect in
> sound/ppc/pmac.c, right?), thanks for the clarification.

That's snd-powermac, yes.

> BTW, that code doesn't seem significantly different from what my patch
> is adding to i2c-powermac.

That would be true, yes, with your patch I don't understand how to load
snd-powermac _or_ snd-aoa based on the platform data (or in the case of
snd-powermac, not load it automatically at all).

> Hmm, OK, I expected the code I moved from the aoa drivers to
> i2c-powermac to only match the subset of machines actually supported by
> aoa. If that's not the case then indeed it is incorrect.

Yeah, that's not the case, I think the 'deq' node will be present on
older machines as well and you would load snd-aoa-codec-tas where
snd-powermac should be loaded.

> > Therefore, probing the codecs in i2c-powermac and automatically loading
> > the corresponding aoa module will break sound on old machines.
> 
> Does this mean that manually loading snd_aoa_codec_tas today on an old
> system with a TAS would break sound too?

Yes, I'm pretty sure it does on some systems. Actually, I'm not entirely
sure, because I don't know whether the i2c core will prevent two drivers
from talking to the same chip on the same bus, and if it doesn't then
this might still work but it would at least be very strange wrt. suspend
resume.

> Anyway, the key point of my patch is to get rid of the legacy i2c
> binding model, not efficiency.

Yes, I understand :)

> > Since I'm away from all machines I could test this with I have no data
> > on any that or the module device table thing I pointed out for now.
> > 
> > Anyway, some more technical comments on your patch:
> >  * I realise you just copied things around but it would be nice to clean
> >    up the coding style, especially comment style, a little while at it.
> >    (yeah, it's my fault)
> 
> I can fix anything you like, just tell me what :)

I think CodingStyle wants
 /*
  * ...
  * ...
  */

> >  * aoa_codec_* is the module name, I see no reason to use that as the
> >    i2c name, that should be the codec's name instead (aka pcm3052 etc)
> 
> The names are totally arbitrary and we can change them if you like.
> Keep in mind though that we should avoid using mere chip names for
> non-generic drivers. The aoa drivers are powermac-specific, we don't
> want the names we pick to collide with another driver, that's why I
> chose to keep the aoa prefix.

Oh ok, that kinda ties in with my very first question about what happens
if the same chip is known in different contexts, on different buses etc.
Makes sense in that case I guess. But I still think that you shouldn't
auto-load the aoa codec modules based on this anyway.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

Hi Johannes,

On Thu, 09 Apr 2009 09:44:49 +0200, Johannes Berg wrote:
> > Can you please point me to the layout fabric / aoa fabric? I'd like to
> > understand how it works. Then I may be able to rewrite my patch
> > completely differently.
> 
> That's in sound/aoa/fabric/layout.c

OK, I understand how it works now, thanks for pointing me to the
relevant piece of code. It's easier to modify the code when you
understand the current logic ;)

> > There are basically two ways to instantiate I2C devices in the new
> > model. The first method is to declare the I2C devices as platform data
> > and let i2c-core instantiate them. The second method is to have the i2c
> > bus driver instantiate the clients. My patch uses the second method
> > because I didn't know how to use the first method. However using the
> > first method would solve the issues you raised. But I need some help
> > from someone more familiar with the powermac platform initialization
> > code to get it right.
> 
> Interesting. Does it need to be _platform_ data, or could
> sound/aoa/fabric/layout.c declare the devices instead?

It can be anywhere as long as it is early enough. Early enough means
that the code must run before device drivers can loaded. As
snd-aoa-fabric-layout can be built as a module, it doesn't seem right.

The idea is to assign fixed i2c bus numbers to the relevant i2c buses,
and declare the i2c devices connected to each bus by bus number and
device address. The i2c-powermac buses are created in
arch/powerpc/platforms/powermac/low_i2c.c, so you would have to
instantiate the i2c devices there too. That would basically mean
merging part of snd-aoa-fabric-layout into
arch/powerpc/platforms/powermac/low_i2c.c as I understand it, I don't
know if this sounds reasonable to you.

More generally, in order to instantiate an i2c device (for the new
binding model), you need at least two things: a reference to the
underlying i2c bus, and the address of the i2c device. The address can
be read from the device tree, but what is more difficult is to get the
reference to the underlying i2c bus. That reference can be the
i2c_adapter number (which only makes sense in a fixed-bus-number
scheme), or a pointer to the i2c_adapter. For now the powermac systems
do not have fixed i2c bus numbers, which is why I put the code
instantiating the devices in i2c-powermac: there I have a pointer to
the i2c_adapter.

So I think we have two options: switch the powermac systems to fixed
i2c bus numbers and instantiate the i2c sound devices from
arch/powerpc/platforms/powermac/*, or find a way to obtain a reference
to the relevant i2c_adapter.

I think I have found a way to achieve the latter. This isn't exactly
how the new model was supposed to work, but it has the advantage to be
way less intrusive than my original proposal. It may require larger
changes in the future as the i2c-core cleanups go on, but this should
do for now.

> > (...)
> > I think I've found that one by now (snd_pmac_detect in
> > sound/ppc/pmac.c, right?), thanks for the clarification.
> 
> That's snd-powermac, yes.
> 
> > BTW, that code doesn't seem significantly different from what my patch
> > is adding to i2c-powermac.
> 
> That would be true, yes, with your patch I don't understand how to load
> snd-powermac _or_ snd-aoa based on the platform data (or in the case of
> snd-powermac, not load it automatically at all).

My patch was really only about snd-aoa, it did not take care about
snd-powermac at all, because I expected them to be essentially
independent. Of course, if the code I added to i2c-powermac
instantiates devices on systems which were previous handled by
snd-powermac, it would break. More work would be needed to handle the
systems supported by snd-powermac.

> (...)
> Oh ok, that kinda ties in with my very first question about what happens
> if the same chip is known in different contexts, on different buses etc.
> Makes sense in that case I guess. But I still think that you shouldn't
> auto-load the aoa codec modules based on this anyway.

My new approach doesn't auto-load anything. Here we go, what you think?
This time there is no logic change, I'm really only turning legacy code
into new-style i2c code (basically calling i2c_new_device() instead of
i2c_attach_client()) and that's about it.

(Once again this is only build-tested and I would like people with the
hardware to give it a try.)

From: Jean Delvare <khali@linux-fr.org>
Subject: AOA: Convert onyx and tas codecs to new-style i2c drivers

The legacy i2c binding model is going away soon, so convert the AOA
codec drivers to the new model or they'll break.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 sound/aoa/codecs/onyx.c |   71 +++++++++++++++++++++++++++++------------------
 sound/aoa/codecs/tas.c  |   63 +++++++++++++++++++++++++----------------
 2 files changed, 84 insertions(+), 50 deletions(-)

--- linux-2.6.30-rc1.orig/sound/aoa/codecs/onyx.c	2009-04-09 11:53:11.000000000 +0200
+++ linux-2.6.30-rc1/sound/aoa/codecs/onyx.c	2009-04-09 13:58:44.000000000 +0200
@@ -47,7 +47,7 @@ MODULE_DESCRIPTION("pcm3052 (onyx) codec
 struct onyx {
 	/* cache registers 65 to 80, they are write-only! */
 	u8			cache[16];
-	struct i2c_client	i2c;
+	struct i2c_client	*i2c;
 	struct aoa_codec	codec;
 	u32			initialised:1,
 				spdif_locked:1,
@@ -72,7 +72,7 @@ static int onyx_read_register(struct ony
 		*value = onyx->cache[reg-FIRSTREGISTER];
 		return 0;
 	}
-	v = i2c_smbus_read_byte_data(&onyx->i2c, reg);
+	v = i2c_smbus_read_byte_data(onyx->i2c, reg);
 	if (v < 0)
 		return -1;
 	*value = (u8)v;
@@ -84,7 +84,7 @@ static int onyx_write_register(struct on
 {
 	int result;
 
-	result = i2c_smbus_write_byte_data(&onyx->i2c, reg, value);
+	result = i2c_smbus_write_byte_data(onyx->i2c, reg, value);
 	if (!result)
 		onyx->cache[reg-FIRSTREGISTER] = value;
 	return result;
@@ -996,12 +996,38 @@ static void onyx_exit_codec(struct aoa_c
 	onyx->codec.soundbus_dev->detach_codec(onyx->codec.soundbus_dev, onyx);
 }
 
-static struct i2c_driver onyx_driver;
-
 static int onyx_create(struct i2c_adapter *adapter,
 		       struct device_node *node,
 		       int addr)
 {
+	struct i2c_board_info info;
+	struct i2c_client *client;
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "aoa_codec_onyx", I2C_NAME_SIZE);
+	if (node) {
+		info.addr = addr;
+		info.platform_data = node;
+		client = i2c_new_device(adapter, &info);
+	} else {
+		/* probe both possible addresses for the onyx chip */
+		unsigned short probe_onyx[] = {
+			0x46, 0x47, I2C_CLIENT_END
+		};
+
+		client = i2c_new_probed_device(adapter, &info, probe_onyx);
+	}
+	if (!client)
+		return -ENODEV;
+
+	list_add_tail(&client->detected, &client->driver->clients);
+	return 0;
+}
+
+static int onyx_i2c_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct device_node *node = client->dev.platform_data;
 	struct onyx *onyx;
 	u8 dummy;
 
@@ -1011,20 +1037,12 @@ static int onyx_create(struct i2c_adapte
 		return -ENOMEM;
 
 	mutex_init(&onyx->mutex);
-	onyx->i2c.driver = &onyx_driver;
-	onyx->i2c.adapter = adapter;
-	onyx->i2c.addr = addr & 0x7f;
-	strlcpy(onyx->i2c.name, "onyx audio codec", I2C_NAME_SIZE);
-
-	if (i2c_attach_client(&onyx->i2c)) {
-		printk(KERN_ERR PFX "failed to attach to i2c\n");
-		goto fail;
-	}
+	onyx->i2c = client;
+	i2c_set_clientdata(client, onyx);
 
 	/* we try to read from register ONYX_REG_CONTROL
 	 * to check if the codec is present */
 	if (onyx_read_register(onyx, ONYX_REG_CONTROL, &dummy) != 0) {
-		i2c_detach_client(&onyx->i2c);
 		printk(KERN_ERR PFX "failed to read control register\n");
 		goto fail;
 	}
@@ -1036,7 +1054,6 @@ static int onyx_create(struct i2c_adapte
 	onyx->codec.node = of_node_get(node);
 
 	if (aoa_codec_register(&onyx->codec)) {
-		i2c_detach_client(&onyx->i2c);
 		goto fail;
 	}
 	printk(KERN_DEBUG PFX "created and attached onyx instance\n");
@@ -1074,19 +1091,13 @@ static int onyx_i2c_attach(struct i2c_ad
 		return -ENODEV;
 
 	printk(KERN_DEBUG PFX "found k2-i2c, checking if onyx chip is on it\n");
-	/* probe both possible addresses for the onyx chip */
-	if (onyx_create(adapter, NULL, 0x46) == 0)
-		return 0;
-	return onyx_create(adapter, NULL, 0x47);
+	return onyx_create(adapter, NULL, 0);
 }
 
-static int onyx_i2c_detach(struct i2c_client *client)
+static int onyx_i2c_remove(struct i2c_client *client)
 {
-	struct onyx *onyx = container_of(client, struct onyx, i2c);
-	int err;
+	struct onyx *onyx = i2c_get_clientdata(client);
 
-	if ((err = i2c_detach_client(client)))
-		return err;
 	aoa_codec_unregister(&onyx->codec);
 	of_node_put(onyx->codec.node);
 	if (onyx->codec_info)
@@ -1095,13 +1106,21 @@ static int onyx_i2c_detach(struct i2c_cl
 	return 0;
 }
 
+static const struct i2c_device_id onyx_i2c_id[] = {
+	{ "aoa_codec_onyx", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, onyx_i2c_id);
+
 static struct i2c_driver onyx_driver = {
 	.driver = {
 		.name = "aoa_codec_onyx",
 		.owner = THIS_MODULE,
 	},
 	.attach_adapter = onyx_i2c_attach,
-	.detach_client = onyx_i2c_detach,
+	.probe = onyx_i2c_probe,
+	.remove = onyx_i2c_remove,
+	.id_table = onyx_i2c_id,
 };
 
 static int __init onyx_init(void)
--- linux-2.6.30-rc1.orig/sound/aoa/codecs/tas.c	2009-04-09 11:53:11.000000000 +0200
+++ linux-2.6.30-rc1/sound/aoa/codecs/tas.c	2009-04-09 13:58:41.000000000 +0200
@@ -82,7 +82,7 @@ MODULE_DESCRIPTION("tas codec driver for
 
 struct tas {
 	struct aoa_codec	codec;
-	struct i2c_client	i2c;
+	struct i2c_client	*i2c;
 	u32			mute_l:1, mute_r:1 ,
 				controls_created:1 ,
 				drc_enabled:1,
@@ -108,9 +108,9 @@ static struct tas *codec_to_tas(struct a
 static inline int tas_write_reg(struct tas *tas, u8 reg, u8 len, u8 *data)
 {
 	if (len == 1)
-		return i2c_smbus_write_byte_data(&tas->i2c, reg, *data);
+		return i2c_smbus_write_byte_data(tas->i2c, reg, *data);
 	else
-		return i2c_smbus_write_i2c_block_data(&tas->i2c, reg, len, data);
+		return i2c_smbus_write_i2c_block_data(tas->i2c, reg, len, data);
 }
 
 static void tas3004_set_drc(struct tas *tas)
@@ -882,12 +882,30 @@ static void tas_exit_codec(struct aoa_co
 }
 
 
-static struct i2c_driver tas_driver;
-
 static int tas_create(struct i2c_adapter *adapter,
 		       struct device_node *node,
 		       int addr)
 {
+	struct i2c_board_info info;
+	struct i2c_client *client;
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	strlcpy(info.type, "aoa_codec_tas", I2C_NAME_SIZE);
+	info.addr = addr;
+	info.platform_data = node;
+
+	client = i2c_new_device(adapter, &info);
+	if (!client)
+		return -ENODEV;
+
+	list_add_tail(&client->detected, &client->driver->clients);
+	return 0;
+}
+
+static int tas_i2c_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device_node *node = client->dev.platform_data;
 	struct tas *tas;
 
 	tas = kzalloc(sizeof(struct tas), GFP_KERNEL);
@@ -896,17 +914,11 @@ static int tas_create(struct i2c_adapter
 		return -ENOMEM;
 
 	mutex_init(&tas->mtx);
-	tas->i2c.driver = &tas_driver;
-	tas->i2c.adapter = adapter;
-	tas->i2c.addr = addr;
+	tas->i2c = client;
+	i2c_set_clientdata(client, tas);
+
 	/* seems that half is a saner default */
 	tas->drc_range = TAS3004_DRC_MAX / 2;
-	strlcpy(tas->i2c.name, "tas audio codec", I2C_NAME_SIZE);
-
-	if (i2c_attach_client(&tas->i2c)) {
-		printk(KERN_ERR PFX "failed to attach to i2c\n");
-		goto fail;
-	}
 
 	strlcpy(tas->codec.name, "tas", MAX_CODEC_NAME_LEN);
 	tas->codec.owner = THIS_MODULE;
@@ -915,14 +927,12 @@ static int tas_create(struct i2c_adapter
 	tas->codec.node = of_node_get(node);
 
 	if (aoa_codec_register(&tas->codec)) {
-		goto detach;
+		goto fail;
 	}
 	printk(KERN_DEBUG
 	       "snd-aoa-codec-tas: tas found, addr 0x%02x on %s\n",
-	       addr, node->full_name);
+	       (unsigned int)client->addr, node->full_name);
 	return 0;
- detach:
-	i2c_detach_client(&tas->i2c);
  fail:
 	mutex_destroy(&tas->mtx);
 	kfree(tas);
@@ -970,14 +980,11 @@ static int tas_i2c_attach(struct i2c_ada
 	return -ENODEV;
 }
 
-static int tas_i2c_detach(struct i2c_client *client)
+static int tas_i2c_remove(struct i2c_client *client)
 {
-	struct tas *tas = container_of(client, struct tas, i2c);
-	int err;
+	struct tas *tas = i2c_get_clientdata(client);
 	u8 tmp = TAS_ACR_ANALOG_PDOWN;
 
-	if ((err = i2c_detach_client(client)))
-		return err;
 	aoa_codec_unregister(&tas->codec);
 	of_node_put(tas->codec.node);
 
@@ -989,13 +996,21 @@ static int tas_i2c_detach(struct i2c_cli
 	return 0;
 }
 
+static const struct i2c_device_id tas_i2c_id[] = {
+	{ "aoa_codec_tas", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tas_i2c_id);
+
 static struct i2c_driver tas_driver = {
 	.driver = {
 		.name = "aoa_codec_tas",
 		.owner = THIS_MODULE,
 	},
 	.attach_adapter = tas_i2c_attach,
-	.detach_client = tas_i2c_detach,
+	.probe = tas_i2c_probe,
+	.remove = tas_i2c_remove,
+	.id_table = tas_i2c_id,
 };
 
 static int __init tas_init(void)


-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2862 bytes --]

Hi Jean,

> OK, I understand how it works now, thanks for pointing me to the
> relevant piece of code. It's easier to modify the code when you
> understand the current logic ;)

:)

> The idea is to assign fixed i2c bus numbers to the relevant i2c buses,
> and declare the i2c devices connected to each bus by bus number and
> device address. The i2c-powermac buses are created in
> arch/powerpc/platforms/powermac/low_i2c.c, so you would have to
> instantiate the i2c devices there too. That would basically mean
> merging part of snd-aoa-fabric-layout into
> arch/powerpc/platforms/powermac/low_i2c.c as I understand it, I don't
> know if this sounds reasonable to you.

That doesn't sound too hot -- the fabric module is quite a lot of code
and data.

> So I think we have two options: switch the powermac systems to fixed
> i2c bus numbers and instantiate the i2c sound devices from
> arch/powerpc/platforms/powermac/*, or find a way to obtain a reference
> to the relevant i2c_adapter.
> 
> I think I have found a way to achieve the latter. This isn't exactly
> how the new model was supposed to work, but it has the advantage to be
> way less intrusive than my original proposal. It may require larger
> changes in the future as the i2c-core cleanups go on, but this should
> do for now.

Heh :)

> My new approach doesn't auto-load anything. Here we go, what you think?
> This time there is no logic change, I'm really only turning legacy code
> into new-style i2c code (basically calling i2c_new_device() instead of
> i2c_attach_client()) and that's about it.
> 
> (Once again this is only build-tested and I would like people with the
> hardware to give it a try.)

Looks reasonable.

>  static int onyx_create(struct i2c_adapter *adapter,
>  		       struct device_node *node,
>  		       int addr)
>  {
> +	struct i2c_board_info info;
> +	struct i2c_client *client;
> +
> +	memset(&info, 0, sizeof(struct i2c_board_info));
> +	strlcpy(info.type, "aoa_codec_onyx", I2C_NAME_SIZE);
> +	if (node) {
> +		info.addr = addr;
> +		info.platform_data = node;
> +		client = i2c_new_device(adapter, &info);
> +	} else {
> +		/* probe both possible addresses for the onyx chip */
> +		unsigned short probe_onyx[] = {
> +			0x46, 0x47, I2C_CLIENT_END
> +		};
> +
> +		client = i2c_new_probed_device(adapter, &info, probe_onyx);
> +	}
> +	if (!client)
> +		return -ENODEV;
> +
> +	list_add_tail(&client->detected, &client->driver->clients);

That list_add looks a little hackish, and wouldn't it be racy?

> +static const struct i2c_device_id onyx_i2c_id[] = {
> +	{ "aoa_codec_onyx", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, onyx_i2c_id);

Should it really export the device table?

(same comments for tas)

It'll be until mid next week that I can test anything.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

Hi Johannes,

On Thu, 09 Apr 2009 14:34:44 +0200, Johannes Berg wrote:
> > My new approach doesn't auto-load anything. Here we go, what you think?
> > This time there is no logic change, I'm really only turning legacy code
> > into new-style i2c code (basically calling i2c_new_device() instead of
> > i2c_attach_client()) and that's about it.
> > 
> > (Once again this is only build-tested and I would like people with the
> > hardware to give it a try.)
> 
> Looks reasonable.
> 
> >  static int onyx_create(struct i2c_adapter *adapter,
> >  		       struct device_node *node,
> >  		       int addr)
> >  {
> > +	struct i2c_board_info info;
> > +	struct i2c_client *client;
> > +
> > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > +	strlcpy(info.type, "aoa_codec_onyx", I2C_NAME_SIZE);
> > +	if (node) {
> > +		info.addr = addr;
> > +		info.platform_data = node;
> > +		client = i2c_new_device(adapter, &info);
> > +	} else {
> > +		/* probe both possible addresses for the onyx chip */
> > +		unsigned short probe_onyx[] = {
> > +			0x46, 0x47, I2C_CLIENT_END
> > +		};
> > +
> > +		client = i2c_new_probed_device(adapter, &info, probe_onyx);
> > +	}
> > +	if (!client)
> > +		return -ENODEV;
> > +
> > +	list_add_tail(&client->detected, &client->driver->clients);
> 
> That list_add looks a little hackish, and wouldn't it be racy?

Yes it is a little hackish, the idea is to let i2c-core remove the
device if our i2c_driver is removed (which happens when you rmmod the
module). I'll add a comment to explain that. If there are more use
cases I might move that to a helper function in i2c-core.

No it's not racy, we're called with i2c-core's main mutex held.

> > +static const struct i2c_device_id onyx_i2c_id[] = {
> > +	{ "aoa_codec_onyx", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, onyx_i2c_id);
> 
> Should it really export the device table?
> 
> (same comments for tas)

No, that's a leftover, I intended to remove it but forgot. It's gone
now. That being said, it's useless, but I don't think it would hurt.

> It'll be until mid next week that I can test anything.

OK, thanks.

-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

On Thu, 9 Apr 2009 14:19:45 +0200, Jean Delvare wrote:
> From: Jean Delvare <khali@linux-fr.org>
> Subject: AOA: Convert onyx and tas codecs to new-style i2c drivers
> 
> The legacy i2c binding model is going away soon, so convert the AOA
> codec drivers to the new model or they'll break.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  sound/aoa/codecs/onyx.c |   71 +++++++++++++++++++++++++++++------------------
>  sound/aoa/codecs/tas.c  |   63 +++++++++++++++++++++++++----------------
>  2 files changed, 84 insertions(+), 50 deletions(-)

Note to potential testers of this patch: you need to apply this i2c
patch first:

ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-loosen-driver-check.patch

> 
> --- linux-2.6.30-rc1.orig/sound/aoa/codecs/onyx.c	2009-04-09 11:53:11.000000000 +0200
> +++ linux-2.6.30-rc1/sound/aoa/codecs/onyx.c	2009-04-09 13:58:44.000000000 +0200
> @@ -47,7 +47,7 @@ MODULE_DESCRIPTION("pcm3052 (onyx) codec
>  struct onyx {
>  	/* cache registers 65 to 80, they are write-only! */
>  	u8			cache[16];
> -	struct i2c_client	i2c;
> +	struct i2c_client	*i2c;
>  	struct aoa_codec	codec;
>  	u32			initialised:1,
>  				spdif_locked:1,
> @@ -72,7 +72,7 @@ static int onyx_read_register(struct ony
>  		*value = onyx->cache[reg-FIRSTREGISTER];
>  		return 0;
>  	}
> -	v = i2c_smbus_read_byte_data(&onyx->i2c, reg);
> +	v = i2c_smbus_read_byte_data(onyx->i2c, reg);
>  	if (v < 0)
>  		return -1;
>  	*value = (u8)v;
> @@ -84,7 +84,7 @@ static int onyx_write_register(struct on
>  {
>  	int result;
>  
> -	result = i2c_smbus_write_byte_data(&onyx->i2c, reg, value);
> +	result = i2c_smbus_write_byte_data(onyx->i2c, reg, value);
>  	if (!result)
>  		onyx->cache[reg-FIRSTREGISTER] = value;
>  	return result;
> @@ -996,12 +996,38 @@ static void onyx_exit_codec(struct aoa_c
>  	onyx->codec.soundbus_dev->detach_codec(onyx->codec.soundbus_dev, onyx);
>  }
>  
> -static struct i2c_driver onyx_driver;
> -
>  static int onyx_create(struct i2c_adapter *adapter,
>  		       struct device_node *node,
>  		       int addr)
>  {
> +	struct i2c_board_info info;
> +	struct i2c_client *client;
> +
> +	memset(&info, 0, sizeof(struct i2c_board_info));
> +	strlcpy(info.type, "aoa_codec_onyx", I2C_NAME_SIZE);
> +	if (node) {
> +		info.addr = addr;
> +		info.platform_data = node;
> +		client = i2c_new_device(adapter, &info);
> +	} else {
> +		/* probe both possible addresses for the onyx chip */
> +		unsigned short probe_onyx[] = {
> +			0x46, 0x47, I2C_CLIENT_END
> +		};
> +
> +		client = i2c_new_probed_device(adapter, &info, probe_onyx);
> +	}
> +	if (!client)
> +		return -ENODEV;
> +
> +	list_add_tail(&client->detected, &client->driver->clients);
> +	return 0;
> +}
> +
> +static int onyx_i2c_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct device_node *node = client->dev.platform_data;
>  	struct onyx *onyx;
>  	u8 dummy;
>  
> @@ -1011,20 +1037,12 @@ static int onyx_create(struct i2c_adapte
>  		return -ENOMEM;
>  
>  	mutex_init(&onyx->mutex);
> -	onyx->i2c.driver = &onyx_driver;
> -	onyx->i2c.adapter = adapter;
> -	onyx->i2c.addr = addr & 0x7f;
> -	strlcpy(onyx->i2c.name, "onyx audio codec", I2C_NAME_SIZE);
> -
> -	if (i2c_attach_client(&onyx->i2c)) {
> -		printk(KERN_ERR PFX "failed to attach to i2c\n");
> -		goto fail;
> -	}
> +	onyx->i2c = client;
> +	i2c_set_clientdata(client, onyx);
>  
>  	/* we try to read from register ONYX_REG_CONTROL
>  	 * to check if the codec is present */
>  	if (onyx_read_register(onyx, ONYX_REG_CONTROL, &dummy) != 0) {
> -		i2c_detach_client(&onyx->i2c);
>  		printk(KERN_ERR PFX "failed to read control register\n");
>  		goto fail;
>  	}
> @@ -1036,7 +1054,6 @@ static int onyx_create(struct i2c_adapte
>  	onyx->codec.node = of_node_get(node);
>  
>  	if (aoa_codec_register(&onyx->codec)) {
> -		i2c_detach_client(&onyx->i2c);
>  		goto fail;
>  	}
>  	printk(KERN_DEBUG PFX "created and attached onyx instance\n");
> @@ -1074,19 +1091,13 @@ static int onyx_i2c_attach(struct i2c_ad
>  		return -ENODEV;
>  
>  	printk(KERN_DEBUG PFX "found k2-i2c, checking if onyx chip is on it\n");
> -	/* probe both possible addresses for the onyx chip */
> -	if (onyx_create(adapter, NULL, 0x46) == 0)
> -		return 0;
> -	return onyx_create(adapter, NULL, 0x47);
> +	return onyx_create(adapter, NULL, 0);
>  }
>  
> -static int onyx_i2c_detach(struct i2c_client *client)
> +static int onyx_i2c_remove(struct i2c_client *client)
>  {
> -	struct onyx *onyx = container_of(client, struct onyx, i2c);
> -	int err;
> +	struct onyx *onyx = i2c_get_clientdata(client);
>  
> -	if ((err = i2c_detach_client(client)))
> -		return err;
>  	aoa_codec_unregister(&onyx->codec);
>  	of_node_put(onyx->codec.node);
>  	if (onyx->codec_info)
> @@ -1095,13 +1106,21 @@ static int onyx_i2c_detach(struct i2c_cl
>  	return 0;
>  }
>  
> +static const struct i2c_device_id onyx_i2c_id[] = {
> +	{ "aoa_codec_onyx", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, onyx_i2c_id);
> +
>  static struct i2c_driver onyx_driver = {
>  	.driver = {
>  		.name = "aoa_codec_onyx",
>  		.owner = THIS_MODULE,
>  	},
>  	.attach_adapter = onyx_i2c_attach,
> -	.detach_client = onyx_i2c_detach,
> +	.probe = onyx_i2c_probe,
> +	.remove = onyx_i2c_remove,
> +	.id_table = onyx_i2c_id,
>  };
>  
>  static int __init onyx_init(void)
> --- linux-2.6.30-rc1.orig/sound/aoa/codecs/tas.c	2009-04-09 11:53:11.000000000 +0200
> +++ linux-2.6.30-rc1/sound/aoa/codecs/tas.c	2009-04-09 13:58:41.000000000 +0200
> @@ -82,7 +82,7 @@ MODULE_DESCRIPTION("tas codec driver for
>  
>  struct tas {
>  	struct aoa_codec	codec;
> -	struct i2c_client	i2c;
> +	struct i2c_client	*i2c;
>  	u32			mute_l:1, mute_r:1 ,
>  				controls_created:1 ,
>  				drc_enabled:1,
> @@ -108,9 +108,9 @@ static struct tas *codec_to_tas(struct a
>  static inline int tas_write_reg(struct tas *tas, u8 reg, u8 len, u8 *data)
>  {
>  	if (len == 1)
> -		return i2c_smbus_write_byte_data(&tas->i2c, reg, *data);
> +		return i2c_smbus_write_byte_data(tas->i2c, reg, *data);
>  	else
> -		return i2c_smbus_write_i2c_block_data(&tas->i2c, reg, len, data);
> +		return i2c_smbus_write_i2c_block_data(tas->i2c, reg, len, data);
>  }
>  
>  static void tas3004_set_drc(struct tas *tas)
> @@ -882,12 +882,30 @@ static void tas_exit_codec(struct aoa_co
>  }
>  
>  
> -static struct i2c_driver tas_driver;
> -
>  static int tas_create(struct i2c_adapter *adapter,
>  		       struct device_node *node,
>  		       int addr)
>  {
> +	struct i2c_board_info info;
> +	struct i2c_client *client;
> +
> +	memset(&info, 0, sizeof(struct i2c_board_info));
> +	strlcpy(info.type, "aoa_codec_tas", I2C_NAME_SIZE);
> +	info.addr = addr;
> +	info.platform_data = node;
> +
> +	client = i2c_new_device(adapter, &info);
> +	if (!client)
> +		return -ENODEV;
> +
> +	list_add_tail(&client->detected, &client->driver->clients);
> +	return 0;
> +}
> +
> +static int tas_i2c_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct device_node *node = client->dev.platform_data;
>  	struct tas *tas;
>  
>  	tas = kzalloc(sizeof(struct tas), GFP_KERNEL);
> @@ -896,17 +914,11 @@ static int tas_create(struct i2c_adapter
>  		return -ENOMEM;
>  
>  	mutex_init(&tas->mtx);
> -	tas->i2c.driver = &tas_driver;
> -	tas->i2c.adapter = adapter;
> -	tas->i2c.addr = addr;
> +	tas->i2c = client;
> +	i2c_set_clientdata(client, tas);
> +
>  	/* seems that half is a saner default */
>  	tas->drc_range = TAS3004_DRC_MAX / 2;
> -	strlcpy(tas->i2c.name, "tas audio codec", I2C_NAME_SIZE);
> -
> -	if (i2c_attach_client(&tas->i2c)) {
> -		printk(KERN_ERR PFX "failed to attach to i2c\n");
> -		goto fail;
> -	}
>  
>  	strlcpy(tas->codec.name, "tas", MAX_CODEC_NAME_LEN);
>  	tas->codec.owner = THIS_MODULE;
> @@ -915,14 +927,12 @@ static int tas_create(struct i2c_adapter
>  	tas->codec.node = of_node_get(node);
>  
>  	if (aoa_codec_register(&tas->codec)) {
> -		goto detach;
> +		goto fail;
>  	}
>  	printk(KERN_DEBUG
>  	       "snd-aoa-codec-tas: tas found, addr 0x%02x on %s\n",
> -	       addr, node->full_name);
> +	       (unsigned int)client->addr, node->full_name);
>  	return 0;
> - detach:
> -	i2c_detach_client(&tas->i2c);
>   fail:
>  	mutex_destroy(&tas->mtx);
>  	kfree(tas);
> @@ -970,14 +980,11 @@ static int tas_i2c_attach(struct i2c_ada
>  	return -ENODEV;
>  }
>  
> -static int tas_i2c_detach(struct i2c_client *client)
> +static int tas_i2c_remove(struct i2c_client *client)
>  {
> -	struct tas *tas = container_of(client, struct tas, i2c);
> -	int err;
> +	struct tas *tas = i2c_get_clientdata(client);
>  	u8 tmp = TAS_ACR_ANALOG_PDOWN;
>  
> -	if ((err = i2c_detach_client(client)))
> -		return err;
>  	aoa_codec_unregister(&tas->codec);
>  	of_node_put(tas->codec.node);
>  
> @@ -989,13 +996,21 @@ static int tas_i2c_detach(struct i2c_cli
>  	return 0;
>  }
>  
> +static const struct i2c_device_id tas_i2c_id[] = {
> +	{ "aoa_codec_tas", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tas_i2c_id);
> +
>  static struct i2c_driver tas_driver = {
>  	.driver = {
>  		.name = "aoa_codec_tas",
>  		.owner = THIS_MODULE,
>  	},
>  	.attach_adapter = tas_i2c_attach,
> -	.detach_client = tas_i2c_detach,
> +	.probe = tas_i2c_probe,
> +	.remove = tas_i2c_remove,
> +	.id_table = tas_i2c_id,
>  };
>  
>  static int __init tas_init(void)
> 
> 


-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

On Fri, 10 Apr 2009 17:02:38 +0200, Jean Delvare wrote:
> On Thu, 9 Apr 2009 14:19:45 +0200, Jean Delvare wrote:
> > From: Jean Delvare <khali@linux-fr.org>
> > Subject: AOA: Convert onyx and tas codecs to new-style i2c drivers
> > 
> > The legacy i2c binding model is going away soon, so convert the AOA
> > codec drivers to the new model or they'll break.
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Johannes Berg <johannes@sipsolutions.net>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >  sound/aoa/codecs/onyx.c |   71 +++++++++++++++++++++++++++++------------------
> >  sound/aoa/codecs/tas.c  |   63 +++++++++++++++++++++++++----------------
> >  2 files changed, 84 insertions(+), 50 deletions(-)
> 
> Note to potential testers of this patch: you need to apply this i2c
> patch first:
> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-loosen-driver-check.patch

This patch is is Linus' tree now:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=935298696f469c0e07c73be687bd055878074ce0

-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Takashi Iwai]

At Tue, 14 Apr 2009 16:37:41 +0200,
Jean Delvare wrote:
> 
> On Fri, 10 Apr 2009 17:02:38 +0200, Jean Delvare wrote:
> > On Thu, 9 Apr 2009 14:19:45 +0200, Jean Delvare wrote:
> > > From: Jean Delvare <khali@linux-fr.org>
> > > Subject: AOA: Convert onyx and tas codecs to new-style i2c drivers
> > > 
> > > The legacy i2c binding model is going away soon, so convert the AOA
> > > codec drivers to the new model or they'll break.
> > > 
> > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > > Cc: Johannes Berg <johannes@sipsolutions.net>
> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > ---
> > >  sound/aoa/codecs/onyx.c |   71 +++++++++++++++++++++++++++++------------------
> > >  sound/aoa/codecs/tas.c  |   63 +++++++++++++++++++++++++----------------
> > >  2 files changed, 84 insertions(+), 50 deletions(-)
> > 
> > Note to potential testers of this patch: you need to apply this i2c
> > patch first:
> > 
> > ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/i2c-loosen-driver-check.patch
> 
> This patch is is Linus' tree now:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=935298696f469c0e07c73be687bd055878074ce0

Great, so we can simply apply patches to sound git tree.

Johannes, please let me know if the patch works.  Then I'll merge them.


thanks,

Takashi

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

On Tue, 14 Apr 2009 16:45:38 +0200, Takashi Iwai wrote:
> Johannes, please let me know if the patch works.  Then I'll merge them.

Note if it matters: the new I2C binding model my patch uses is only
available since kernel 2.6.26.

-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Takashi Iwai]

At Thu, 16 Apr 2009 09:53:39 +0200,
Jean Delvare wrote:
> 
> On Tue, 14 Apr 2009 16:45:38 +0200, Takashi Iwai wrote:
> > Johannes, please let me know if the patch works.  Then I'll merge them.
> 
> Note if it matters: the new I2C binding model my patch uses is only
> available since kernel 2.6.26.

Yep, I'll add some backward compatible stuff to alsa-driver external
tree so that it can be built for older kernels.


thanks,

Takashi

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

On Thu, 2009-04-09 at 14:19 +0200, Jean Delvare wrote:

> From: Jean Delvare <khali@linux-fr.org>
> Subject: AOA: Convert onyx and tas codecs to new-style i2c drivers
> 
> The legacy i2c binding model is going away soon, so convert the AOA
> codec drivers to the new model or they'll break.

So I went to test this, but for some reason aoa doesn't work for me at
all, neither with nor without this patch.

The reason seems to be that aoa_fabric_layout_probe() is never called,
but my driver model fu is weak so I can't tell why -- I definitely see
the i2sbus soundbus devices registered. soundbus_probe() isn't getting
called either though, but we do of_device_register(). I'm confused, but
don't have time to dig further right now.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On Tue, 2009-04-14 at 17:40 +0200, Johannes Berg wrote:

> So I went to test this, but for some reason aoa doesn't work for me at
> all, neither with nor without this patch.
> 
> The reason seems to be that aoa_fabric_layout_probe() is never called,
> but my driver model fu is weak so I can't tell why -- I definitely see
> the i2sbus soundbus devices registered. soundbus_probe() isn't getting
> called either though, but we do of_device_register(). I'm confused, but
> don't have time to dig further right now.

2.6.29 works -- guess somebody could bisect, but I can't now.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

Hi Johannes,

On Tue, 14 Apr 2009 17:50:27 +0200, Johannes Berg wrote:
> On Tue, 2009-04-14 at 17:40 +0200, Johannes Berg wrote:
> 
> > So I went to test this, but for some reason aoa doesn't work for me at
> > all, neither with nor without this patch.
> > 
> > The reason seems to be that aoa_fabric_layout_probe() is never called,
> > but my driver model fu is weak so I can't tell why -- I definitely see
> > the i2sbus soundbus devices registered. soundbus_probe() isn't getting
> > called either though, but we do of_device_register(). I'm confused, but
> > don't have time to dig further right now.
> 
> 2.6.29 works -- guess somebody could bisect, but I can't now.

Thanks for your try. Note that you should be able to apply my AOA patch
to 2.6.29 for testing purposes. All you need is to also apply the other
patch first:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=935298696f469c0e07c73be687bd055878074ce0

Thanks,
-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 5305 bytes --]

On Tue, 2009-04-14 at 17:51 +0200, Johannes Berg wrote:
> On Tue, 2009-04-14 at 17:40 +0200, Johannes Berg wrote:
> 
> > So I went to test this, but for some reason aoa doesn't work for me at
> > all, neither with nor without this patch.
> > 
> > The reason seems to be that aoa_fabric_layout_probe() is never called,
> > but my driver model fu is weak so I can't tell why -- I definitely see
> > the i2sbus soundbus devices registered. soundbus_probe() isn't getting
> > called either though, but we do of_device_register(). I'm confused, but
> > don't have time to dig further right now.
> 
> 2.6.29 works -- guess somebody could bisect, but I can't now.

Alright, with the patch Andreas pointed out it loads, but segfaults, as
below. Works fine without your patch.

johannes

[   10.267137] snd-aoa-codec-onyx: found pcm3052
[   10.267238] PM: Adding info for i2c:2-0046
[   10.267926] snd-aoa-fabric-layout: platform-onyx-codec-ref doesn't match!
[   10.267930] snd-aoa: fabric didn't like codec onyx
[   10.268041] aoa_codec_onyx: probe of 2-0046 failed with error -22
[   10.268067] Unable to handle kernel paging request for data at address 0x000000d0
[   10.268070] Faulting instruction address: 0xd000000001291d68
[   10.268074] Oops: Kernel access of bad area, sig: 11 [#1]
[   10.268076] PREEMPT SMP NR_CPUS=4 PowerMac
[   10.268080] Modules linked in: snd_aoa_codec_onyx(+) firewire_ohci usbhid(+) arc4 snd_aoa_fabric_layout(+) snd_aoa ecb firewire_core crc_itu_t snd_aoa_i2sbus snd_aoa_soundbus iwlagn iwlcore snd_pcm snd_page_alloc ohci_hcd snd_timer ohci1394 ehci_hcd rfkill led_class ieee1394 usbcore snd mac80211 soundcore cfg80211
[   10.268109] NIP: d000000001291d68 LR: d000000001291d20 CTR: 0000000000000000
[   10.268113] REGS: c0000001e19a34e0 TRAP: 0300   Not tainted  (2.6.30-rc1-wl-dirty)
[   10.268115] MSR: 9000000000009032 <EE,ME,IR,DR>  CR: 22000488  XER: 00000000
[   10.268124] DAR: 00000000000000d0, DSISR: 0000000040000000
[   10.268127] TASK = c000000205cd9d30[3391] 'modprobe' THREAD: c0000001e19a0000 CPU: 1
[   10.268130] GPR00: d000000001291d20 c0000001e19a3760 d00000000129cdd8 c000000200c177e0 
[   10.268135] GPR04: c000000205cda760 0000000000000007 c000000205cd9d68 c000000205cd9d68 
[   10.268140] GPR08: 0000000000000000 0000000000000000 c000000200c179f0 c0000000010ab590 
[   10.268145] GPR12: 0000000088000488 c0000000009c2500 0000000000000000 0000000000000000 
[   10.268150] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 
[   10.268155] GPR20: 0000000000000000 0000000000000001 000000000054bda0 00000000001ed780 
[   10.268160] GPR24: 0000000000548080 00000000008c1858 c000000000847b18 0000000000000046 
[   10.268165] GPR28: c000000217ff56d8 c000000200a59e68 d00000000129c990 c0000001e19a37d8 
[   10.268175] NIP [d000000001291d68] .onyx_create+0xc0/0x100 [snd_aoa_codec_onyx]
[   10.268181] LR [d000000001291d20] .onyx_create+0x78/0x100 [snd_aoa_codec_onyx]
[   10.268183] Call Trace:
[   10.268188] [c0000001e19a3760] [d000000001291d20] .onyx_create+0x78/0x100 [snd_aoa_codec_onyx] (unreliable)
[   10.268196] [c0000001e19a3840] [c00000000037f900] .__attach_adapter+0x4c/0x6c
[   10.268203] [c0000001e19a38d0] [c0000000002f7e34] .class_for_each_device+0xb4/0x10c
[   10.268208] [c0000001e19a3990] [c00000000037ecec] .i2c_register_driver+0xf0/0x128
[   10.268214] [c0000001e19a3a30] [d000000001291f20] .onyx_init+0x20/0x668 [snd_aoa_codec_onyx]
[   10.268219] [c0000001e19a3ab0] [c000000000007f68] .do_one_initcall+0x9c/0x1dc
[   10.268225] [c0000001e19a3d90] [c000000000099b0c] .SyS_init_module+0xd8/0x238
[   10.268229] [c0000001e19a3e30] [c000000000007554] syscall_exit+0x0/0x40
[   10.268232] Instruction dump:
[   10.268235] a0090022 8129001e b0010074 91210070 48000341 e8410028 2fa30000 3900ffed 
[   10.268242] 419e0028 e9230020 39430210 39000000 <e96900d0> 380900c8 f94900d0 f8030210 
[   10.268253] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/3391
[   10.268258] caller is .die+0x188/0x1d4
[   10.268260] Call Trace:
[   10.268264] [c0000001e19a3210] [c00000000000fcc4] .show_stack+0x6c/0x174 (unreliable)
[   10.268271] [c0000001e19a32c0] [c00000000029e65c] .debug_smp_processor_id+0xe0/0x118
[   10.268275] [c0000001e19a3350] [c000000000020918] .die+0x188/0x1d4
[   10.268280] [c0000001e19a33f0] [c000000000027f74] .bad_page_fault+0xb8/0xd4
[   10.268284] [c0000001e19a3470] [c00000000000524c] handle_page_fault+0x3c/0x5c
[   10.268293] --- Exception: 300 at .onyx_create+0xc0/0x100 [snd_aoa_codec_onyx]
[   10.268295]     LR = .onyx_create+0x78/0x100 [snd_aoa_codec_onyx]
[   10.268299] [c0000001e19a3840] [c00000000037f900] .__attach_adapter+0x4c/0x6c
[   10.268304] [c0000001e19a38d0] [c0000000002f7e34] .class_for_each_device+0xb4/0x10c
[   10.268308] [c0000001e19a3990] [c00000000037ecec] .i2c_register_driver+0xf0/0x128
[   10.268315] [c0000001e19a3a30] [d000000001291f20] .onyx_init+0x20/0x668 [snd_aoa_codec_onyx]
[   10.268319] [c0000001e19a3ab0] [c000000000007f68] .do_one_initcall+0x9c/0x1dc
[   10.268323] [c0000001e19a3d90] [c000000000099b0c] .SyS_init_module+0xd8/0x238
[   10.268328] [c0000001e19a3e30] [c000000000007554] syscall_exit+0x0/0x40
[   10.268331] ---[ end trace dbcf63aa775331e7 ]---


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

Hi Johannes,

On Tue, 14 Apr 2009 19:41:55 +0200, Johannes Berg wrote:
> Alright, with the patch Andreas pointed out it loads, but segfaults, as
> below. Works fine without your patch.

Thanks for the quick test and sorry that it didn't work. I'll take a
look at the trace below and try to figure out what went wrong.

Did you remove the 2 MODULE_DEVICE_TABLE from my patch? If you didn't,
please pick the latest version of my patch which doesn't have them:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/sound-aoa-codecs-convert-to-new-style.patch
I don't think they are the reason of the crash, but who knows...

Are you using a machine with onyx or tas? I guess onyx but I want to be
sure.

> [   10.267137] snd-aoa-codec-onyx: found pcm3052
> [   10.267238] PM: Adding info for i2c:2-0046
> [   10.267926] snd-aoa-fabric-layout: platform-onyx-codec-ref doesn't match!

Does this error also happen without my patch? It would help to see the
logs without my patch to see where it starts diverging.

> [   10.267930] snd-aoa: fabric didn't like codec onyx
> [   10.268041] aoa_codec_onyx: probe of 2-0046 failed with error -22

Apparently aoa_codec_register failed in onyx_i2c_probe(), I have to
understand why.

> [   10.268067] Unable to handle kernel paging request for data at address 0x000000d0
> [   10.268070] Faulting instruction address: 0xd000000001291d68
> [   10.268074] Oops: Kernel access of bad area, sig: 11 [#1]
> [   10.268076] PREEMPT SMP NR_CPUS=4 PowerMac
> [   10.268080] Modules linked in: snd_aoa_codec_onyx(+) firewire_ohci usbhid(+) arc4 snd_aoa_fabric_layout(+) snd_aoa ecb firewire_core crc_itu_t snd_aoa_i2sbus snd_aoa_soundbus iwlagn iwlcore snd_pcm snd_page_alloc ohci_hcd snd_timer ohci1394 ehci_hcd rfkill led_class ieee1394 usbcore snd mac80211 soundcore cfg80211
> [   10.268109] NIP: d000000001291d68 LR: d000000001291d20 CTR: 0000000000000000
> [   10.268113] REGS: c0000001e19a34e0 TRAP: 0300   Not tainted  (2.6.30-rc1-wl-dirty)
> [   10.268115] MSR: 9000000000009032 <EE,ME,IR,DR>  CR: 22000488  XER: 00000000
> [   10.268124] DAR: 00000000000000d0, DSISR: 0000000040000000
> [   10.268127] TASK = c000000205cd9d30[3391] 'modprobe' THREAD: c0000001e19a0000 CPU: 1
> [   10.268130] GPR00: d000000001291d20 c0000001e19a3760 d00000000129cdd8 c000000200c177e0 
> [   10.268135] GPR04: c000000205cda760 0000000000000007 c000000205cd9d68 c000000205cd9d68 
> [   10.268140] GPR08: 0000000000000000 0000000000000000 c000000200c179f0 c0000000010ab590 
> [   10.268145] GPR12: 0000000088000488 c0000000009c2500 0000000000000000 0000000000000000 
> [   10.268150] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000 
> [   10.268155] GPR20: 0000000000000000 0000000000000001 000000000054bda0 00000000001ed780 
> [   10.268160] GPR24: 0000000000548080 00000000008c1858 c000000000847b18 0000000000000046 
> [   10.268165] GPR28: c000000217ff56d8 c000000200a59e68 d00000000129c990 c0000001e19a37d8 
> [   10.268175] NIP [d000000001291d68] .onyx_create+0xc0/0x100 [snd_aoa_codec_onyx]
> [   10.268181] LR [d000000001291d20] .onyx_create+0x78/0x100 [snd_aoa_codec_onyx]
> [   10.268183] Call Trace:
> [   10.268188] [c0000001e19a3760] [d000000001291d20] .onyx_create+0x78/0x100 [snd_aoa_codec_onyx] (unreliable)
> [   10.268196] [c0000001e19a3840] [c00000000037f900] .__attach_adapter+0x4c/0x6c
> [   10.268203] [c0000001e19a38d0] [c0000000002f7e34] .class_for_each_device+0xb4/0x10c
> [   10.268208] [c0000001e19a3990] [c00000000037ecec] .i2c_register_driver+0xf0/0x128
> [   10.268214] [c0000001e19a3a30] [d000000001291f20] .onyx_init+0x20/0x668 [snd_aoa_codec_onyx]
> [   10.268219] [c0000001e19a3ab0] [c000000000007f68] .do_one_initcall+0x9c/0x1dc
> [   10.268225] [c0000001e19a3d90] [c000000000099b0c] .SyS_init_module+0xd8/0x238
> [   10.268229] [c0000001e19a3e30] [c000000000007554] syscall_exit+0x0/0x40
> [   10.268232] Instruction dump:
> [   10.268235] a0090022 8129001e b0010074 91210070 48000341 e8410028 2fa30000 3900ffed 
> [   10.268242] 419e0028 e9230020 39430210 39000000 <e96900d0> 380900c8 f94900d0 f8030210 
> [   10.268253] BUG: using smp_processor_id() in preemptible [00000000] code: modprobe/3391
> [   10.268258] caller is .die+0x188/0x1d4
> [   10.268260] Call Trace:
> [   10.268264] [c0000001e19a3210] [c00000000000fcc4] .show_stack+0x6c/0x174 (unreliable)
> [   10.268271] [c0000001e19a32c0] [c00000000029e65c] .debug_smp_processor_id+0xe0/0x118
> [   10.268275] [c0000001e19a3350] [c000000000020918] .die+0x188/0x1d4
> [   10.268280] [c0000001e19a33f0] [c000000000027f74] .bad_page_fault+0xb8/0xd4
> [   10.268284] [c0000001e19a3470] [c00000000000524c] handle_page_fault+0x3c/0x5c
> [   10.268293] --- Exception: 300 at .onyx_create+0xc0/0x100 [snd_aoa_codec_onyx]
> [   10.268295]     LR = .onyx_create+0x78/0x100 [snd_aoa_codec_onyx]
> [   10.268299] [c0000001e19a3840] [c00000000037f900] .__attach_adapter+0x4c/0x6c
> [   10.268304] [c0000001e19a38d0] [c0000000002f7e34] .class_for_each_device+0xb4/0x10c
> [   10.268308] [c0000001e19a3990] [c00000000037ecec] .i2c_register_driver+0xf0/0x128
> [   10.268315] [c0000001e19a3a30] [d000000001291f20] .onyx_init+0x20/0x668 [snd_aoa_codec_onyx]
> [   10.268319] [c0000001e19a3ab0] [c000000000007f68] .do_one_initcall+0x9c/0x1dc
> [   10.268323] [c0000001e19a3d90] [c000000000099b0c] .SyS_init_module+0xd8/0x238
> [   10.268328] [c0000001e19a3e30] [c000000000007554] syscall_exit+0x0/0x40
> [   10.268331] ---[ end trace dbcf63aa775331e7 ]---
> 


-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]

Hi Jean,

> Thanks for the quick test and sorry that it didn't work. I'll take a
> look at the trace below and try to figure out what went wrong.

No worries, seems some error path is going wrong but I can't see what it
is right now.

> Did you remove the 2 MODULE_DEVICE_TABLE from my patch? If you didn't,
> please pick the latest version of my patch which doesn't have them:
> ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/sound-aoa-codecs-convert-to-new-style.patch
> I don't think they are the reason of the crash, but who knows...

No, I didn't, but I was loading the modules manually so that didn't kick
in.

> Are you using a machine with onyx or tas? I guess onyx but I want to be
> sure.

onyx only.

> > [   10.267137] snd-aoa-codec-onyx: found pcm3052
> > [   10.267238] PM: Adding info for i2c:2-0046
> > [   10.267926] snd-aoa-fabric-layout: platform-onyx-codec-ref doesn't match!
> 
> Does this error also happen without my patch? It would help to see the
> logs without my patch to see where it starts diverging.

Yes -- this happens normally.

> > [   10.267930] snd-aoa: fabric didn't like codec onyx
> > [   10.268041] aoa_codec_onyx: probe of 2-0046 failed with error -22
> 
> Apparently aoa_codec_register failed in onyx_i2c_probe(), I have to
> understand why.

Because the device-tree is broken -- there are two nodes for the same
device, and only one of them can be used. Then the fabric rejects the
first instantiation from the broken node. Here's how it looks normally:

...
[   10.398296] snd-aoa-codec-onyx: found pcm3052
[   10.398472] PM: Adding info for i2c:2-0046
[   10.412189] snd-aoa-fabric-layout: platform-onyx-codec-ref doesn't match!
[   10.462593] snd-aoa: fabric didn't like codec onyx
[   10.468030] PM: Removing info for i2c:2-0046
[   10.473892] snd-aoa-codec-onyx: found pcm3052
[   10.479317] PM: Adding info for i2c:3-0046
[   10.485631] snd-aoa-fabric-layout: can use this codec
...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

On Tue, 14 Apr 2009 23:59:59 +0200, Johannes Berg wrote:
> Hi Jean,
> 
> > Thanks for the quick test and sorry that it didn't work. I'll take a
> > look at the trace below and try to figure out what went wrong.
> 
> No worries, seems some error path is going wrong but I can't see what it
> is right now.
> 
> > Did you remove the 2 MODULE_DEVICE_TABLE from my patch? If you didn't,
> > please pick the latest version of my patch which doesn't have them:
> > ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/sound-aoa-codecs-convert-to-new-style.patch
> > I don't think they are the reason of the crash, but who knows...
> 
> No, I didn't, but I was loading the modules manually so that didn't kick
> in.
> 
> > Are you using a machine with onyx or tas? I guess onyx but I want to be
> > sure.
> 
> onyx only.
> 
> > > [   10.267137] snd-aoa-codec-onyx: found pcm3052
> > > [   10.267238] PM: Adding info for i2c:2-0046
> > > [   10.267926] snd-aoa-fabric-layout: platform-onyx-codec-ref doesn't match!
> > 
> > Does this error also happen without my patch? It would help to see the
> > logs without my patch to see where it starts diverging.
> 
> Yes -- this happens normally.

OK. I couldn't see how my patch would cause this error, so if it was
already there before, it's good news.

> > > [   10.267930] snd-aoa: fabric didn't like codec onyx
> > > [   10.268041] aoa_codec_onyx: probe of 2-0046 failed with error -22
> > 
> > Apparently aoa_codec_register failed in onyx_i2c_probe(), I have to
> > understand why.
> 
> Because the device-tree is broken -- there are two nodes for the same
> device, and only one of them can be used. Then the fabric rejects the
> first instantiation from the broken node. Here's how it looks normally:
> 
> ...
> [   10.398296] snd-aoa-codec-onyx: found pcm3052
> [   10.398472] PM: Adding info for i2c:2-0046
> [   10.412189] snd-aoa-fabric-layout: platform-onyx-codec-ref doesn't match!
> [   10.462593] snd-aoa: fabric didn't like codec onyx
> [   10.468030] PM: Removing info for i2c:2-0046
> [   10.473892] snd-aoa-codec-onyx: found pcm3052
> [   10.479317] PM: Adding info for i2c:3-0046
> [   10.485631] snd-aoa-fabric-layout: can use this codec
> ...

OK, I understand better what is going on now. I do not understand the
crash at the end though, but I suspect it isn't a bug in my code but
simply a faulty error path which had never been taken before.

Now, the fact that there are two nodes, one right and one wrong, is
quite a problem. My code expected the device tree to be trustworthy.
The new i2c binding model cleanly separates the instantiation of
devices and their binding to a driver. The codec check which fails,
will cause the i2c device to not bind to its driver, but it will still
be present, while there is no underlying physical I2C device if I
understand properly. In the new i2c model, you have to ensure that an
i2c device exists _before_ you instantiate it.

Well, there is a dirty workaround, which I will apply for now, but...
ideally the layout factory should be revisited so that the codec check
happens earlier. Is this something you could help with?

That's something which isn't too clear to me: is there a physical
device at 2-0046 and 3-0046? The onyx codec is accepted for the latter,
however it seems that the test of a device presence at 2-0046 succeeds
as well...

-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2432 bytes --]

Hi,

> > Because the device-tree is broken -- there are two nodes for the same
> > device, and only one of them can be used. Then the fabric rejects the
> > first instantiation from the broken node. Here's how it looks normally:
> > 
> > ...
> > [   10.398296] snd-aoa-codec-onyx: found pcm3052
> > [   10.398472] PM: Adding info for i2c:2-0046
> > [   10.412189] snd-aoa-fabric-layout: platform-onyx-codec-ref doesn't match!
> > [   10.462593] snd-aoa: fabric didn't like codec onyx
> > [   10.468030] PM: Removing info for i2c:2-0046
> > [   10.473892] snd-aoa-codec-onyx: found pcm3052
> > [   10.479317] PM: Adding info for i2c:3-0046
> > [   10.485631] snd-aoa-fabric-layout: can use this codec
> > ...
> 
> OK, I understand better what is going on now. I do not understand the
> crash at the end though, but I suspect it isn't a bug in my code but
> simply a faulty error path which had never been taken before.

That would be weird -- the error path _has_ to be taken always in onyx.
Unless you're talking about something in the i2c core or whatever?

> Now, the fact that there are two nodes, one right and one wrong, is
> quite a problem. My code expected the device tree to be trustworthy.

No such luck.

> The new i2c binding model cleanly separates the instantiation of
> devices and their binding to a driver. The codec check which fails,
> will cause the i2c device to not bind to its driver, but it will still
> be present, while there is no underlying physical I2C device if I
> understand properly. In the new i2c model, you have to ensure that an
> i2c device exists _before_ you instantiate it.

Oh, no, there _is_ an underlying i2c device, the same one. The DT
accidentally has two almost identical nodes for the same chip, but we
can only use the one with the correct reference.

> Well, there is a dirty workaround, which I will apply for now, but...
> ideally the layout factory should be revisited so that the codec check
> happens earlier. Is this something you could help with?

That's not really possible unless the factory post-processes the entire
device-tree -- very ugly.

> That's something which isn't too clear to me: is there a physical
> device at 2-0046 and 3-0046? The onyx codec is accepted for the latter,
> however it seems that the test of a device presence at 2-0046 succeeds
> as well...

It's the _same_ physical device.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

On Wed, 15 Apr 2009 14:52:14 +0200, Johannes Berg wrote:
> > OK, I understand better what is going on now. I do not understand the
> > crash at the end though, but I suspect it isn't a bug in my code but
> > simply a faulty error path which had never been taken before.
> 
> That would be weird -- the error path _has_ to be taken always in onyx.
> Unless you're talking about something in the i2c core or whatever?

Yes, i2c core or even driver core. I'll see if I can reproduce it.

> > (...)
> > Well, there is a dirty workaround, which I will apply for now, but...
> > ideally the layout factory should be revisited so that the codec check
> > happens earlier. Is this something you could help with?
> 
> That's not really possible unless the factory post-processes the entire
> device-tree -- very ugly.

What I had in mind was not so complex. Simply, we could move the
i2c_new_device() calls into layout_found_codec(). That way we can
decide to instantiate the I2C device if and only if check_codec() is
successful. This is more efficient that creating the device, letting
the driver attach to it, with probing eventually failing, and then
removing the device if it wasn't the right one.

That is, the i2c client would be a mere helper on top of struct
aoa_codec, rather than the other way around.

There may be preliminary work needed, for example switching powermac to
numbered I2C buses.

> > That's something which isn't too clear to me: is there a physical
> > device at 2-0046 and 3-0046? The onyx codec is accepted for the latter,
> > however it seems that the test of a device presence at 2-0046 succeeds
> > as well...
> 
> It's the _same_ physical device.

Wow. One I2C device which can be reached through 2 different I2C buses?
First time I hear about something like this. Very odd. I can't see the
point of doing this.

-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2067 bytes --]

On Wed, 2009-04-15 at 15:06 +0200, Jean Delvare wrote:

> > That would be weird -- the error path _has_ to be taken always in onyx.
> > Unless you're talking about something in the i2c core or whatever?
> 
> Yes, i2c core or even driver core. I'll see if I can reproduce it.

Alright.

> > > (...)
> > > Well, there is a dirty workaround, which I will apply for now, but...
> > > ideally the layout factory should be revisited so that the codec check
> > > happens earlier. Is this something you could help with?
> > 
> > That's not really possible unless the factory post-processes the entire
> > device-tree -- very ugly.
> 
> What I had in mind was not so complex. Simply, we could move the
> i2c_new_device() calls into layout_found_codec(). That way we can
> decide to instantiate the I2C device if and only if check_codec() is
> successful. This is more efficient that creating the device, letting
> the driver attach to it, with probing eventually failing, and then
> removing the device if it wasn't the right one.
> 
> That is, the i2c client would be a mere helper on top of struct
> aoa_codec, rather than the other way around.

Ah. That would probably work, but right now I have little motivation to
work on it -- I hardly use the G5 desktop machine any more.

> > > That's something which isn't too clear to me: is there a physical
> > > device at 2-0046 and 3-0046? The onyx codec is accepted for the latter,
> > > however it seems that the test of a device presence at 2-0046 succeeds
> > > as well...
> > 
> > It's the _same_ physical device.
> 
> Wow. One I2C device which can be reached through 2 different I2C buses?
> First time I hear about something like this. Very odd. I can't see the
> point of doing this.

Hmm. How do you know it's on different buses? I don't really know
anything -- all I know is that the DT is buggered and lists the codec
twice. Since we can talk to both, then I'm sure it's just one chip, they
wouldn't put the chip in twice when you can use only one of them :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

On Wed, 15 Apr 2009 15:18:10 +0200, Johannes Berg wrote:
> On Wed, 2009-04-15 at 15:06 +0200, Jean Delvare wrote:
> > Yes, i2c core or even driver core. I'll see if I can reproduce it.
> 
> Alright.

Hmm, couldn't reproduce it. Maybe it is fixed in rc2. I don't have too
much time to spend on this, so if we don't hit it again I consider it
solved.

> > (...)
> > What I had in mind was not so complex. Simply, we could move the
> > i2c_new_device() calls into layout_found_codec(). That way we can
> > decide to instantiate the I2C device if and only if check_codec() is
> > successful. This is more efficient that creating the device, letting
> > the driver attach to it, with probing eventually failing, and then
> > removing the device if it wasn't the right one.
> > 
> > That is, the i2c client would be a mere helper on top of struct
> > aoa_codec, rather than the other way around.
> 
> Ah. That would probably work, but right now I have little motivation to
> work on it -- I hardly use the G5 desktop machine any more.

OK, no problem. I don't want to force anyone to spend time on this. But
if anyone ever does and need my help for the i2c part, just drop me a
line!

> > Wow. One I2C device which can be reached through 2 different I2C buses?
> > First time I hear about something like this. Very odd. I can't see the
> > point of doing this.
> 
> Hmm. How do you know it's on different buses?

2-0046 fails and 3-0046 succeeds. The second number is the hexadecimal
I2C address, the first number is the I2C bus number. So, unless
i2c-powermac was told to register the same I2C bus twice (which would
be dangerous), the device can be accessed through 2 different buses.

> I don't really know
> anything -- all I know is that the DT is buggered and lists the codec
> twice. Since we can talk to both, then I'm sure it's just one chip, they
> wouldn't put the chip in twice when you can use only one of them :)

There's probably little point in trying to guess anything further, the
only thing that would help are the detailed schematics of that part of
the board.

-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Andreas Schwab]

Jean Delvare <khali@linux-fr.org> writes:

> Hi Johannes,
>
> On Tue, 14 Apr 2009 19:41:55 +0200, Johannes Berg wrote:
>> Alright, with the patch Andreas pointed out it loads, but segfaults, as
>> below. Works fine without your patch.
>
> Thanks for the quick test and sorry that it didn't work. I'll take a
> look at the trace below and try to figure out what went wrong.
>
> Did you remove the 2 MODULE_DEVICE_TABLE from my patch? If you didn't,
> please pick the latest version of my patch which doesn't have them:
> ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/sound-aoa-codecs-convert-to-new-style.patch
> I don't think they are the reason of the crash, but who knows...

I tried with a tas-based iBook, and it works fine here.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Jean Delvare]

On Wed, 15 Apr 2009 00:48:08 +0200, Andreas Schwab wrote:
> Jean Delvare <khali@linux-fr.org> writes:
> 
> > Hi Johannes,
> >
> > On Tue, 14 Apr 2009 19:41:55 +0200, Johannes Berg wrote:
> >> Alright, with the patch Andreas pointed out it loads, but segfaults, as
> >> below. Works fine without your patch.
> >
> > Thanks for the quick test and sorry that it didn't work. I'll take a
> > look at the trace below and try to figure out what went wrong.
> >
> > Did you remove the 2 MODULE_DEVICE_TABLE from my patch? If you didn't,
> > please pick the latest version of my patch which doesn't have them:
> > ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-i2c/sound-aoa-codecs-convert-to-new-style.patch
> > I don't think they are the reason of the crash, but who knows...
> 
> I tried with a tas-based iBook, and it works fine here.

Good news, thanks Andreas!

-- 
Jean Delvare

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Andreas Schwab]

Johannes Berg <johannes@sipsolutions.net> writes:

> So I went to test this, but for some reason aoa doesn't work for me at
> all, neither with nor without this patch.

That is already fixed by
<http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver-core.current/driver-core-fix-driver_match_device.patch>

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] AOA: Convert onyx and tas codecs to new-style i2c drivers

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

On Tue, 2009-04-14 at 18:48 +0200, Andreas Schwab wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > So I went to test this, but for some reason aoa doesn't work for me at
> > all, neither with nor without this patch.
> 
> That is already fixed by
> <http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver-core.current/driver-core-fix-driver_match_device.patch>

Ah, cool, thanks. I'll add that to the mix and test again.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]