* Jaroslav Kysela | 2008-04-25 10:23:51 [+0200]:
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:
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 10:22:00 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_IRQ 0x20495251 /* in ASCII: <space>IRQ */
/*
*/ @@ -545,6 +547,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 &&
*((unsigned int *)ac97->device_private_data) == 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 10:22:00 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_IRQ)) { 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 = ((int *) ucb->ac97->device_private_data)[1];}
error = request_irq(ucb->irq, ucb1400_hard_irq, IRQF_TRIGGER_RISING,
This is getting beyond what I planned. Now I have to allocate a struct and I don't like the void * to int casts. I just did it once to save an allocation of 4 bytes for my int *. Why is it a problem to keep an anonymous struct? If some one uses a wrong struct than it crashes immediatelly or bails out because 0x20495251 is way too large be an IRQ. Putting that magic and casting for every single possible data blows code for no good reason. Don't recover from errors which should not have happen, solve them at root level not where the leaves are. What I intended in first place is to allocate a private field in the bus struct so can pass informations to the lower driver. If you need multiple arguments, create your own struct put it in the void * slot, your driver knows what to do.
Usually you have a bus, you probe that bus and you get all your informations you need from this bus. In this case, once your ac97 is ready you probe again and according to the vendor-id the ucb1400 driver is responsible for this. From what I can see, we probe every ac97-driver gets probed because there is no standard field for vendor / device id within ac97 register space. So the ucb1400 driver gets probed knowing only that it is attached to the ac97. It finds out that it is the correct device reading from a private register. At that point you can safely access the void * and assuming it is the irq number. In case ucb1400 assumes wrongly that it is responsible for that device (and that void * is something complete different) than bad things happen anyway.
Ah SPI. Same issue. What do I see there? void *controller_data; Who is using this? Right the driver that is talking to a device behind a the SPI bus and needs some nen-standard non-spi things to get things to work, like an interrupt number. And this informations there are driver specific (like ucb1400).
Jaroslav
Sebastian