[alsa-devel] [RFC] add a "private" field to the ac97 struct
I have a PowerPC board with an AC97 controller and that one has an UCB1400 attached. Currently the UCB1400 driver discovers the interrupt via probing. This doesn't really work here. The resources are read from the device tree. I haven't found any other driver besides ucb1400_ts that uses the ac97_bus_type so I can't check where others get their HW information from.
My proposal is to introduce a new field in struct snd_ac97 where the AC97 driver can leave the required HW info regarding the attached (in my case the interrupt for the ucb1400) before calling snd_card_register(). I attached just a single int field since I am not aware if multiple devices / resource are realistic / possible. In that case maybe a void * or struct platform_device would be a better choice.
Signed-off-by: Sebastian Siewior sebastian@breakpoint.cc --- drivers/input/touchscreen/ucb1400_ts.c | 13 ++++++++----- include/sound/ac97_codec.h | 2 ++ 2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/input/touchscreen/ucb1400_ts.c b/drivers/input/touchscreen/ucb1400_ts.c index 607f993..95d1fea 100644 --- a/drivers/input/touchscreen/ucb1400_ts.c +++ b/drivers/input/touchscreen/ucb1400_ts.c @@ -492,11 +492,14 @@ static int ucb1400_ts_probe(struct device *dev) goto err_free_devs; }
- error = ucb1400_detect_irq(ucb); - if (error) { - printk(KERN_ERR "UCB1400: IRQ probe failed\n"); - goto err_free_devs; - } + if (!ucb->ac97->device_irq) { + error = ucb1400_detect_irq(ucb); + if (error) { + printk(KERN_ERR "UCB1400: IRQ probe failed\n"); + goto err_free_devs; + } + } else + ucb->irq = ucb->ac97->device_irq;
error = request_irq(ucb->irq, ucb1400_hard_irq, IRQF_TRIGGER_RISING, "UCB1400", ucb); diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h index 0148058..bf920a2 100644 --- a/include/sound/ac97_codec.h +++ b/include/sound/ac97_codec.h @@ -474,6 +474,8 @@ struct snd_ac97 { struct snd_ac97_build_ops * build_ops; void *private_data; void (*private_free) (struct snd_ac97 *ac97); + /* device attached to the AC97 bus */ + int device_irq; /* --- */ struct snd_ac97_bus *bus; struct pci_dev *pci; /* assigned PCI device - used for quirks */
Hi,
Why not modify ucb1400_detect_irq() to get an openfirmware handle on PowerPC and read the resource tree at that time? This seems like a better approach.
William
On Thu, 2008-03-06 at 17:42 +0100, Sebastian Siewior wrote:
I have a PowerPC board with an AC97 controller and that one has an UCB1400 attached. Currently the UCB1400 driver discovers the interrupt via probing. This doesn't really work here. The resources are read from the device tree. I haven't found any other driver besides ucb1400_ts that uses the ac97_bus_type so I can't check where others get their HW information from.
My proposal is to introduce a new field in struct snd_ac97 where the AC97 driver can leave the required HW info regarding the attached (in my case the interrupt for the ucb1400) before calling snd_card_register(). I attached just a single int field since I am not aware if multiple devices / resource are realistic / possible. In that case maybe a void * or struct platform_device would be a better choice.
Signed-off-by: Sebastian Siewior sebastian@breakpoint.cc
drivers/input/touchscreen/ucb1400_ts.c | 13 ++++++++----- include/sound/ac97_codec.h | 2 ++ 2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/input/touchscreen/ucb1400_ts.c b/drivers/input/touchscreen/ucb1400_ts.c index 607f993..95d1fea 100644 --- a/drivers/input/touchscreen/ucb1400_ts.c +++ b/drivers/input/touchscreen/ucb1400_ts.c @@ -492,11 +492,14 @@ static int ucb1400_ts_probe(struct device *dev) goto err_free_devs; }
- error = ucb1400_detect_irq(ucb);
- if (error) {
printk(KERN_ERR "UCB1400: IRQ probe failed\n");goto err_free_devs;- }
if (!ucb->ac97->device_irq) {
error = ucb1400_detect_irq(ucb);if (error) {printk(KERN_ERR "UCB1400: IRQ probe failed\n");goto err_free_devs;}} else
ucb->irq = ucb->ac97->device_irq;error = request_irq(ucb->irq, ucb1400_hard_irq, IRQF_TRIGGER_RISING, "UCB1400", ucb);
diff --git a/include/sound/ac97_codec.h b/include/sound/ac97_codec.h index 0148058..bf920a2 100644 --- a/include/sound/ac97_codec.h +++ b/include/sound/ac97_codec.h @@ -474,6 +474,8 @@ struct snd_ac97 { struct snd_ac97_build_ops * build_ops; void *private_data; void (*private_free) (struct snd_ac97 *ac97);
- /* device attached to the AC97 bus */
- int device_irq; /* --- */ struct snd_ac97_bus *bus; struct pci_dev *pci; /* assigned PCI device - used for quirks */
* William Pitcock | 2008-03-06 11:12:37 [-0600]:
Hi,
Hi,
Why not modify ucb1400_detect_irq() to get an openfirmware handle on PowerPC and read the resource tree at that time? This seems like a better approach.
That was my first approach:
--- drivers/input/touchscreen/Kconfig | 7 ++++ drivers/input/touchscreen/ucb1400_ts.c | 57 +++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletions(-)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig index 90e8e92..8f4d752 100644 --- a/drivers/input/touchscreen/Kconfig +++ b/drivers/input/touchscreen/Kconfig @@ -185,6 +185,13 @@ config TOUCHSCREEN_UCB1400 To compile this driver as a module, choose M here: the module will be called ucb1400_ts.
+config TOUCHSCREEN_UCB1400_OF + bool "Get device informations via the device tree" + depends on TOUCHSCREEN_UCB1400 && OF + help + Select this option if you want to obtain interrupt information from + the device tree instead of auto probing. + config TOUCHSCREEN_USB_COMPOSITE tristate "USB Touchscreen Driver" depends on USB_ARCH_HAS_HCD diff --git a/drivers/input/touchscreen/ucb1400_ts.c b/drivers/input/touchscreen/ucb1400_ts.c index 607f993..9337352 100644 --- a/drivers/input/touchscreen/ucb1400_ts.c +++ b/drivers/input/touchscreen/ucb1400_ts.c @@ -26,6 +26,11 @@ #include <linux/kthread.h> #include <linux/freezer.h>
+#ifdef CONFIG_TOUCHSCREEN_UCB1400_OF +#include <linux/of.h> +#include <linux/of_platform.h> +#endif + #include <sound/core.h> #include <sound/ac97_codec.h>
@@ -418,6 +423,9 @@ static int ucb1400_ts_resume(struct device *dev) #define NO_IRQ 0 #endif
+#ifdef CONFIG_TOUCHSCREEN_UCB1400_OF +static int ucb1400_irq; +#else /* * Try to probe our interrupt, rather than relying on lots of * hard-coded machine dependencies. @@ -467,6 +475,7 @@ static int ucb1400_detect_irq(struct ucb1400 *ucb)
return 0; } +#endif
static int ucb1400_ts_probe(struct device *dev) { @@ -491,12 +500,15 @@ static int ucb1400_ts_probe(struct device *dev) error = -ENODEV; goto err_free_devs; } - +#ifdef CONFIG_TOUCHSCREEN_UCB1400_OF + ucb->irq = ucb1400_irq; +#else error = ucb1400_detect_irq(ucb); if (error) { printk(KERN_ERR "UCB1400: IRQ probe failed\n"); goto err_free_devs; } +#endif
error = request_irq(ucb->irq, ucb1400_hard_irq, IRQF_TRIGGER_RISING, "UCB1400", ucb); @@ -562,14 +574,57 @@ static struct device_driver ucb1400_ts_driver = { .resume = ucb1400_ts_resume, };
+#ifdef CONFIG_TOUCHSCREEN_UCB1400_OF +static int __devinit of_ucb_probe(struct of_device *dev, + const struct of_device_id *match) +{ + struct device_node *dp = dev->node; + + ucb1400_irq = irq_of_parse_and_map(dp, 0); + if (ucb1400_irq == IRQ_NONE) + return -ENODEV; + + return driver_register(&ucb1400_ts_driver); +} + +static int of_ucb_remove(struct of_device *dev) +{ + driver_unregister(&ucb1400_ts_driver); + return 0; +} + +static struct of_device_id of_ucb_match[] = { + { + .compatible = "Phillips-ucb1400_ts", + }, + { }, +}; +MODULE_DEVICE_TABLE(of, of_ucb_match); + +static struct of_platform_driver of_ucb1400_ts_driver = { + .name = "of-ucb1400_ts", + .match_table = of_ucb_match, + .probe = of_ucb_probe, + .remove = of_ucb_remove, +}; +#endif + static int __init ucb1400_ts_init(void) { +#ifdef CONFIG_TOUCHSCREEN_UCB1400_OF + return of_register_platform_driver(&of_ucb1400_ts_driver); +#else return driver_register(&ucb1400_ts_driver); +#endif }
static void __exit ucb1400_ts_exit(void) { +#ifdef CONFIG_TOUCHSCREEN_UCB1400_OF + of_unregister_platform_driver(&of_ucb1400_ts_driver); +#else driver_unregister(&ucb1400_ts_driver); +#endif }
module_param(adcsync, bool, 0444);
On Thu, Mar 06, 2008 at 06:22:25PM +0100, Sebastian Siewior wrote:
The problem is more or less that I can't pass the value to the other probe function and the global variable doesn't look all that pretty. Am I missing something here?
This is also a problem for the wm97xx touchscreen drivers. They have a similar issue with wanting to get platform configuration into a device probed off the AC97 bus with the additional issue that features like syncing with screen redraw will often require code to interface with other hardware in the system. If something is added to the AC97 data it'd be helpful if it were a void * so the wm97xx touch driver could use it to pass a struct with more data.
This is not really an AC97-specific issue - any device discoverd via a bus is going to have a similar thing if it needs to obtain platform data. I don't know how OpenFirmware normally handles these things? It's going to be an issue for other buses like PCI too in many systems, MAC addresses for NICs being the example I've run into myself in a previous life.
In the case of the wm97xx touchscreen we've avoided the problem by having the touch driver register platform devices; this isn't ideal but works well in the systems which are the typical targets for these devices. I had been intending to revisit this issue at some point but it's quite a way down my TODO list.
The wm97xx drivers touch aren't in mainline yet - if you want to have a look the most current upstream submission versions can be found in the upstream branch of:
git://opensource.wolfsonmicro.com/linux-2.6-touch
gitweb:
http://opensource.wolfsonmicro.com/cgi-bin/gitweb.cgi?p=linux-2.6-touch.git;...
participants (3)
-
Mark Brown -
Sebastian Siewior -
William Pitcock