[alsa-devel] [PATCH] keywest: Convert to new-style i2c driver
The legacy i2c binding model is going away soon, so convert the ppc keywest sound driver to the new model or it will break.
Signed-off-by: Jean Delvare khali@linux-fr.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.
I did not get any test report for this one, but it's relatively simple so I am confident I got it right.
sound/ppc/keywest.c | 82 +++++++++++++++++++++++++-------------------------- 1 file changed, 41 insertions(+), 41 deletions(-)
--- linux-2.6.30-rc2.orig/sound/ppc/keywest.c 2009-04-20 22:40:27.000000000 +0200 +++ linux-2.6.30-rc2/sound/ppc/keywest.c 2009-04-20 22:47:34.000000000 +0200 @@ -33,26 +33,25 @@ static struct pmac_keywest *keywest_ctx;
-static int keywest_attach_adapter(struct i2c_adapter *adapter); -static int keywest_detach_client(struct i2c_client *client); - -struct i2c_driver keywest_driver = { - .driver = { - .name = "PMac Keywest Audio", - }, - .attach_adapter = &keywest_attach_adapter, - .detach_client = &keywest_detach_client, -}; - - #ifndef i2c_device_name #define i2c_device_name(x) ((x)->name) #endif
+static int keywest_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + i2c_set_clientdata(client, keywest_ctx); + return 0; +} + +/* + * This is kind of a hack, best would be to turn powermac to fixed i2c + * bus numbers and declare the sound device as part of platform + * initialization + */ static int keywest_attach_adapter(struct i2c_adapter *adapter) { - int err; - struct i2c_client *new_client; + struct i2c_board_info info;
if (! keywest_ctx) return -EINVAL; @@ -60,46 +59,47 @@ static int keywest_attach_adapter(struct if (strncmp(i2c_device_name(adapter), "mac-io", 6)) return 0; /* ignored */
- new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL); - if (! new_client) - return -ENOMEM; - - new_client->addr = keywest_ctx->addr; - i2c_set_clientdata(new_client, keywest_ctx); - new_client->adapter = adapter; - new_client->driver = &keywest_driver; - new_client->flags = 0; - - strcpy(i2c_device_name(new_client), keywest_ctx->name); - keywest_ctx->client = new_client; + memset(&info, 0, sizeof(struct i2c_board_info)); + strlcpy(info.type, "keywest", I2C_NAME_SIZE); + info.addr = keywest_ctx->addr; + keywest_ctx->client = i2c_new_device(adapter, &info); - /* Tell the i2c layer a new client has arrived */ - if (i2c_attach_client(new_client)) { - snd_printk(KERN_ERR "tumbler: cannot attach i2c client\n"); - err = -ENODEV; - goto __err; - } - + /* + * 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(&keywest_ctx->client->detected, + &keywest_ctx->client->driver->clients); return 0; - - __err: - kfree(new_client); - keywest_ctx->client = NULL; - return err; }
-static int keywest_detach_client(struct i2c_client *client) +static int keywest_remove(struct i2c_client *client) { + i2c_set_clientdata(client, NULL); if (! keywest_ctx) return 0; if (client == keywest_ctx->client) keywest_ctx->client = NULL;
- i2c_detach_client(client); - kfree(client); return 0; }
+ +static const struct i2c_device_id keywest_i2c_id[] = { + { "keywest", 0 }, + { } +}; + +struct i2c_driver keywest_driver = { + .driver = { + .name = "PMac Keywest Audio", + }, + .attach_adapter = keywest_attach_adapter, + .probe = keywest_probe, + .remove = keywest_remove, + .id_table = keywest_i2c_id, +}; + /* exported */ void snd_pmac_keywest_cleanup(struct pmac_keywest *i2c) {
Jean Delvare writes:
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.
I really don't think you can remove it from Linus' tree at this stage in the 2.6.30 cycle. If it was going to be removed it should have been removed in the merge window. Removing it now has too much risk of introducing regressions in my opinion.
I presume you have a development tree where you queue up commits for the i2c subsystem for the next merge window. I suggest you do the removal there now (or whenever you like) and push it to Linus in the next merge window.
Paul.
At Tue, 21 Apr 2009 08:34:02 +1000, Paul Mackerras wrote:
Jean Delvare writes:
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.
I really don't think you can remove it from Linus' tree at this stage in the 2.6.30 cycle. If it was going to be removed it should have been removed in the merge window. Removing it now has too much risk of introducing regressions in my opinion.
I presume you have a development tree where you queue up commits for the i2c subsystem for the next merge window. I suggest you do the removal there now (or whenever you like) and push it to Linus in the next merge window.
At least, the conversion patch Jean posted can be in 2.6.30, I think. As the old API is marked deprecated, it should be fixed sooner or later.
Whether to remove the whole old i2c-binding in 2.6.30 is a different question, although I myself feel it's feasible.
thanks,
Takashi
Hi Paul, Takashi,
On Tue, 21 Apr 2009 08:33:43 +0200, Takashi Iwai wrote:
At Tue, 21 Apr 2009 08:34:02 +1000, Paul Mackerras wrote:
Jean Delvare writes:
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.
I really don't think you can remove it from Linus' tree at this stage in the 2.6.30 cycle. If it was going to be removed it should have been removed in the merge window.
It would have happened if the developers/maintainers who see deprecation warnings when they build their drivers had paid attention to them. And actually some did, but not all. The remaining drivers are the ones nobody cared about, and this is the reason why I have to take care of them myself, that late in the development cycle.
Note that the removal had already been scheduled for 2.6.29, and it did not happen. The set of legacy drivers did shrink a bit between 2.6.29-rc1 and 2.6.30-rc1 (thanks to Hans Verkuil) but not as much as it should have. Basically the number of remaining driver was halved. If the number of remaining drivers is halved with each release cycle, the legacy model is never going to be removed ;)
Removing it now has too much risk of introducing regressions in my opinion.
Not removing it now has a high risk of developers continuing to ignore the deprecation warnings and adding new legacy drivers, which I then must convert to the new model. This never ends.
I know my behavior may seem a bit rude, but apparently this is the only way to get things to actually happen. I've been waiting for over a year already!
I don't think the risk is that high, at least not for sound drivers. The conversions are fairly easy and if something really went wrong, fixing it is a matter of minutes.
Please note that the removal of the legacy model isn't my goal per se. The fact is that the legacy model needs to be removed for further developments of i2c-core to happen, in particular the support of i2C bus multiplexers. There are patches waiting for inclusion since early February, which I can't take as long as the legacy i2c model is in. This is why I am pushing.
I presume you have a development tree where you queue up commits for the i2c subsystem for the next merge window. I suggest you do the removal there now (or whenever you like) and push it to Linus in the next merge window.
At least, the conversion patch Jean posted can be in 2.6.30, I think. As the old API is marked deprecated, it should be fixed sooner or later.
Whether to remove the whole old i2c-binding in 2.6.30 is a different question, although I myself feel it's feasible.
I have converted all remaining drivers by now: http://i2c.wiki.kernel.org/index.php/Legacy_drivers_to_be_converted It's really only a matter of getting them tested in time now. Given that most drivers are powermac ones, what I really need here is powermac users/maintainers to test my patches and report success or failure.
Thanks,
Jean Delvare writes:
Not removing it now has a high risk of developers continuing to ignore the deprecation warnings and adding new legacy drivers, which I then must convert to the new model. This never ends.
I know my behavior may seem a bit rude, but apparently this is the only way to get things to actually happen. I've been waiting for over a year already!
I sympathize, but throwing disruptive changes into Linus' tree when we're past -rc3 is not the way to solve the problem.
The way to solve the problem is to (a) publish a branch where you put the stuff you're going to ask Linus to pull in the next merge window, (b) push a commit there that removes the legacy interfaces, (c) ask Stephen Rothwell to include that branch in linux-next.
The linux-next tree gets built for a wide range of architectures and configs, and any breakages get noticed and fixed pretty quickly. Getting the removal of the legacy interfaces into linux-next will do more in a week than a year's worth of deprecation warnings. :)
I don't think the risk is that high, at least not for sound drivers. The conversions are fairly easy and if something really went wrong, fixing it is a matter of minutes.
We're past -rc3 now. This is not the time for pushing this sort of change into Linus' tree. Ask Linus if you don't believe me.
I have converted all remaining drivers by now: http://i2c.wiki.kernel.org/index.php/Legacy_drivers_to_be_converted It's really only a matter of getting them tested in time now. Given that most drivers are powermac ones, what I really need here is powermac users/maintainers to test my patches and report success or failure.
OK, but please work in your next branch and linux-next, not Linus' tree.
Paul.
Hi Paul,
On Wed, 22 Apr 2009 19:34:40 +1000, Paul Mackerras wrote:
Jean Delvare writes:
Not removing it now has a high risk of developers continuing to ignore the deprecation warnings and adding new legacy drivers, which I then must convert to the new model. This never ends.
I know my behavior may seem a bit rude, but apparently this is the only way to get things to actually happen. I've been waiting for over a year already!
I sympathize, but throwing disruptive changes into Linus' tree when we're past -rc3 is not the way to solve the problem.
We're past -rc3 because people discuss instead of testing my patches. Otherwise everything would be merged already.
And really, these changes (sound drivers) don't qualify as disruptive. You might argue about the thermal management driver changes if you want. But sound drivers, nothing bad will happen if they accidentally break.
The way to solve the problem is to (a) publish a branch where you put the stuff you're going to ask Linus to pull in the next merge window, (b) push a commit there that removes the legacy interfaces, (c) ask Stephen Rothwell to include that branch in linux-next.
The linux-next tree gets built for a wide range of architectures and configs, and any breakages get noticed and fixed pretty quickly. Getting the removal of the legacy interfaces into linux-next will do more in a week than a year's worth of deprecation warnings. :)
What do you think, that I don't know about linux-next? My i2c tree is already there. It has been since linux-next exists.
But linux-next will only build-test the code. That I have already done, so it really doesn't buy my anything. Developers (including me) don't look at warnings in linux-next, and I don't think linux-next gets a lot of testing.
Also, I can't push all untested patches to linux-next. In particular the 4 patches that touch thermal management on powermac, need to be tested successfully by at least one person before I can push them. You did test the therm_pm72 patch, and I thank you for that, so that one is in linux-next, but the other 3 ones need testing.
So, really, you're trying to solve the wrong problem. Whether the patches go to Linus now or in the next merge window, doesn't really matter. What matters is that the patches can't go anywhere before they have been tested. So, can the powerpc people please test my patches? You (powerpc developers, not you Paul in particular) didn't pay attention to the deprecation warnings. I did warn Ben that this would become a problem during the Kernel Summit last year in Portland though, but apparently it did not help. I would expect that you at least help me with testing once I have done all the conversion work.
Out of the 10 remaining legacy i2c drivers, 9 are powerpc drivers!
I don't think the risk is that high, at least not for sound drivers. The conversions are fairly easy and if something really went wrong, fixing it is a matter of minutes.
We're past -rc3 now. This is not the time for pushing this sort of change into Linus' tree. Ask Linus if you don't believe me.
And 2.6.31 isn't the kernel to remove an interface which was scheduled for removal in 2.6.29 and then 2.6.30 and which is blocking the development of features people need badly. One of these two events will happen still. The real world isn't as perfect as we'd like.
So, once again, can powermac developers/users please test my patches?
Thanks,
Jean Delvare writes:
I sympathize, but throwing disruptive changes into Linus' tree when we're past -rc3 is not the way to solve the problem.
We're past -rc3 because people discuss instead of testing my patches. Otherwise everything would be merged already.
Well, no. The first conversion patch that I saw was posted after the merge window had already closed, on 8 April.
And really, these changes (sound drivers) don't qualify as disruptive. You might argue about the thermal management driver changes if you want. But sound drivers, nothing bad will happen if they accidentally break.
That's what we call a "regression". :)
But linux-next will only build-test the code. That I have already done, so it really doesn't buy my anything. Developers (including me) don't look at warnings in linux-next, and I don't think linux-next gets a lot of testing.
If you remove the legacy interfaces then even a build test will point out the drivers that still need to be converted. :)
Also, I can't push all untested patches to linux-next. In particular the 4 patches that touch thermal management on powermac, need to be tested successfully by at least one person before I can push them. You did test the therm_pm72 patch, and I thank you for that, so that one is in linux-next, but the other 3 ones need testing.
So, really, you're trying to solve the wrong problem. Whether the patches go to Linus now or in the next merge window, doesn't really matter.
It does matter, actually.
And 2.6.31 isn't the kernel to remove an interface which was scheduled for removal in 2.6.29 and then 2.6.30 and which is blocking the development of features people need badly.
What's so special about 2.6.30 that it matters whether the legacy interfaces are still there or not?
As for blocking development, you can remove the legacy interfaces today in your next branch and get on with development immediately. So the "blocking development" argument has zero relevance to what goes into Linus' tree for 2.6.30. Even if you got the legacy interfaces removed for 2.6.30 you still wouldn't be able to get any new features based on that into 2.6.30.
And this is a particularly bad time to be hastily trying to get powermac driver changes upstream, since Ben H. is on vacation.
So, once again, can powermac developers/users please test my patches?
Can we compromise on this? I'll do everything I reasonably can to help you get the powermac driver patches tested and working before the 2.6.31 merge window, if you agree to leave the drivers and interfaces in Linus' tree as they are for 2.6.30.
Paul.
At Wed, 22 Apr 2009 22:56:52 +1000, Paul Mackerras wrote:
Jean Delvare writes:
I sympathize, but throwing disruptive changes into Linus' tree when we're past -rc3 is not the way to solve the problem.
We're past -rc3 because people discuss instead of testing my patches. Otherwise everything would be merged already.
Well, no. The first conversion patch that I saw was posted after the merge window had already closed, on 8 April.
And really, these changes (sound drivers) don't qualify as disruptive. You might argue about the thermal management driver changes if you want. But sound drivers, nothing bad will happen if they accidentally break.
That's what we call a "regression". :)
Well, I believe it's even better to merge this conversion patch now, from practical viewpoint.
If we get a regression for this particular device, the patch can be simply reverted as long as the legacy i2c framework exists. If we merge everything, i.e. the conversion and the removal of old i2c binding, at the same time in 2.6.31 merge window, reverting is much harder.
So, I agree with that the removal of old i2c binding can be postponed to 2.6.31. It can be done on linux-next from now on, and we can keep watching. But, this small conversion patch can be more easily managed in 2.6.30.
thanks,
Takashi
Hi Paul,
On Wed, 22 Apr 2009 22:56:52 +1000, Paul Mackerras wrote:
Jean Delvare writes:
We're past -rc3 because people discuss instead of testing my patches. Otherwise everything would be merged already.
Well, no. The first conversion patch that I saw was posted after the merge window had already closed, on 8 April.
I had the hope that the developers/maintainers involved would not ignore the warnings and had patches ready, that would go to Linus during the merge window. This didn't happen, call me naive if you want. So right after rc1 I started working on the conversion myself.
And really, these changes (sound drivers) don't qualify as disruptive. You might argue about the thermal management driver changes if you want. But sound drivers, nothing bad will happen if they accidentally break.
That's what we call a "regression". :)
Regressions happen all the time, no matter how hard we all try to avoid them. As long as they are fixed before -final, this isn't a big problem though.
But linux-next will only build-test the code. That I have already done, so it really doesn't buy my anything. Developers (including me) don't look at warnings in linux-next, and I don't think linux-next gets a lot of testing.
If you remove the legacy interfaces then even a build test will point out the drivers that still need to be converted. :)
I don't consider breaking the linux-next build a good practice, sorry. I suspect this tree doesn't get a lot of testing, breaking it isn't going to improve the situation. Not to mention that this will make Stephen's life harder, which isn't my goal. I do have a list of drivers that aren't yet converted upstream, I don't need linux-next to tell me. Except for new drivers, of course, there I agree your strategy has value (but has a flaw, see below.)
Also, I can't push all untested patches to linux-next. In particular the 4 patches that touch thermal management on powermac, need to be tested successfully by at least one person before I can push them. You did test the therm_pm72 patch, and I thank you for that, so that one is in linux-next, but the other 3 ones need testing.
So, really, you're trying to solve the wrong problem. Whether the patches go to Linus now or in the next merge window, doesn't really matter.
It does matter, actually.
And 2.6.31 isn't the kernel to remove an interface which was scheduled for removal in 2.6.29 and then 2.6.30 and which is blocking the development of features people need badly.
What's so special about 2.6.30 that it matters whether the legacy interfaces are still there or not?
3 months ago, people told me "what's so special about 2.6.29". The result is that we made very little progress and the legacy interface was still used by 10 (non-staging) drivers in 2.6.30-rc1. There's nothing special about 2.6.30 other than the fact that I am fed up waiting for all drivers to drop using a deprecated API so that I can remove it. Again, further i2c developments are blocked by the existence of this deprecated API.
As for blocking development, you can remove the legacy interfaces today in your next branch and get on with development immediately.
No, I can't, that would break the linux-next build, as explained above. I would also have to include all the driver conversion patches there. The problem is that this still touches drivers/macintosh, drivers/media/video and sound/ppc at the moment, each of which is supposed to be maintained in a different tree. If I include these patches in my i2c tree, chances of collisions with other trees are big, which means more work for Stephen. If the patches are instead included in their respective trees (as Takashi just did for sound/ppc), you avoid the collisions, but then I can't remove the legacy API in linux-next, because the i2c tree is listed relatively early, so bisecting (or just incrementally building) linux-next would fail horribly.
So, as you can see, the problem you think is so easily solved by linux-next, isn't. The only thing I can do at this point is keep the patches in a local tree and point other interested developers thereto. Basically I'll tell them "you need to use linux-next plus a number of public patches that can't go to linux-next plus the development patches we are discussing." This makes the work and discussions about further developments much more difficult, and results in zero testing outside the development group, which makes it unclear if they will be ready for 2.6.31.
Compare this with the case where all driver conversions are already in Linus' tree (even without removing the legacy interface): I can put the legacy interface removal and all the development patches in linux-next and interested people can test just this, and this is build tested on several architectures, and possibly even runtime-tested by a few random users. This is way less work for everyone and a much better quality in the end.
So the "blocking development" argument has zero relevance to what goes into Linus' tree for 2.6.30. Even if you got the legacy interfaces removed for 2.6.30 you still wouldn't be able to get any new features based on that into 2.6.30.
But I would be able to get the new features in 2.6.31. With your plan, I doubt this can happen.
And this is a particularly bad time to be hastily trying to get powermac driver changes upstream, since Ben H. is on vacation.
Yes, that I admit is pretty bad luck :(
So, once again, can powermac developers/users please test my patches?
Can we compromise on this? I'll do everything I reasonably can to help you get the powermac driver patches tested and working before the 2.6.31 merge window, if you agree to leave the drivers and interfaces in Linus' tree as they are for 2.6.30.
"Before the 2.6.31 merge window" is way too late if I want any i2c development to make it into 2.6.31 as well. The conversions must go in linux-next as soon as possible. Even that isn't ideal as I explained above, but that's the bare minimum to make sure everything (driver conversions, interface removal and i2c developments) has a chance to be ready for 2.6.31.
Note that I did get some more test results meanwhile. Johannes Berg was able to test the windfarm drivers conversion, and Andreas Schwab tested the therm_adt746x conversion, both successfully. So the only two drivers which didn't get any testing at this point are keywest (snd-powermac) and therm_windtunnel. That being said, more testing for every driver is certainly welcome. You can check the current status at: http://i2c.wiki.kernel.org/index.php/Legacy_drivers_to_be_converted
I agree to stop pushing driver conversions to Linus for 2.6.30, which means the legacy API will stay there. Some drivers have already been converted though (go7007 in rc3) and some conversions are scheduled to go to Linus already (sound drivers in rc4, Takashi said.) This means 7 remaining unconverted drivers in 2.6.30 (6 powermac thermal management drivers, and ir-kbd-i2c.)
At Mon, 20 Apr 2009 22:56:59 +0200, Jean Delvare wrote:
The legacy i2c binding model is going away soon, so convert the ppc keywest sound driver to the new model or it will break.
Signed-off-by: Jean Delvare khali@linux-fr.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.
I did not get any test report for this one, but it's relatively simple so I am confident I got it right.
Applied this one, too. Thanks.
Takashi
On Tue, 21 Apr 2009 08:31:00 +0200, Takashi Iwai wrote:
At Mon, 20 Apr 2009 22:56:59 +0200, Jean Delvare wrote:
The legacy i2c binding model is going away soon, so convert the ppc keywest sound driver to the new model or it will break.
Signed-off-by: Jean Delvare khali@linux-fr.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.
I did not get any test report for this one, but it's relatively simple so I am confident I got it right.
Applied this one, too. Thanks.
Thanks Takashi.
BTW, I forgot to mention again that the new i2c binding model is functional since kernel 2.6.25, so for the external alsa driver tree you will want to guard these changes with appropriate ifdef magic.
At Tue, 21 Apr 2009 11:23:00 +0200, Jean Delvare wrote:
On Tue, 21 Apr 2009 08:31:00 +0200, Takashi Iwai wrote:
At Mon, 20 Apr 2009 22:56:59 +0200, Jean Delvare wrote:
The legacy i2c binding model is going away soon, so convert the ppc keywest sound driver to the new model or it will break.
Signed-off-by: Jean Delvare khali@linux-fr.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.
I did not get any test report for this one, but it's relatively simple so I am confident I got it right.
Applied this one, too. Thanks.
Thanks Takashi.
BTW, I forgot to mention again that the new i2c binding model is functional since kernel 2.6.25, so for the external alsa driver tree you will want to guard these changes with appropriate ifdef magic.
I thought some patch (applied on 2.6.30) was needed to get them working properly? Anyway, I already added ifdef in alsa-driver external tree to make 2.6.29 and earlier building with the old i2c code.
thanks,
Takashi
On Tue, 21 Apr 2009 11:23:00 +0200, Jean Delvare wrote:
On Tue, 21 Apr 2009 08:31:00 +0200, Takashi Iwai wrote:
At Mon, 20 Apr 2009 22:56:59 +0200, Jean Delvare wrote:
The legacy i2c binding model is going away soon, so convert the ppc keywest sound driver to the new model or it will break.
Signed-off-by: Jean Delvare khali@linux-fr.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.
I did not get any test report for this one, but it's relatively simple so I am confident I got it right.
Applied this one, too. Thanks.
Thanks Takashi.
BTW, I forgot to mention again that the new i2c binding model is functional since kernel 2.6.25, so for the external alsa driver tree you will want to guard these changes with appropriate ifdef magic.
Err, make that 2.6.30, sorry. While the infrastructure was there since 2.6.25, the way I converted the sound drivers doesn't fit in what earlier kernels considered acceptable (the checks on which driver methods were implemented was a little too strict IMHO). So the converted drivers can only be used with kernels >= 2.6.30.
participants (3)
-
Jean Delvare -
Paul Mackerras -
Takashi Iwai