Message ID | 20090408150249.5a62a56c@hyperion.delvare (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
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
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.
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
--- 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)
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(-)