[alsa-devel] [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field
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
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.
* 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
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.
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.
* 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
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.
* 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
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.
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
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 */ };
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
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.
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
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.
* 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
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
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.
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.
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.
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.
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.
* 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
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
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.
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:
Indeed.
Takashi
* 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
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
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 *.
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.
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
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.
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 datamust 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.
participants (5)
-
Dmitry Torokhov -
Jaroslav Kysela -
Mark Brown -
Sebastian Siewior -
Takashi Iwai