[alsa-devel] [PATCH] Palm interrupt driven touchscreen driver
Hi,
this patch adds interrupt driven touchscreen driver. It's an addition to mainstone-wm97xx, but does some reorganization of the code there.
Mark, it's the same patch we discussed on IRC, so it should be OK.
Please consider applying, it's against Eric's tree.
Thanks
On Thu, Jun 4, 2009 at 5:41 AM, Marek Vasut marek.vasut@gmail.com wrote:
Hi,
this patch adds interrupt driven touchscreen driver. It's an addition to mainstone-wm97xx, but does some reorganization of the code there.
Mark, it's the same patch we discussed on IRC, so it should be OK.
Mark, the driver looks generic enough to me, is there some specific reason this remains named "mainstone-wm97xx.c"?
I didn't look into the code too much, yet I'm wondering if it's possible to come up with a platform_data to cover the difference of boards and make this driver generic.
Eric Miao wrote:
Mark, the driver looks generic enough to me, is there some specific reason this remains named "mainstone-wm97xx.c"?
I didn't look into the code too much, yet I'm wondering if it's possible to come up with a platform_data to cover the difference of boards and make this driver generic.
eseries can use this idea IIRC...
On Thu, Jun 04, 2009 at 09:35:17AM +0800, Eric Miao wrote:
On Thu, Jun 4, 2009 at 5:41 AM, Marek Vasut marek.vasut@gmail.com wrote:
Mark, it's the same patch we discussed on IRC, so it should be OK.
Mark, the driver looks generic enough to me, is there some specific reason this remains named "mainstone-wm97xx.c"?
No, and in fact I've asked Marek in some off-list discussions to do something along those lines and/or remove the mainstone code entirely but he doesn't seem keen on it for some reason. Now we have more widely distributed platforms merged I see no reason to keep the Mainstone code in mainline.
I didn't look into the code too much, yet I'm wondering if it's possible to come up with a platform_data to cover the difference of boards and make this driver generic.
As I've said a number of times before AC97 does not support platform data and the ALSA guys blocked adding it.
On Thu, 4 Jun 2009, Mark Brown wrote:
to come up with a platform_data to cover the difference of boards and make this driver generic.
As I've said a number of times before AC97 does not support platform data and the ALSA guys blocked adding it.
We're not blocking adding more data to AC97 structures. We just blocked adding new variable when it's not used.
In other words - send a driver code using this variable with the variable addition to the ALSA AC97 structure.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
On Thu, Jun 04, 2009 at 11:09:34AM +0200, Jaroslav Kysela wrote:
On Thu, 4 Jun 2009, Mark Brown wrote:
As I've said a number of times before AC97 does not support platform data and the ALSA guys blocked adding it.
We're not blocking adding more data to AC97 structures. We just blocked adding new variable when it's not used.
There was a very strong pushback against the idea of adding a void * to the structure, which is what I'd be looking for.
In other words - send a driver code using this variable with the variable addition to the ALSA AC97 structure.
IIRC the original posting had something along with it? As far as examples go things like drivers/power/wm97xx_battery.c are already in tree and pretty much ready to use platform data if it became available.
At Thu, 4 Jun 2009 10:37:37 +0100, Mark Brown wrote:
On Thu, Jun 04, 2009 at 11:09:34AM +0200, Jaroslav Kysela wrote:
On Thu, 4 Jun 2009, Mark Brown wrote:
As I've said a number of times before AC97 does not support platform data and the ALSA guys blocked adding it.
We're not blocking adding more data to AC97 structures. We just blocked adding new variable when it's not used.
There was a very strong pushback against the idea of adding a void * to the structure, which is what I'd be looking for.
Yes, I was against the void pointer because there is no guarantee of the data-type correctness between the data and the user. The normal drvdata is fine because the creator and the user are the same. But, the proposal of private_driver_data (of the last one, at least) is no such a scenario.
Takashi
On Thu, Jun 04, 2009 at 11:44:22AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
There was a very strong pushback against the idea of adding a void * to the structure, which is what I'd be looking for.
Yes, I was against the void pointer because there is no guarantee of the data-type correctness between the data and the user. The normal drvdata is fine because the creator and the user are the same. But, the proposal of private_driver_data (of the last one, at least) is no such a scenario.
Do you have a pointer to this proposal? I don't remember it and my google-fu is weak today.
At Thu, 4 Jun 2009 11:38:07 +0100, Mark Brown wrote:
On Thu, Jun 04, 2009 at 11:44:22AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
There was a very strong pushback against the idea of adding a void * to the structure, which is what I'd be looking for.
Yes, I was against the void pointer because there is no guarantee of the data-type correctness between the data and the user. The normal drvdata is fine because the creator and the user are the same. But, the proposal of private_driver_data (of the last one, at least) is no such a scenario.
Do you have a pointer to this proposal? I don't remember it and my google-fu is weak today.
I couldn't find it. But IIRC, the original patch was to add "int irq" field to struct snd_ac97. I would accept it because it can do any harm.
But, adding a generic "void *driver_data" instead of "int irq" can play a bad game. It can be dangerous when the creator and the user of this pointer are different drivers. That was my argument.
thanks,
Takashi
On Fri, Jun 05, 2009 at 11:16:12AM +0200, Takashi Iwai wrote:
I couldn't find it. But IIRC, the original patch was to add "int irq" field to struct snd_ac97. I would accept it because it can do any harm.
Ah, yes - I do remember that proposal. The problem with that is that it doesn't scale up to all the platform data you might want to pass in. The WM97xx battery and touchscreen drivers are the main things I'm interested in here - the platform data for the WM97xx touch driver is struct wm97xx_mach_ops in include/linux/wm97xx.h for example.
But, adding a generic "void *driver_data" instead of "int irq" can play a bad game. It can be dangerous when the creator and the user of this pointer are different drivers. That was my argument.
There is potential risk involved in these interfaces but it's the standard idiom for doing this on embedded systems. The buses definitely don't want to know anything about the random data that drivers may need and since you're not dealing with pluggable or variable hardware in the same way as you do on PCs the risks of mismatched data aren't such a big deal - you'd generally only be able to encounter problems if you're building your own kernel.
At Fri, 5 Jun 2009 10:30:02 +0100, Mark Brown wrote:
On Fri, Jun 05, 2009 at 11:16:12AM +0200, Takashi Iwai wrote:
I couldn't find it. But IIRC, the original patch was to add "int irq" field to struct snd_ac97. I would accept it because it can do any harm.
Ah, yes - I do remember that proposal. The problem with that is that it doesn't scale up to all the platform data you might want to pass in. The WM97xx battery and touchscreen drivers are the main things I'm interested in here - the platform data for the WM97xx touch driver is struct wm97xx_mach_ops in include/linux/wm97xx.h for example.
But, adding a generic "void *driver_data" instead of "int irq" can play a bad game. It can be dangerous when the creator and the user of this pointer are different drivers. That was my argument.
There is potential risk involved in these interfaces but it's the standard idiom for doing this on embedded systems. The buses definitely don't want to know anything about the random data that drivers may need and since you're not dealing with pluggable or variable hardware in the same way as you do on PCs the risks of mismatched data aren't such a big deal - you'd generally only be able to encounter problems if you're building your own kernel.
Well, but you know that can be broken :)
BTW, can't it be simply driver_data/platform_data of ac97.dev...?
Takashi
On Fri, Jun 05, 2009 at 12:05:11PM +0200, Takashi Iwai wrote:
BTW, can't it be simply driver_data/platform_data of ac97.dev...?
We'd need to add the void * to snd_ac97_template too so that the data is initialised before snd_ac97_mixer() instantiates the device but that's the only issue I can see. If you're happy with that approach I can send some patches for it?
At Fri, 5 Jun 2009 11:20:37 +0100, Mark Brown wrote:
On Fri, Jun 05, 2009 at 12:05:11PM +0200, Takashi Iwai wrote:
BTW, can't it be simply driver_data/platform_data of ac97.dev...?
We'd need to add the void * to snd_ac97_template too so that the data is initialised before snd_ac97_mixer() instantiates the device but that's the only issue I can see. If you're happy with that approach I can send some patches for it?
Just setting it after calling snd_ac97_mixer() doesn't suffice? It's always NULL as default.
Takashi
On Fri, Jun 05, 2009 at 12:38:05PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
We'd need to add the void * to snd_ac97_template too so that the data is initialised before snd_ac97_mixer() instantiates the device but that's the only issue I can see. If you're happy with that approach I can send some patches for it?
Just setting it after calling snd_ac97_mixer() doesn't suffice? It's always NULL as default.
Ah, should be - I'd misremembered how much snd_device_new() was doing when looking at the snd_ac97_mixer() code.
On Thu, Jun 04, 2009 at 09:54:07AM +0100, Mark Brown wrote:
Now we have more widely distributed platforms merged I see no reason to keep the Mainstone code in mainline.
Why not? So what do those of us with mainstone platforms but not these "more widely distributed platforms" use instead?
On Thu, Jun 04, 2009 at 10:13:29AM +0100, Russell King - ARM Linux wrote:
On Thu, Jun 04, 2009 at 09:54:07AM +0100, Mark Brown wrote:
Now we have more widely distributed platforms merged I see no reason to keep the Mainstone code in mainline.
Why not? So what do those of us with mainstone platforms but not these "more widely distributed platforms" use instead?
This isn't a standard fit module for the Mainstone - it's the Mainstone with the default audio plugin module replaced by a Wolfson plugin module. If you just have a standard Mainstone you'd need a different accelerated touch driver for the relevant AC97 controller.
If there are people out there with these setups then obviously the driver is worth keeping in mainline, but I'm not aware of any active users.
participants (7)
-
Eric Miao -
Ian Molton -
Jaroslav Kysela -
Marek Vasut -
Mark Brown -
Russell King - ARM Linux -
Takashi Iwai