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.
Defining a simple struct would be safer and easier.
thanks,
Takashi
>
> 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,
>
> Jaroslav
>
> -----
> 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
