[RFC] add a "private" field to the ac97 struct

[RFC] add a "private" field to the ac97 struct

[Sebastian Siewior]

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 */
-- 
1.5.3.5

Re: [RFC] add a "private" field to the ac97 struct

[William Pitcock]

[-- Attachment #1.1: Type: text/plain, Size: 2639 bytes --]

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 */

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC] add a "private" field to the ac97 struct

[Sebastian Siewior]

* 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);
-- 
1.5.3.5

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?

>William

Sebastian

Re: [RFC] add a "private" field to the ac97 struct

[Mark Brown]

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;a=summary