[alsa-devel] [PATCH] General fix for Palm27x aSoC driver
Hi,
Firstly, this patch makes the palm27x asoc driver a little more sane. Also, since all affected devices use GPIO95 as AC97_nRESET, this patch sets that properly. Affected are PalmT5, TX and LifeDrive.
Please consider applying, it's against Russells tree. If Eric wants to merge it, I can remake it.
Thanks.
On Sun, Apr 12, 2009 at 06:58:42PM +0200, Marek Vasut wrote:
+static struct platform_device palmld_pxa2xx_pcm = {
- .name = "pxa2xx-pcm",
- .id = -1,
+};
This should follow the pattern that everything else uses with the struct platform_device in devices.c and board files triggering registration of that.
+static struct platform_device palmld_pxa2xx_ac97 = {
- .name = "pxa2xx-ac97",
- .id = -1,
- .dev = {
.platform_data = &palmld_ac97_pdata,- },
+};
No, use pxa_set_ac97_info().
+static struct platform_device palmld_wm9712_codec = {
No, the WM9712 is not a platform device.
On Sunday 12 of April 2009 19:06:26 Mark Brown wrote:
On Sun, Apr 12, 2009 at 06:58:42PM +0200, Marek Vasut wrote:
+static struct platform_device palmld_pxa2xx_pcm = {
- .name = "pxa2xx-pcm",
- .id = -1,
+};
This should follow the pattern that everything else uses with the struct platform_device in devices.c and board files triggering registration of that.
Half of the platforms use static struct, half use struct. Probably someone should send a huge patch to make it consistent. I'd like to have it consistent at least inside the platform file.
+static struct platform_device palmld_pxa2xx_ac97 = {
- .name = "pxa2xx-ac97",
- .id = -1,
- .dev = {
.platform_data = &palmld_ac97_pdata,- },
+};
No, use pxa_set_ac97_info().
How?
+static struct platform_device palmld_wm9712_codec = {
No, the WM9712 is not a platform device.
On Sunday 12 of April 2009 19:20:15 Marek Vasut wrote:
On Sunday 12 of April 2009 19:06:26 Mark Brown wrote:
On Sun, Apr 12, 2009 at 06:58:42PM +0200, Marek Vasut wrote:
+static struct platform_device palmld_pxa2xx_pcm = {
- .name = "pxa2xx-pcm",
- .id = -1,
+};
This should follow the pattern that everything else uses with the struct platform_device in devices.c and board files triggering registration of that.
Half of the platforms use static struct, half use struct. Probably someone should send a huge patch to make it consistent. I'd like to have it consistent at least inside the platform file.
+static struct platform_device palmld_pxa2xx_ac97 = {
- .name = "pxa2xx-ac97",
- .id = -1,
- .dev = {
.platform_data = &palmld_ac97_pdata,- },
+};
No, use pxa_set_ac97_info().
How?
+static struct platform_device palmld_wm9712_codec = {
No, the WM9712 is not a platform device.
btw mio_a701 uses it the same way ... why can I ? Why was the mio code merged then?
Marek Vasut marek.vasut@gmail.com writes:
btw mio_a701 uses it the same way ... why can I ? Why was the mio code merged then?
The pxa_set_ac97_info() was introduced in 2008, June the 10th. The original mio code was submitted in early 2008, in March I think. Do you see now why ?
And that code was recently amended by Eric Miao, because it has not evolved with the introduction of pxa_set_ac97_info() as it should had (blame me). The part of code you're talking about is now gone.
Cheers.
-- Robert
On Sun, Apr 12, 2009 at 07:24:24PM +0200, Marek Vasut wrote:
On Sunday 12 of April 2009 19:20:15 Marek Vasut wrote:
On Sunday 12 of April 2009 19:06:26 Mark Brown wrote:j
No, the WM9712 is not a platform device.
btw mio_a701 uses it the same way ... why can I ? Why was the mio code merged then?
mio_a701 is buggy, the device should be removed from there. I didn't review that code so didn't have a chance to notice that the device was there.
On Sun, Apr 12, 2009 at 07:20:15PM +0200, Marek Vasut wrote:
On Sunday 12 of April 2009 19:06:26 Mark Brown wrote:
+static struct platform_device palmld_pxa2xx_pcm = {
This should follow the pattern that everything else uses with the struct platform_device in devices.c and board files triggering registration of that.
Half of the platforms use static struct, half use struct. Probably someone should send a huge patch to make it consistent. I'd like to have it consistent at least inside the platform file.
The general rule is that if it's a part of the CPU it should go in devices.c - things that aren't doing that are mostly just waiting for cleanup.
+static struct platform_device palmld_pxa2xx_ac97 = {
- .name = "pxa2xx-ac97",
- .id = -1,
- .dev = {
.platform_data = &palmld_ac97_pdata,- },
+};
No, use pxa_set_ac97_info().
How?
Pass the platform data as the argument. This will currently involve merging the two different platform data structures that we have right now.
On Sunday 12 of April 2009 19:58:00 Mark Brown wrote:
On Sun, Apr 12, 2009 at 07:20:15PM +0200, Marek Vasut wrote:
On Sunday 12 of April 2009 19:06:26 Mark Brown wrote:
+static struct platform_device palmld_pxa2xx_pcm = {
This should follow the pattern that everything else uses with the struct platform_device in devices.c and board files triggering registration of that.
Half of the platforms use static struct, half use struct. Probably someone should send a huge patch to make it consistent. I'd like to have it consistent at least inside the platform file.
The general rule is that if it's a part of the CPU it should go in devices.c - things that aren't doing that are mostly just waiting for cleanup.
+static struct platform_device palmld_pxa2xx_ac97 = {
- .name = "pxa2xx-ac97",
- .id = -1,
- .dev = {
.platform_data = &palmld_ac97_pdata,- },
+};
No, use pxa_set_ac97_info().
How?
Pass the platform data as the argument. This will currently involve merging the two different platform data structures that we have right now.
OK, I see, thanks :-) Will you be happier with this patch then ?
On Sunday 12 of April 2009 20:51:19 Marek Vasut wrote:
On Sunday 12 of April 2009 19:58:00 Mark Brown wrote:
On Sun, Apr 12, 2009 at 07:20:15PM +0200, Marek Vasut wrote:
On Sunday 12 of April 2009 19:06:26 Mark Brown wrote:
+static struct platform_device palmld_pxa2xx_pcm = {
This should follow the pattern that everything else uses with the struct platform_device in devices.c and board files triggering registration of that.
Half of the platforms use static struct, half use struct. Probably someone should send a huge patch to make it consistent. I'd like to have it consistent at least inside the platform file.
The general rule is that if it's a part of the CPU it should go in devices.c - things that aren't doing that are mostly just waiting for cleanup.
+static struct platform_device palmld_pxa2xx_ac97 = {
- .name = "pxa2xx-ac97",
- .id = -1,
- .dev = {
.platform_data = &palmld_ac97_pdata,- },
+};
No, use pxa_set_ac97_info().
How?
Pass the platform data as the argument. This will currently involve merging the two different platform data structures that we have right now.
OK, I see, thanks :-) Will you be happier with this patch then ?
So is it ok this way ? I'd be glad to fix the problem ASAP.
On Sun, Apr 12, 2009 at 08:51:19PM +0200, Marek Vasut wrote:
+static struct pxa2xx_ac97_platform_data palmld_ac97_pdata = {
- .reset_gpio = 95,
+};
The type of this will need changing to reflect the patch that got merged for this but other than that minor point this approach is fine.
On Tuesday 14 of April 2009 21:50:36 Mark Brown wrote:
On Sun, Apr 12, 2009 at 08:51:19PM +0200, Marek Vasut wrote:
+static struct pxa2xx_ac97_platform_data palmld_ac97_pdata = {
- .reset_gpio = 95,
+};
The type of this will need changing to reflect the patch that got merged for this but other than that minor point this approach is fine.
OK, shall I change it and resend (ps. to what if you dont mind telling me?) ? Also, do you want to push it through also tree or ARM tree ? I'm for the second option as it's more of a bugfix suitable for that tree.
Thanks
On Wed, Apr 15, 2009 at 5:42 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Tuesday 14 of April 2009 21:50:36 Mark Brown wrote:
On Sun, Apr 12, 2009 at 08:51:19PM +0200, Marek Vasut wrote:
+static struct pxa2xx_ac97_platform_data palmld_ac97_pdata = {
- .reset_gpio = 95,
+};
The type of this will need changing to reflect the patch that got merged for this but other than that minor point this approach is fine.
OK, shall I change it and resend (ps. to what if you dont mind telling me?) ? Also, do you want to push it through also tree or ARM tree ? I'm for the second option as it's more of a bugfix suitable for that tree.
Sorry, late on this. The changes to the platform part look OK to me, and some minor things you may have another look:
-static int __init palm27x_asoc_init(void) +static int palm27x_asoc_probe(struct platform_device *pdev)
__devinit
{ int ret;
@@ -208,6 +208,10 @@ static int __init palm27x_asoc_init(void) machine_is_palmld())) return -ENODEV;
- if (pdev->dev.platform_data)
palm27x_ep_gpio = ((struct palm27x_asoc_info *)(pdev->dev.platform_data))->jack_gpio;
This is not so readable, I'd prefer to introduce a variable for the 'struct palm27x_asoc_info *' pointer.
ret = gpio_request(palm27x_ep_gpio, "Headphone Jack"); if (ret) return ret;
On Wednesday 15 of April 2009 04:23:49 Eric Miao wrote:
On Wed, Apr 15, 2009 at 5:42 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Tuesday 14 of April 2009 21:50:36 Mark Brown wrote:
On Sun, Apr 12, 2009 at 08:51:19PM +0200, Marek Vasut wrote:
+static struct pxa2xx_ac97_platform_data palmld_ac97_pdata = {
- .reset_gpio = 95,
+};
The type of this will need changing to reflect the patch that got merged for this but other than that minor point this approach is fine.
OK, shall I change it and resend (ps. to what if you dont mind telling me?) ? Also, do you want to push it through also tree or ARM tree ? I'm for the second option as it's more of a bugfix suitable for that tree.
Sorry, late on this. The changes to the platform part look OK to me,
and some minor things you may have another look:
-static int __init palm27x_asoc_init(void) +static int palm27x_asoc_probe(struct platform_device *pdev)
__devinit
Thanks, true, will revise it later today and resend ...
{ int ret;
@@ -208,6 +208,10 @@ static int __init palm27x_asoc_init(void) machine_is_palmld())) return -ENODEV;
- if (pdev->dev.platform_data)
palm27x_ep_gpio = ((struct palm27x_asoc_info *)(pdev->dev.platform_data))->jack_gpio;This is not so readable, I'd prefer to introduce a variable for the 'struct palm27x_asoc_info *' pointer.
Come on, we are not doing the kernel only for ub...u so this should be OK for everyone who can code in C. :-)
ret = gpio_request(palm27x_ep_gpio, "Headphone Jack"); if (ret) return ret;
On Wed, Apr 15, 2009 at 1:00 PM, Marek Vasut marek.vasut@gmail.com wrote:
On Wednesday 15 of April 2009 04:23:49 Eric Miao wrote:
On Wed, Apr 15, 2009 at 5:42 AM, Marek Vasut marek.vasut@gmail.com wrote:
On Tuesday 14 of April 2009 21:50:36 Mark Brown wrote:
On Sun, Apr 12, 2009 at 08:51:19PM +0200, Marek Vasut wrote:
+static struct pxa2xx_ac97_platform_data palmld_ac97_pdata = {
- .reset_gpio = 95,
+};
The type of this will need changing to reflect the patch that got merged for this but other than that minor point this approach is fine.
OK, shall I change it and resend (ps. to what if you dont mind telling me?) ? Also, do you want to push it through also tree or ARM tree ? I'm for the second option as it's more of a bugfix suitable for that tree.
Sorry, late on this. The changes to the platform part look OK to me,
and some minor things you may have another look:
-static int __init palm27x_asoc_init(void) +static int palm27x_asoc_probe(struct platform_device *pdev)
__devinit
Thanks, true, will revise it later today and resend ...
{ int ret;
@@ -208,6 +208,10 @@ static int __init palm27x_asoc_init(void) machine_is_palmld())) return -ENODEV;
- if (pdev->dev.platform_data)
- palm27x_ep_gpio = ((struct palm27x_asoc_info *)
- (pdev->dev.platform_data))->jack_gpio;
This is not so readable, I'd prefer to introduce a variable for the 'struct palm27x_asoc_info *' pointer.
Come on, we are not doing the kernel only for ub...u so this should be OK for everyone who can code in C. :-)
OK, I have to admit I'm sometimes a bit nitpicking. But when it comes to the time that your platform_data introduces more members other than jack_gpio, you may remember this thread :-)
Anyway, it's up to you. I don't want myself to be too captious.
ret = gpio_request(palm27x_ep_gpio, "Headphone Jack"); if (ret) return ret;
On Tue, Apr 14, 2009 at 11:42:34PM +0200, Marek Vasut wrote:
OK, shall I change it and resend (ps. to what if you dont mind telling me?) ?
pxa2xx_audio_ops_t
Also, do you want to push it through also tree or ARM tree ? I'm for the second option as it's more of a bugfix suitable for that tree.
ARM.
On Wed, Apr 15, 2009 at 4:21 PM, Mark Brown broonie@sirena.org.uk wrote:
On Tue, Apr 14, 2009 at 11:42:34PM +0200, Marek Vasut wrote:
OK, shall I change it and resend (ps. to what if you dont mind telling me?) ?
pxa2xx_audio_ops_t
Also, do you want to push it through also tree or ARM tree ? I'm for the second option as it's more of a bugfix suitable for that tree.
ARM.
Marek,
Any update?
On Thursday 16 of April 2009 04:43:27 Eric Miao wrote:
On Wed, Apr 15, 2009 at 4:21 PM, Mark Brown broonie@sirena.org.uk wrote:
On Tue, Apr 14, 2009 at 11:42:34PM +0200, Marek Vasut wrote:
OK, shall I change it and resend (ps. to what if you dont mind telling me?) ?
pxa2xx_audio_ops_t
Also, do you want to push it through also tree or ARM tree ? I'm for the second option as it's more of a bugfix suitable for that tree.
ARM.
Marek,
Any update?
Sorry, LinuxEXPO takes place here so my sleep amount is reduced to about three hours a day ...
I'll try asap (I hope today), sorry for the delay.
On Thursday 16 of April 2009 06:45:17 Marek Vasut wrote:
On Thursday 16 of April 2009 04:43:27 Eric Miao wrote:
On Wed, Apr 15, 2009 at 4:21 PM, Mark Brown broonie@sirena.org.uk wrote:
On Tue, Apr 14, 2009 at 11:42:34PM +0200, Marek Vasut wrote:
OK, shall I change it and resend (ps. to what if you dont mind telling me?) ?
pxa2xx_audio_ops_t
Also, do you want to push it through also tree or ARM tree ? I'm for the second option as it's more of a bugfix suitable for that tree.
ARM.
Marek,
Any update?
Sorry, LinuxEXPO takes place here so my sleep amount is reduced to about three hours a day ...
I'll try asap (I hope today), sorry for the delay.
Ok, this one should be fine with you
On Friday 17 of April 2009 11:42:08 Marek Vasut wrote:
On Thursday 16 of April 2009 06:45:17 Marek Vasut wrote:
On Thursday 16 of April 2009 04:43:27 Eric Miao wrote:
On Wed, Apr 15, 2009 at 4:21 PM, Mark Brown broonie@sirena.org.uk
wrote:
On Tue, Apr 14, 2009 at 11:42:34PM +0200, Marek Vasut wrote:
OK, shall I change it and resend (ps. to what if you dont mind telling me?) ?
pxa2xx_audio_ops_t
Also, do you want to push it through also tree or ARM tree ? I'm for the second option as it's more of a bugfix suitable for that tree.
ARM.
Marek,
Any update?
Sorry, LinuxEXPO takes place here so my sleep amount is reduced to about three hours a day ...
I'll try asap (I hope today), sorry for the delay.
Ok, this one should be fine with you
Any update from your side?
participants (4)
-
Eric Miao -
Marek Vasut -
Mark Brown -
Robert Jarzmik