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

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

[Sebastian Siewior]

Hello,

I've sent a RFC to the alsa mailing list [1] about adding an extra field in
order to pass the IRQ from the AC97 driver to the ucb1400 driver. The
result was:
- William Pitcock recommended to hack up the ucb1400 driver and save the
  irq there. The result is [2]. What I don't like very much is the
  global variable and the fact that has to be done for every platform.
- Mark Brown has the same problem with his wm97xx driver [3].

and while browsing a dead list archive of linux-input I noticed that a
guy named Peter Ma has the same problem on his AVR32 [4].

I haven't heard anything from the ALSA Maintainer but according to git
he didn't put a sing-of-by line since Thu Jan 31 17:40:18 2008 +0100 so
maybe he is sleeping. That's why I've add Takashi Iwai since he seems to
be in charge some how.

Now I'm curious what solution the people here prefer:
- adding a private field [1] (my favorite)
- hacking up the ucb1400 [2] (doesn't solve [3] and needs addition code
  to solve [4]).
- something totally different what did not come to my attention yet.

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2008-March/006555.html
[2] http://mailman.alsa-project.org/pipermail/alsa-devel/2008-March/006559.html
[3] http://mailman.alsa-project.org/pipermail/alsa-devel/2008-March/006564.html
[4] http://www.mail-archive.com/linux-input@vger.kernel.org/msg00307.html

Sebastian

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Jaroslav Kysela]

On Thu, 24 Apr 2008, Sebastian Siewior wrote:

> I've sent a RFC to the alsa mailing list [1] about adding an extra field 
> in order to pass the IRQ from the AC97 driver to the ucb1400 driver. The 
> result was:

The original patch seems to be acceptable. But I would choose a more 
universal name and 'void *' type like 'void *device_private_data' and 
specify more the purpose of this member (add USB1400 as example device).

> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2008-March/006555.html

Could you change and resend your patch?

				Thanks,
					Jaroslav

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

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Sebastian Siewior]

* Jaroslav Kysela | 2008-04-24 16:32:37 [+0200]:
good morning :)

>On Thu, 24 Apr 2008, Sebastian Siewior wrote:
>
>> I've sent a RFC to the alsa mailing list [1] about adding an extra field 
>> in order to pass the IRQ from the AC97 driver to the ucb1400 driver. The 
>> result was:
>
>The original patch seems to be acceptable. But I would choose a more 
>universal name and 'void *' type like 'void *device_private_data' and 
>specify more the purpose of this member (add USB1400 as example device).
I came to this conclusion after the wmXXX thingi.

>> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2008-March/006555.html
>
>Could you change and resend your patch?
yes sir.
>
>				Thanks,
>					Jaroslav
>

Sebastian

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Mark Brown]

On Thu, Apr 24, 2008 at 04:04:59PM +0200, Sebastian Siewior wrote:

> I've sent a RFC to the alsa mailing list [1] about adding an extra field in
> order to pass the IRQ from the AC97 driver to the ucb1400 driver. The
> result was:

> Now I'm curious what solution the people here prefer:
> - adding a private field [1] (my favorite)

As I indicated in reply to your initial RFC any such private field
ought to be a void * in order to allow other information to be passed
through to drivers.

Note that this will also need changes in all the relevant AC97 drivers
to support getting the private data from platform/machine definition
code to the relevant driver using whatever methods are appropriate for
the platform.

> - hacking up the ucb1400 [2] (doesn't solve [3] and needs addition code
>   to solve [4]).

[3] is the issue with the WM97xx touchscreen drivers.  That's currently
solved by exactly this issue - as far as I can see from the patch you
cite you're using OpenFirmware.  In that case isn't modifying the driver
to query OpenFirmware an idiomatic solution anyway (though it still
leaves other platforms in the lurch)?

[4] appears to be be handling of IRQs greater than 32 - ISTR that
autoprobing of IRQs greater than 32 is now handled as a result of a
patch in the past week.

> - something totally different what did not come to my attention yet.

Something that worked for more than just AC97 would be nice - a method
for getting platform data to drivers for devices on buses that are
normally autoprobed.

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

[Mark Brown]

On Thu, Apr 24, 2008 at 03:57:52PM +0100, Mark Brown wrote:

> > - hacking up the ucb1400 [2] (doesn't solve [3] and needs addition code
> >   to solve [4]).

> [3] is the issue with the WM97xx touchscreen drivers.  That's currently
> solved by exactly this issue - as far as I can see from the patch you

The second issue should be method.

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Sebastian Siewior]

* Mark Brown | 2008-04-24 16:02:12 [+0100]:

>On Thu, Apr 24, 2008 at 03:57:52PM +0100, Mark Brown wrote:
>
>> > - hacking up the ucb1400 [2] (doesn't solve [3] and needs addition code
>> >   to solve [4]).
>
>> [3] is the issue with the WM97xx touchscreen drivers.  That's currently
>> solved by exactly this issue - as far as I can see from the patch you
>
>The second issue should be method.

I don't think that this is the right way in this case:
- this information equals a global variable. You can't have two
  touchscreens without exra hacking.
- if we keep doing this for all drivers (not just ucb1400) they gonna
  look like drivers/usb/host/ehci-hcd.c (look line 998+) once this chip
  spreads across buses.

Sebatian

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

[Mark Brown]

On Thu, Apr 24, 2008 at 05:44:10PM +0200, Sebastian Siewior wrote:
> * Mark Brown | 2008-04-24 16:02:12 [+0100]:

> >> [3] is the issue with the WM97xx touchscreen drivers.  That's currently
> >> solved by exactly this issue - as far as I can see from the patch you

> >The second issue should be method.

> I don't think that this is the right way in this case:

Sorry, I didn't mean to suggest that this was desperately elegant.

> - this information equals a global variable. You can't have two
>   touchscreens without extra hacking.

Indeed.  In this case it doesn't really matter since it's vanishingly
unlikely that any such systems would actually exist.

> - if we keep doing this for all drivers (not just ucb1400) they gonna
>   look like drivers/usb/host/ehci-hcd.c (look line 998+) once this chip
>   spreads across buses.

The WM97xx driver avoids this by registering a child driver for the
platform code to bind to.  Again, I don't mean to suggest that this is
particularly elegant.

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

[Sebastian Siewior]

* Mark Brown | 2008-04-24 15:57:52 [+0100]:

>On Thu, Apr 24, 2008 at 04:04:59PM +0200, Sebastian Siewior wrote:
>
>> I've sent a RFC to the alsa mailing list [1] about adding an extra field in
>> order to pass the IRQ from the AC97 driver to the ucb1400 driver. The
>> result was:
>
>> Now I'm curious what solution the people here prefer:
>> - adding a private field [1] (my favorite)
>
>As I indicated in reply to your initial RFC any such private field
>ought to be a void * in order to allow other information to be passed
>through to drivers.
I got that.

>Note that this will also need changes in all the relevant AC97 drivers
>to support getting the private data from platform/machine definition
>code to the relevant driver using whatever methods are appropriate for
>the platform.
Sure. I haven't found any driver that needs any extra information. For
instance a board that uses ucb1400_ts and gets the interrupt via auto
probing can't be auto converted due to -ENOCLUE. Therefore the ucb1400
driver acts like before if the private field is NULL.

>> - hacking up the ucb1400 [2] (doesn't solve [3] and needs addition code
>>   to solve [4]).
>
>[3] is the issue with the WM97xx touchscreen drivers.  That's currently
>solved by exactly this issue - as far as I can see from the patch you
>cite you're using OpenFirmware.  In that case isn't modifying the driver
>to query OpenFirmware an idiomatic solution anyway (though it still
>leaves other platforms in the lurch)?
Not really. I have to parse the whole device tree and pick one single
value. This isn't done by any other driver as far as I can see and it
equals a global variable.
My device tree currently looks like the following:

|                ac97@f0000400 {
|                        compatible = "fpga-ac97";
|                        reg = <f0000400 100>;
|                        interrupts = <5 1>;
|                        interrupt-parent = <&mpic>;
|                        ucb1400@ac97 {
|                                compatible = "Phillips,ucb1400_ts"
|                                interrupts = <9 0>;
|                                interrupt-parent = <&mpic>;
|                        }
|                };

Now, while I get auto probed for my device (fpga-ac97) I grab and setup
the IRQ for the ucb1400_ts device and pass the IRQ-number in the ac97
struct. If you have a platform driver you can still do something like:

|static struct resource smc91x_resources[] = {
|        {
|                .start = 0x20200300,
|                .end = 0x20200300 + 16,
|                .flags = IORESOURCE_MEM,
|        }, {
|                .start = IRQ_PF0,
|                .end = IRQ_PF0,
|                .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHLEVEL,
|        },
|};
|static struct platform_device smc91x_device = {
|        .name = "smc91x",
|        .id = 0,
|        .num_resources = ARRAY_SIZE(smc91x_resources),
|        .resource = smc91x_resources,
|};

and add another IORESOURCE_IRQ field with holds the interrupt number for
your ac97 device (in case of ucb1400_ts or some other thing for a total
different driver). The ucb1400_ts driver itself does not care anymore
because it is getting this information from the ac97 struct.

>> - something totally different what did not come to my attention yet.
>
>Something that worked for more than just AC97 would be nice - a method
>for getting platform data to drivers for devices on buses that are
>normally autoprobed.
I thing here is a miss understanding. What would be something beside
ac97? Gimme a real world example plz. According to grep ucb1400 is the
driver attached to ac97 bus (well, nothing else matches on ac97_bus_type
except the sound & codec thing in sound/ which don't need any extra
parameters).

Sebastian

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Mark Brown]

On Thu, Apr 24, 2008 at 05:35:20PM +0200, Sebastian Siewior wrote:
> * Mark Brown | 2008-04-24 15:57:52 [+0100]:
> >On Thu, Apr 24, 2008 at 04:04:59PM +0200, Sebastian Siewior wrote:

> >cite you're using OpenFirmware.  In that case isn't modifying the driver
> >to query OpenFirmware an idiomatic solution anyway (though it still
> >leaves other platforms in the lurch)?

> Not really. I have to parse the whole device tree and pick one single
> value. This isn't done by any other driver as far as I can see and it
> equals a global variable.

There are several PCI device drivers which have OpenFirmware support -
they appear to deal with this using the pci_device_to_OF_node() call
rather than by groveling through the entire OF tree.  I've not looked
much further than that.

In any case, this is a bit of a sidetrack since it doesn't remove the need
for something beyond OpenFirmware given that there's quite a few
architectures that don't use it at all (at least so far).

> >Something that worked for more than just AC97 would be nice - a method
> >for getting platform data to drivers for devices on buses that are
> >normally autoprobed.

> I thing here is a miss understanding. What would be something beside
> ac97? Gimme a real world example plz. According to grep ucb1400 is the

Most of the examples I've encountered come when PCI devices (and other
devices for buses which do autoprobing) are used in embedded systems and
in order to save component costs the SEPROMs normally used to hold
essential device configuration data (such as MAC addresses for ethernet
devices) are not fitted and instead a single configuration source is
used for the system.

When I've seen it this has generally been handled with ifdefed calls
into machine specific code to read from wherever the data should be read
from - many of the OpenFirmware-using PCI devices appear to be doing
basically this.

> driver attached to ac97 bus (well, nothing else matches on ac97_bus_type
> except the sound & codec thing in sound/ which don't need any extra
> parameters).

The WM97xx drivers should appear in the kernel during this merge window,
FWIW.

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

[Takashi Iwai]

At Thu, 24 Apr 2008 15:57:52 +0100,
Mark Brown wrote:
> 
> On Thu, Apr 24, 2008 at 04:04:59PM +0200, Sebastian Siewior wrote:
> 
> > I've sent a RFC to the alsa mailing list [1] about adding an extra field in
> > order to pass the IRQ from the AC97 driver to the ucb1400 driver. The
> > result was:
> 
> > Now I'm curious what solution the people here prefer:
> > - adding a private field [1] (my favorite)
> 
> As I indicated in reply to your initial RFC any such private field
> ought to be a void * in order to allow other information to be passed
> through to drivers.

The problem with void * is that you don't know what it really is.
Yes, it's exactly the purpose - to be generic.  But, this means that
the true shape of the tossed data from the ac97 controller driver to
the platform driver is anonymous, too.

So, from that perspective, I find 'int irq' better to assure a strong
binding.  Of course, if there are more other use cases, this argument
doesn't apply well.

> Note that this will also need changes in all the relevant AC97 drivers
> to support getting the private data from platform/machine definition
> code to the relevant driver using whatever methods are appropriate for
> the platform.

What kind of data are needed be passed?


thanks,

Takashi

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

[Mark Brown]

On Thu, Apr 24, 2008 at 06:09:52PM +0200, Takashi Iwai wrote:

> The problem with void * is that you don't know what it really is.
> Yes, it's exactly the purpose - to be generic.  But, this means that
> the true shape of the tossed data from the ac97 controller driver to
> the platform driver is anonymous, too.

Indeed, but unfortunately if you try to cater for everything that might
need to pass data through you rapidly get a balloning stucture - it's
the same use case as the device_data in struct device (in fact, now that
I think about it it may be best to just do the same thing as the
platform bus does and define accessor macros for getting at this from a
struct snd_ac97).

> > Note that this will also need changes in all the relevant AC97 drivers
> > to support getting the private data from platform/machine definition
> > code to the relevant driver using whatever methods are appropriate for
> > the platform.

> What kind of data are needed be passed?

In the case of the WM97xx driver it currently takes a structure
containing seven function pointers, an interrupt number and a boolean
for managing integration of the touchscreen interface an auxiliary ADCs
with the rest of the system.  It's really not terribly generic and
there's probably more things that could be added if there were use
cases:

/* Machine specific and accelerated touch operations */
struct wm97xx_mach_ops {

        /* accelerated touch readback - coords are transmited on AC97 link */
        int acc_enabled;
        void (*acc_pen_up) (struct wm97xx *);
        int (*acc_pen_down) (struct wm97xx *);
        int (*acc_startup) (struct wm97xx *);
        void (*acc_shutdown) (struct wm97xx *);

        /* interrupt mask control - required for accelerated operation */
        void (*irq_enable) (struct wm97xx *, int enable);

        /* GPIO pin used for accelerated operation */
        int irq_gpio;

        /* pre and post sample - can be used to minimise any analog noise */
        void (*pre_sample) (int);  /* function to run before sampling */
        void (*post_sample) (int);  /* function to run after sampling */
};

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Takashi Iwai]

At Thu, 24 Apr 2008 19:56:58 +0100,
Mark Brown wrote:
> 
> On Thu, Apr 24, 2008 at 06:09:52PM +0200, Takashi Iwai wrote:
> 
> > The problem with void * is that you don't know what it really is.
> > Yes, it's exactly the purpose - to be generic.  But, this means that
> > the true shape of the tossed data from the ac97 controller driver to
> > the platform driver is anonymous, too.
> 
> Indeed, but unfortunately if you try to cater for everything that might
> need to pass data through you rapidly get a balloning stucture - it's
> the same use case as the device_data in struct device (in fact, now that
> I think about it it may be best to just do the same thing as the
> platform bus does and define accessor macros for getting at this from a
> struct snd_ac97).

Not really as same as device_data.  Normally, device data is handled
by superseding the base device class (just includes struct device) and
retrieve the class via container_of() or such.
In this case, however, you don't know exactly whether the given struct
ac97 is really created by the specific controller, and thus you cannot
assume that the ac97 can be cast to its specific class.


> > > Note that this will also need changes in all the relevant AC97 drivers
> > > to support getting the private data from platform/machine definition
> > > code to the relevant driver using whatever methods are appropriate for
> > > the platform.
> 
> > What kind of data are needed be passed?
> 
> In the case of the WM97xx driver it currently takes a structure
> containing seven function pointers, an interrupt number and a boolean
> for managing integration of the touchscreen interface an auxiliary ADCs
> with the rest of the system.  It's really not terribly generic and
> there's probably more things that could be added if there were use
> cases:
> 
> /* Machine specific and accelerated touch operations */
> struct wm97xx_mach_ops {
> 
>         /* accelerated touch readback - coords are transmited on AC97 link */
>         int acc_enabled;
>         void (*acc_pen_up) (struct wm97xx *);
>         int (*acc_pen_down) (struct wm97xx *);
>         int (*acc_startup) (struct wm97xx *);
>         void (*acc_shutdown) (struct wm97xx *);
> 
>         /* interrupt mask control - required for accelerated operation */
>         void (*irq_enable) (struct wm97xx *, int enable);
> 
>         /* GPIO pin used for accelerated operation */
>         int irq_gpio;
> 
>         /* pre and post sample - can be used to minimise any analog noise */
>         void (*pre_sample) (int);  /* function to run before sampling */
>         void (*post_sample) (int);  /* function to run after sampling */
> };

Hm, indeed it's more than a single value...

For multiple anonymous data, we can use a data with a key like below:

	struct device_hint {
		char *key;
		void *data;
		struct list_head list;
	};

and the controller driver assigns the data like

	controller_init()
	{
		...
		controller.hint.key = "ucb1000-irq";
		controller.hint.data = whatever;
		list_add(&controller.hint. &ac97->device_hint_list);
		...
	}

and the device driver retrieves the data like

	device_init()
	{
		struct device_hint *hint;
		hint = device_hint_get(&ac97->device_hint_list, "ucb1000-irq");
		if (hint) {
			whatever = hint->data;
			...
		}
	}

where device_hint_get() is defined like

	struct device_hint *device_hint_get(struct device_hint *head,
			const char *key)
	{
		struct device_hint *hint;
		list_for_each_entry(hint, head, list)
			if (!strcmp(hint, key))
				return hint;
		return NULL;
	}

Of course, we can cast via container_of() to a container type instead
of using a void pointer there.

This is obviously an overweight for a single use-case, but if we have
more and more complex ones, maybe worth to consider.


thanks,

Takashi

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

[Jaroslav Kysela]

On Fri, 25 Apr 2008, Takashi Iwai wrote:

> Hm, indeed it's more than a single value...
> 
> For multiple anonymous data, we can use a data with a key like below:
> 
> 	struct device_hint {
> 		char *key;
> 		void *data;
> 		struct list_head list;
> 	};
> 
> and the controller driver assigns the data like
> 
> 	controller_init()
> 	{
> 		...
> 		controller.hint.key = "ucb1000-irq";
> 		controller.hint.data = whatever;
> 		list_add(&controller.hint. &ac97->device_hint_list);
> 		...
> 	}
> 
> and the device driver retrieves the data like
> 
> 	device_init()
> 	{
> 		struct device_hint *hint;
> 		hint = device_hint_get(&ac97->device_hint_list, "ucb1000-irq");
> 		if (hint) {
> 			whatever = hint->data;
> 			...
> 		}
> 	}
> 
> where device_hint_get() is defined like
> 
> 	struct device_hint *device_hint_get(struct device_hint *head,
> 			const char *key)
> 	{
> 		struct device_hint *hint;
> 		list_for_each_entry(hint, head, list)
> 			if (!strcmp(hint, key))
> 				return hint;
> 		return NULL;
> 	}
> 
> Of course, we can cast via container_of() to a container type instead
> of using a void pointer there.
> 
> This is obviously an overweight for a single use-case, but if we have
> more and more complex ones, maybe worth to consider.

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.

						Jaroslav

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

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

[Takashi Iwai]

At Fri, 25 Apr 2008 09:10:31 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Fri, 25 Apr 2008, Takashi Iwai wrote:
> 
> > Hm, indeed it's more than a single value...
> > 
> > For multiple anonymous data, we can use a data with a key like below:
> > 
> > 	struct device_hint {
> > 		char *key;
> > 		void *data;
> > 		struct list_head list;
> > 	};
> > 
> > and the controller driver assigns the data like
> > 
> > 	controller_init()
> > 	{
> > 		...
> > 		controller.hint.key = "ucb1000-irq";
> > 		controller.hint.data = whatever;
> > 		list_add(&controller.hint. &ac97->device_hint_list);
> > 		...
> > 	}
> > 
> > and the device driver retrieves the data like
> > 
> > 	device_init()
> > 	{
> > 		struct device_hint *hint;
> > 		hint = device_hint_get(&ac97->device_hint_list, "ucb1000-irq");
> > 		if (hint) {
> > 			whatever = hint->data;
> > 			...
> > 		}
> > 	}
> > 
> > where device_hint_get() is defined like
> > 
> > 	struct device_hint *device_hint_get(struct device_hint *head,
> > 			const char *key)
> > 	{
> > 		struct device_hint *hint;
> > 		list_for_each_entry(hint, head, list)
> > 			if (!strcmp(hint, key))
> > 				return hint;
> > 		return NULL;
> > 	}
> > 
> > Of course, we can cast via container_of() to a container type instead
> > of using a void pointer there.
> > 
> > This is obviously an overweight for a single use-case, but if we have
> > more and more complex ones, maybe worth to consider.
> 
> 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
irq" as in the original patch instead of void data and cast.


thanks,

Takashi

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

[Jaroslav Kysela]

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

					Jaroslav

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

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

[Sebastian Siewior]

* Jaroslav Kysela | 2008-04-25 09:35:47 [+0200]:

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

My driver is OOT right now. The HW isn't final yet and there are no
users (except me and the HW folks). Parts of the driver will certainly
change.

>
>					Jaroslav

Sebastian

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

[Takashi Iwai]

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.


thanks,

Takashi

Re: [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 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,

					Jaroslav

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

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Takashi Iwai]

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@perex.cz>
> Linux Kernel Sound Maintainer
> ALSA Project, Red Hat, Inc.
> 

Re: [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@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: [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@perex.cz>
> Linux Kernel Sound Maintainer
> ALSA Project, Red Hat, Inc.
> 

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

[Jaroslav Kysela]

On Fri, 25 Apr 2008, Takashi Iwai wrote:

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

+ 1 does not work, but driver can define own private structure and it will 
work. The point is that 32-bit magic must be always at offset 0.

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

As I mentioned, this structure can be redefined. I can imagine, that 
drivers won't use predefined structure and create own like:

struct mydata {
	unsigned int magic;	/* required */
	unsigned long value1;
	char *name;
	int somevalue;
};

The only required thing is to have 32-bit magic value at offset 0.

						Jaroslav

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

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

[Sebastian Siewior]

* 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

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Takashi Iwai]

At Fri, 25 Apr 2008 12:54:29 +0200,
Sebastian Siewior wrote:
> 
> * 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?

You describe in your text below :)

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

The root level of the problem is that you pass the anonymous data.
It _IS_ unsafe and wrong unless handled properly.

> What I intended in first place is to allocate a private field in the bus
> struct so can pass informations to the lower driver.

As mentioned in my earlier mail, I'm fine with your first patch.  The
problem occurs when we generalize it.

> If you need
> multiple arguments, create your own struct put it in the void * slot,
> your driver knows what to do.

Your driver does _not_ know what type it is because the data isn't
created by your driver but the controller driver.  And, it's free to
attach your driver to any controller.


thanks,

Takashi

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

[Jaroslav Kysela]

On Fri, 25 Apr 2008, Takashi Iwai wrote:

> Your driver does _not_ know what type it is because the data isn't
> created by your driver but the controller driver.  And, it's free to
> attach your driver to any controller.

But controller knows which codec (using ID bytes) is attached, thus it can 
fill appropriate information only for this codec. Of course, it will work 
when only one additional device is sharing bus with codec. See:

http://www.nxp.com/pip/UCB1400-02.html

						Jaroslav

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

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

[Takashi Iwai]

At Fri, 25 Apr 2008 13:22:11 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Fri, 25 Apr 2008, Takashi Iwai wrote:
> 
> > Your driver does _not_ know what type it is because the data isn't
> > created by your driver but the controller driver.  And, it's free to
> > attach your driver to any controller.
> 
> But controller knows which codec (using ID bytes) is attached, thus it can 
> fill appropriate information only for this codec.

This would be an option, too.

> Of course, it will work 
> when only one additional device is sharing bus with codec. See:
> 
> http://www.nxp.com/pip/UCB1400-02.html

Indeed.


Takashi

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

[Sebastian Siewior]

* Takashi Iwai | 2008-04-25 13:10:31 [+0200]:

>> Why is it a problem to keep an anonymous struct?
>
>You describe in your text below :)
Right :D

>> 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.
>
>The root level of the problem is that you pass the anonymous data.
>It _IS_ unsafe and wrong unless handled properly.
Yes and this should not happen in my perfect world :)

>
>> What I intended in first place is to allocate a private field in the bus
>> struct so can pass informations to the lower driver.
>
>As mentioned in my earlier mail, I'm fine with your first patch.  The
>problem occurs when we generalize it.
Generalize? You mean once you need an array of multiple parameters like
struct ressource where the controler driver and device driver are
independent and don't know each other? In this case I understand why you
prefered the int irq over the void pointer.

>> If you need
>> multiple arguments, create your own struct put it in the void * slot,
>> your driver knows what to do.
>
>Your driver does _not_ know what type it is because the data isn't
>created by your driver but the controller driver.  And, it's free to
>attach your driver to any controller.
Sure. But why should the controller attach data that is not desired for
the driver chip? So if the ucb1400 gets probed with private data that is
desired for another driver it will bail out after checking the device
id and nothing happens. In my case the controller knows that there is
ucb1400 touchscreen attached and I can't imagine why the controller
shouldn't know.

>thanks,
>
>Takashi

Sebastian

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

[Takashi Iwai]

At Fri, 25 Apr 2008 14:49:08 +0200,
Sebastian Siewior wrote:
> 
> >> What I intended in first place is to allocate a private field in the bus
> >> struct so can pass informations to the lower driver.
> >
> >As mentioned in my earlier mail, I'm fine with your first patch.  The
> >problem occurs when we generalize it.
> Generalize? You mean once you need an array of multiple parameters like
> struct ressource where the controler driver and device driver are
> independent and don't know each other?

Right, that's why void pointer was proposed and I'm against.  The
controller driver and the device driver are independent and
implemented so.  If they have the exlusive tight connection, then you
must have no problem.  But, you can bind any device driver to any
controller driver in theory as long as they use the same bus, and
there is no check to prevent it.


Takashi

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

[Dmitry Torokhov]

On Fri, Apr 25, 2008 at 09:35:47AM +0200, 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 *'.

Need to pass irq data will be fairly common so we should probably have
it in its own right and allow additional data be attached via a void
*.

-- 
Dmitry

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

[Mark Brown]

On Fri, Apr 25, 2008 at 09:02:27AM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > I think about it it may be best to just do the same thing as the
> > platform bus does and define accessor macros for getting at this from a
> > struct snd_ac97).

> Not really as same as device_data.  Normally, device data is handled
> by superseding the base device class (just includes struct device) and
> retrieve the class via container_of() or such.

That's normal usage for getting generic class data, not for obtaining
information specific to a particular driver.

> In this case, however, you don't know exactly whether the given struct
> ac97 is really created by the specific controller, and thus you cannot
> assume that the ac97 can be cast to its specific class.

Right, which is why the device_data pointer is used by things like
platform drivers for getting device-specific (as opposed to class
specific) data into the driver from the machine code.

> For multiple anonymous data, we can use a data with a key like below:

This sounds like a reinvention of OpenFirmware, which presents pretty
much the same sort of key/value interface.  It'd be nice to be able to
share the same client code.

> and the controller driver assigns the data like

Doing this would mean that the controller would need to be modified for
each system that wants to pass configuration data to a driver - at that
point much of the win from providing an interface like this is lost.

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Takashi Iwai]

At Fri, 25 Apr 2008 10:51:07 +0100,
Mark Brown wrote:
> 
> > For multiple anonymous data, we can use a data with a key like below:
> 
> This sounds like a reinvention of OpenFirmware, which presents pretty
> much the same sort of key/value interface.  It'd be nice to be able to
> share the same client code.

Yeah, that looks so.

> > and the controller driver assigns the data like
> 
> Doing this would mean that the controller would need to be modified for
> each system that wants to pass configuration data to a driver - at that
> point much of the win from providing an interface like this is lost.

The question (must have been primery one) is what this data passing
should do really.  From my undesrstanding, it's data the controller
driver gives as some hints for the proper device configuration, IFF
the standard configuration doesn't work.

In the case of ucb1400, it's just an irq number.  It sounds logical to
just pass this as an optional info to the driver.
Now, you want to make it general so that you can use it for other
purposes -- but what are the other purpose exactly?  IOW, such data
must be passed through ac97 struct inevitably?


Takashi

Re: [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Jaroslav Kysela]

On Fri, 25 Apr 2008, Takashi Iwai wrote:

> In the case of ucb1400, it's just an irq number.  It sounds logical to
> just pass this as an optional info to the driver.
> Now, you want to make it general so that you can use it for other
> purposes -- but what are the other purpose exactly?  IOW, such data
> must be passed through ac97 struct inevitably?

It's good question. But at least for UCB1400 it looks like that driver for 
the AC97 controller should pass this value. But it's only my guess (I 
don't know these hardware internals).

						Jaroslav

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

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

[Mark Brown]

On Fri, Apr 25, 2008 at 12:15:50PM +0200, Takashi Iwai wrote:

> Now, you want to make it general so that you can use it for other
> purposes -- but what are the other purpose exactly?

In the case of the WM97xx touchscreen driver it is providing information
about non-standard functionality provided by the chips which depends
very strongly on how the chip has been built into the system.  The touch
functionality of the devices can use several multi-function pins on the
device to improve performance, usually in conjunction with
multi-function pins on the SoC.  The driver can also use callback
functions to synchronise touchscreen samples with display updates in
order to minimise electrical interference to the touch panel caused by
activity sending data to the LCD.

This part of the devices is effectively using AC97 as a generic device
access bus rather than using the standard device classes AC97 codecs
normally implement (there is standard AC97 codec functionality in the
devices as well, this is in addition to that).

>                                                       IOW, such data
> must be passed through ac97 struct inevitably?

Well, the wm97xx-ts driver is currently using an alternative method of
getting what it needs but the way it does that leaves something to be
desired.  This comes back to what I was saying about providing a generic
way of getting platform-specific data into devices enumerated via auto
probed buses.