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

[Takashi Iwai]

At Fri, 25 Apr 2008 11:45:01 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> 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.

No, suppose the data like

	int magic;
	void *ptr;

and the code to retrive the magic is

	magic = *(int*)device_data;

and thus the next data pointer is (int*)device_data + 1.
This doesn't work.

> 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:

Hm, but this allows you pass only one value and has an ugly union.

I'm not sure which paratemers are still needed to be passed and how
often and how many of them are used at once.  We need more exact use
cases for implementing this properly.

If majority of cases are a single int or long, we can use such a
style.  Then even a single void pointer would be enough instead of
union.  But, if the driver requires multiple values in most cases,
better to use container_of().


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 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.
>