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@xxxxxxxx> Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html