[alsa-devel] [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Jaroslav Kysela]

On Fri, 25 Apr 2008, Takashi Iwai wrote:

> At Fri, 25 Apr 2008 10:23:51 +0200 (CEST),
> Jaroslav Kysela wrote:
> > 
> > On Fri, 25 Apr 2008, Takashi Iwai wrote:
> > 
> > > At Fri, 25 Apr 2008 09:35:47 +0200 (CEST),
> > > Jaroslav Kysela wrote:
> > > > 
> > > > On Fri, 25 Apr 2008, Takashi Iwai wrote:
> > > > 
> > > > > > Sure. I applied the simple 'void *device_private_data' patch, because 
> > > > > > current usage request is really trivial. We can implement complex code to 
> > > > > > handle data for multiple "extra" devices on AC97 bus later.
> > > > > 
> > > > > Actually, it's not "used" yet.  The ucb1000 reads the data but no one
> > > > > stores yet.  And, if its usage request is trivial, we should use "int
> > > > 
> > > > Yes, I hope that the appropriate initialization code will be added to SoC 
> > > > drivers, too.
> > > > 
> > > > > irq" as in the original patch instead of void data and cast.
> > > > 
> > > > But other SoC (or other) drivers might want to pass to extra devices on 
> > > > AC97 bus something different or more complex. Mark Brown already noted
> > > > that. I would keep it as 'void *'.
> > > 
> > > That's the very problem I've been trying to point out.
> > > The void pointer is good if the same driver assigns and casts.  But,
> > > in this case, the allocator and the receiver are different.  Thus,
> > > there is no guarantee that the data type is what you want.  OTOH, if
> > > it's "int irq", this is crystal clear.
> > > 
> > > So, in short:
> > > 
> > > - if only one device needs such data, it should be a strong type like
> > >   "int irq" anyway -- no extra need to cast to void pointer
> > > - if multiple devices need such a pass-away mechanism, then they can
> > >   crash because you have no data type check.  The void pointer is
> > >   dangerous for multiple devices.
> > 
> > I see. In this case, I would propose to add a 32-bit "magic" at the 
> > start of 'void *' data. How about this modification:
> 
> The magic number sounds OK, but funky cast to integer pointer is bad.
> If you have a long or a pointer after int, you can have an alignment
> problem on 64bit archs, for example.

Not really. First 32-bit word has offset 0 and next 64-bit word has offset 
8 on x86-64. So alignment is OK.

Anyway, I agree that with a struct, code is more readable without hidden 
things. Also, drivers can define own struct with magic member as first.

Here is corrected patch:

diff -r e2ff47e8771b include/ac97_codec.h
--- a/include/ac97_codec.h	Fri Apr 25 08:29:05 2008 +0200
+++ b/include/ac97_codec.h	Fri Apr 25 11:43:11 2008 +0200
@@ -407,6 +407,9 @@
 #define AC97_RATES_MIC_ADC	4
 #define AC97_RATES_SPDIF	5
 
+/* device private data magic number */
+#define AC97_PDEVMAGIC_UIRQ	0x55495251	/* in ASCII: UIRQ, for UCB1400 */
+
 /*
  *
  */
@@ -470,6 +473,15 @@ struct snd_ac97_template {
 	const struct snd_ac97_res_table *res_table;	/* static resolution */
 };
 
+struct ac97_pdevdata {
+	unsigned int magic;	/* see AC97_PDEVMAGIC */
+	union {
+		int ivalue;
+		long lvalue;
+		void *pvalue;
+	} u;
+};
+
 struct snd_ac97 {
 	/* -- lowlevel (hardware) driver specific -- */
 	struct snd_ac97_build_ops * build_ops;
@@ -478,7 +490,7 @@ struct snd_ac97 {
 	/* This field is used by device drivers which serve devices which are
 	 * attached to the AC97 bus.
 	 */
-	void *device_private_data;
+	struct ac97_pdevdata *device_private_data;
 	/* --- */
 	struct snd_ac97_bus *bus;
 	struct pci_dev *pci;	/* assigned PCI device - used for quirks */
@@ -545,6 +557,11 @@ static inline int ac97_can_spdif(struct 
 static inline int ac97_can_spdif(struct snd_ac97 * ac97)
 {
 	return (ac97->ext_id & AC97_EI_SPDIF) != 0;
+}
+static inline int ac97_check_pdevdata_magic(struct snd_ac97 * ac97, unsigned int magic)
+{
+	return ac97->device_private_data &&
+	       ac97->device_private_data->magic == magic;
 }
 
 /* functions */
diff -r e2ff47e8771b kernel/drivers/input/touchscreen/ucb1400_ts.c
--- a/kernel/drivers/input/touchscreen/ucb1400_ts.c	Fri Apr 25 08:29:05 2008 +0200
+++ b/kernel/drivers/input/touchscreen/ucb1400_ts.c	Fri Apr 25 11:43:11 2008 +0200
@@ -492,14 +492,14 @@ static int ucb1400_ts_probe(struct devic
 		goto err_free_devs;
 	}
 
-	if (!ucb->ac97->device_private_data) {
+	if (!ac97_check_pdevdata_magic(usb->ac97, AC97_PDEVMAGIC_UIRQ)) {
 		error = ucb1400_detect_irq(ucb);
 		if (error) {
 			printk(KERN_ERR "UCB1400: IRQ probe failed\n");
 			goto err_free_devs;
 		}
 	} else {
-		ucb->irq = (int) ucb->ac97->device_private_data;
+		ucb->irq = ucb->ac97->device_private_data->u.ivalue;
 	}
 
 	error = request_irq(ucb->irq, ucb1400_hard_irq, IRQF_TRIGGER_RISING,

-----
Jaroslav Kysela <perex at perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.