Re: [alsa-devel] [PATCH] PalmTX aSoC sound support
Dne Friday 04 of July 2008 22:26:15 Mark Brown napsal(a):
On Fri, Jul 04, 2008 at 09:49:57PM +0200, Marek Vasut wrote:
Like Liam said, audio patches should be CCed to alsa-devel as well - a
couple of comments from a quick scan through:
- /* add palmtx specific widgets */
- for (i = 0; i < ARRAY_SIZE(palmtx_dapm_widgets); i++)
snd_soc_dapm_new_control(codec, &palmtx_dapm_widgets[i]);Ideally this should use the new snd_soc_dapm_new_controls() function...
- /* set up palmtx specific audio path audio_map */
- for (i = 0; audio_map[i][0] != NULL; i++)
snd_soc_dapm_connect_input(codec, audio_map[i][0],audio_map[i][1], audio_map[i][2]);and this should use snd_soc_dapm_add_routes (which would also mean you could drop the null terminator from audio_map()). Both are queued for merge in the window - see ALSA git, it'll also appear in linux-next.
- ret = platform_device_add(palmtx_snd_device);
- if (!ret)
return 0;- platform_device_put(palmtx_snd_device);
It took me a double take to follow this - it'd be more idiomatic to say:
if (ret != 0) goto put_device;
return 0; put_device:
ok, I applied some alsa patches to my branch of "devel"
Dne Saturday 05 of July 2008 00:19:08 Marek Vasut napsal(a):
Dne Friday 04 of July 2008 22:26:15 Mark Brown napsal(a):
On Fri, Jul 04, 2008 at 09:49:57PM +0200, Marek Vasut wrote:
Like Liam said, audio patches should be CCed to alsa-devel as well - a
couple of comments from a quick scan through:
- /* add palmtx specific widgets */
- for (i = 0; i < ARRAY_SIZE(palmtx_dapm_widgets); i++)
snd_soc_dapm_new_control(codec, &palmtx_dapm_widgets[i]);Ideally this should use the new snd_soc_dapm_new_controls() function...
- /* set up palmtx specific audio path audio_map */
- for (i = 0; audio_map[i][0] != NULL; i++)
snd_soc_dapm_connect_input(codec, audio_map[i][0],audio_map[i][1], audio_map[i][2]);and this should use snd_soc_dapm_add_routes (which would also mean you could drop the null terminator from audio_map()). Both are queued for merge in the window - see ALSA git, it'll also appear in linux-next.
- ret = platform_device_add(palmtx_snd_device);
- if (!ret)
return 0;- platform_device_put(palmtx_snd_device);
It took me a double take to follow this - it'd be more idiomatic to say:
if (ret != 0) goto put_device;
return 0; put_device:
ok, I applied some alsa patches to my branch of "devel"
even better version of the previous one
On Sat, Jul 05, 2008 at 07:17:41AM +0200, Marek Vasut wrote:
even better version of the previous one
Signed-off-by: Marek Vasut marek.vasut@gmail.com
Acked-by: Mark Brown broonie@opensource.wolfsonmicro.com
It might be best to merge this via the ARM tree since it depends on the machine support which is still pending but the Kconfig should prevent anyone actually trying to build it until that happens.
At Mon, 7 Jul 2008 13:29:25 +0100, Mark Brown wrote:
On Sat, Jul 05, 2008 at 07:17:41AM +0200, Marek Vasut wrote:
even better version of the previous one
Signed-off-by: Marek Vasut marek.vasut@gmail.com
Acked-by: Mark Brown broonie@opensource.wolfsonmicro.com
It might be best to merge this via the ARM tree since it depends on the machine support which is still pending but the Kconfig should prevent anyone actually trying to build it until that happens.
Doesn't matter much to me, so just let me know to merge via which tree.
thanks,
Takashi
At Mon, 14 Jul 2008 16:07:58 +0100, Mark Brown wrote:
On Mon, Jul 07, 2008 at 05:58:32PM +0200, Takashi Iwai wrote:
Doesn't matter much to me, so just let me know to merge via which tree.
Either way it needs another spin of the driver so...
OK, it's fine to push it to ALSA tree. But I think it's a bit too late for 2.6.27...
Marek, could you give a proper changelog? I had only a patch.
thanks,
Takashi
Dne Wednesday 16 of July 2008 13:17:53 Takashi Iwai napsal(a):
At Mon, 14 Jul 2008 16:07:58 +0100,
Mark Brown wrote:
On Mon, Jul 07, 2008 at 05:58:32PM +0200, Takashi Iwai wrote:
Doesn't matter much to me, so just let me know to merge via which tree.
Either way it needs another spin of the driver so...
OK, it's fine to push it to ALSA tree. But I think it's a bit too late for 2.6.27...
wasnt merge window for .27 opened just a while ago ?
Marek, could you give a proper changelog? I had only a patch.
I have better (more universal) patch, would you mind waiting for that one ?
thanks,
Takashi
At Wed, 16 Jul 2008 22:10:09 +0200, Marek Vasut wrote:
Dne Wednesday 16 of July 2008 13:17:53 Takashi Iwai napsal(a):
At Mon, 14 Jul 2008 16:07:58 +0100,
Mark Brown wrote:
On Mon, Jul 07, 2008 at 05:58:32PM +0200, Takashi Iwai wrote:
Doesn't matter much to me, so just let me know to merge via which tree.
Either way it needs another spin of the driver so...
OK, it's fine to push it to ALSA tree. But I think it's a bit too late for 2.6.27...
wasnt merge window for .27 opened just a while ago ?
It doesn't mean to allow us to push any unreviewed/untested patches. Trivial or fix patches can go immediately, but a new driver code is a different story. In general, the new driver codes should be merged to the subsystem tree in a few weeks before the merge window.
Marek, could you give a proper changelog? I had only a patch.
I have better (more universal) patch, would you mind waiting for that one ?
Yes.
thanks,
Takashi
On Thu, Jul 17, 2008 at 12:05:43PM +0200, Takashi Iwai wrote:
At Wed, 16 Jul 2008 22:10:09 +0200, Marek Vasut wrote:
Dne Wednesday 16 of July 2008 13:17:53 Takashi Iwai napsal(a):
At Mon, 14 Jul 2008 16:07:58 +0100,
Mark Brown wrote:
On Mon, Jul 07, 2008 at 05:58:32PM +0200, Takashi Iwai wrote:
Doesn't matter much to me, so just let me know to merge via which tree.
Either way it needs another spin of the driver so...
OK, it's fine to push it to ALSA tree. But I think it's a bit too late for 2.6.27...
wasnt merge window for .27 opened just a while ago ?
It doesn't mean to allow us to push any unreviewed/untested patches. Trivial or fix patches can go immediately, but a new driver code is a different story. In general, the new driver codes should be merged to the subsystem tree in a few weeks before the merge window.
We can't moan about it not having been exposed though - it first appeared on July 4th here, and July 5th on alsa-devel.
It has been reviewed by Mark Brown, and you yourself even said "so just let me know to merge via which tree." which sounds to me like you were ready to merge it - but just wanted to know _which_ route it was going to take.
So, to now come back and whinge about it being unreviewed and/or untested and about patches being submitted before the merge window is a little silly, don't you think?
Anyway, my present position with my tree is that I'm not merging anything further into my tree until the remainder of the code queued up previously has been merged, which will only happen when the SPI tree is eventually merged (or I get fed up with waiting for that to happen and choose build time breakage over the correct merge ordering.)
The reason for this is simple - if I push a new tree out, the nice diff versions for non-git users of my tree will vanish, despite there being changes still in there, and I don't want a flood of "where's my changes gone? they aren't in Linus' tree and they aren't in the diffs." questions.
At Fri, 18 Jul 2008 15:49:20 +0100, Russell King - ARM Linux wrote:
On Thu, Jul 17, 2008 at 12:05:43PM +0200, Takashi Iwai wrote:
At Wed, 16 Jul 2008 22:10:09 +0200, Marek Vasut wrote:
Dne Wednesday 16 of July 2008 13:17:53 Takashi Iwai napsal(a):
At Mon, 14 Jul 2008 16:07:58 +0100,
Mark Brown wrote:
On Mon, Jul 07, 2008 at 05:58:32PM +0200, Takashi Iwai wrote:
Doesn't matter much to me, so just let me know to merge via which tree.
Either way it needs another spin of the driver so...
OK, it's fine to push it to ALSA tree. But I think it's a bit too late for 2.6.27...
wasnt merge window for .27 opened just a while ago ?
It doesn't mean to allow us to push any unreviewed/untested patches. Trivial or fix patches can go immediately, but a new driver code is a different story. In general, the new driver codes should be merged to the subsystem tree in a few weeks before the merge window.
We can't moan about it not having been exposed though - it first appeared on July 4th here, and July 5th on alsa-devel.
It has been reviewed by Mark Brown, and you yourself even said "so just let me know to merge via which tree." which sounds to me like you were ready to merge it - but just wanted to know _which_ route it was going to take.
So, to now come back and whinge about it being unreviewed and/or untested and about patches being submitted before the merge window is a little silly, don't you think?
Not quite. Merging to the subsystem tree doesn't mean to push the stuff to Linus tree "immediately" -- especially it's never been tested with that merged tree. And in general, the new stuff has to be built and checked via linux-next and/or mm trees properly before reaching to the Linus tree. So, unless it's properly checked through the whole usual process, one shouldn't expect much that it must be merged so soonish.
Oh, before someone misunderstands again: actually I'm willing to merge his patch soon when it's ready. My comment is whether to push for 2.6.27 now or not.
Anyway, my present position with my tree is that I'm not merging anything further into my tree until the remainder of the code queued up previously has been merged, which will only happen when the SPI tree is eventually merged (or I get fed up with waiting for that to happen and choose build time breakage over the correct merge ordering.)
The reason for this is simple - if I push a new tree out, the nice diff versions for non-git users of my tree will vanish, despite there being changes still in there, and I don't want a flood of "where's my changes gone? they aren't in Linus' tree and they aren't in the diffs." questions.
Well, changes over several areas are sometimes painful.
I still think the "right" order for this kind of changes (addition of new driver depending on a certain architecture) should be: arch-change -> driver-addition. So I've wanted to hear from your first.
Anyway, I don't care much now how it's going. I'll merge the driver part soon once after it's ready. That sounds like the most practical solution.
thanks,
Takashi
On Fri, Jul 18, 2008 at 05:13:09PM +0200, Takashi Iwai wrote:
At Fri, 18 Jul 2008 15:49:20 +0100, Russell King - ARM Linux wrote:
The reason for this is simple - if I push a new tree out, the nice diff versions for non-git users of my tree will vanish, despite there being changes still in there, and I don't want a flood of "where's my changes gone? they aren't in Linus' tree and they aren't in the diffs." questions.
Well, changes over several areas are sometimes painful.
It doesn't have to be - in this particular instance is down to patch 5088/3, which introduced a new #include into pcm027.c, where the file to be included is in the SPI tree.
In hind sight, that change should've been separated into two patches: 1. create the new API and change existing users over 2. add new pcm027 support (reliant on SPI tree)
This would've meant that things could have been arranged so that the _only_ outstanding patch was (2) rather than the existing situation where quite a lot of other peoples changes are currently stuck (because they inadvertently and indirectly depend on (1)).
That's something to watch in the future...
On Sat, Jul 05, 2008 at 12:19:08AM +0200, Marek Vasut wrote:
+/* PalmTX audio map */ +static const char *audio_map[][3] = {
This should be an array of struct snd_soc_dapm_route.
Other than that one minor nit everything looks good.
participants (4)
-
Marek Vasut -
Mark Brown -
Russell King - ARM Linux -
Takashi Iwai