From: Sebastian Siewior <hidden> Date: 2008-04-24 14:04:59
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
From: Jaroslav Kysela <perex@perex.cz> Date: 2008-04-24 14:32:37
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).
Could you change and resend your patch?
Thanks,
Jaroslav
-----
Jaroslav Kysela [off-list ref]
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.
From: Sebastian Siewior <hidden> Date: 2008-04-24 14:35:02
* Jaroslav Kysela | 2008-04-24 16:32:37 [+0200]:
good morning :)
On Thu, 24 Apr 2008, Sebastian Siewior wrote:
quoted
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).
From: Mark Brown <hidden> Date: 2008-04-24 14:57:52
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.
From: Sebastian Siewior <hidden> Date: 2008-04-24 15:35:25
* Mark Brown | 2008-04-24 15:57:52 [+0100]:
On Thu, Apr 24, 2008 at 04:04:59PM +0200, Sebastian Siewior wrote:
quoted
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:
quoted
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.
quoted
- 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.
quoted
- 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
From: Sebastian Siewior <hidden> Date: 2008-04-24 15:44:10
* Mark Brown | 2008-04-24 16:02:12 [+0100]:
On Thu, Apr 24, 2008 at 03:57:52PM +0100, Mark Brown wrote:
quoted
quoted
- hacking up the ucb1400 [2] (doesn't solve [3] and needs addition code
to solve [4]).
quoted
[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
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:
quoted
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:
quoted
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
From: Mark Brown <hidden> Date: 2008-04-24 18:57:06
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).
quoted
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 */
};
From: Mark Brown <hidden> Date: 2008-04-24 20:04:46
On Thu, Apr 24, 2008 at 05:35:20PM +0200, Sebastian Siewior wrote:
* Mark Brown | 2008-04-24 15:57:52 [+0100]:
quoted
On Thu, Apr 24, 2008 at 04:04:59PM +0200, Sebastian Siewior wrote:
quoted
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).
quoted
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.
From: Mark Brown <hidden> Date: 2008-04-24 21:33:36
On Thu, Apr 24, 2008 at 05:44:10PM +0200, Sebastian Siewior wrote:
* Mark Brown | 2008-04-24 16:02:12 [+0100]:
quoted
quoted
[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
quoted
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.
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:
quoted
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.
quoted
quoted
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.
quoted
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
From: Jaroslav Kysela <perex@perex.cz> Date: 2008-04-25 07:10:33
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 [off-list ref]
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:
quoted
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
From: Jaroslav Kysela <perex@perex.cz> Date: 2008-04-25 07:35:49
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
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 [off-list ref]
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.
From: Sebastian Siewior <hidden> Date: 2008-04-25 07:46:16
* Jaroslav Kysela | 2008-04-25 09:35:47 [+0200]:
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
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.
At Fri, 25 Apr 2008 09:35:47 +0200 (CEST),
Jaroslav Kysela wrote:
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
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.
quoted
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
From: Jaroslav Kysela <perex@perex.cz> Date: 2008-04-25 08:23:53
On Fri, 25 Apr 2008, Takashi Iwai wrote:
At Fri, 25 Apr 2008 09:35:47 +0200 (CEST),
Jaroslav Kysela wrote:
quoted
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
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.
quoted
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
At Fri, 25 Apr 2008 10:23:51 +0200 (CEST),
Jaroslav Kysela wrote:
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
At Fri, 25 Apr 2008 09:35:47 +0200 (CEST),
Jaroslav Kysela wrote:
quoted
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
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.
quoted
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
From: Jaroslav Kysela <perex@perex.cz> Date: 2008-04-25 09:45:04
On Fri, 25 Apr 2008, Takashi Iwai wrote:
At Fri, 25 Apr 2008 10:23:51 +0200 (CEST),
Jaroslav Kysela wrote:
quoted
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
At Fri, 25 Apr 2008 09:35:47 +0200 (CEST),
Jaroslav Kysela wrote:
quoted
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
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.
quoted
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
@@ -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 {conststructsnd_ac97_res_table*res_table;/* static resolution */};+structac97_pdevdata{+unsignedintmagic;/* see AC97_PDEVMAGIC */+union{+intivalue;+longlvalue;+void*pvalue;+}u;+};+structsnd_ac97{/* -- lowlevel (hardware) driver specific -- */structsnd_ac97_build_ops*build_ops;
@@ -478,7 +490,7 @@ struct snd_ac97 {/* This field is used by device drivers which serve devices which are*attachedtotheAC97bus.*/-void*device_private_data;+structac97_pdevdata*device_private_data;/* --- */structsnd_ac97_bus*bus;structpci_dev*pci;/* assigned PCI device - used for quirks */
From: Mark Brown <hidden> Date: 2008-04-25 09:51:09
On Fri, Apr 25, 2008 at 09:02:27AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
quoted
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 11:45:01 +0200 (CEST),
Jaroslav Kysela wrote:
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
At Fri, 25 Apr 2008 10:23:51 +0200 (CEST),
Jaroslav Kysela wrote:
quoted
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
At Fri, 25 Apr 2008 09:35:47 +0200 (CEST),
Jaroslav Kysela wrote:
quoted
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
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.
quoted
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
@@ -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 {conststructsnd_ac97_res_table*res_table;/* static resolution */};+structac97_pdevdata{+unsignedintmagic;/* see AC97_PDEVMAGIC */+union{+intivalue;+longlvalue;+void*pvalue;+}u;+};+structsnd_ac97{/* -- lowlevel (hardware) driver specific -- */structsnd_ac97_build_ops*build_ops;
@@ -478,7 +490,7 @@ struct snd_ac97 {/* This field is used by device drivers which serve devices which are*attachedtotheAC97bus.*/-void*device_private_data;+structac97_pdevdata*device_private_data;/* --- */structsnd_ac97_bus*bus;structpci_dev*pci;/* assigned PCI device - used for quirks */
At Fri, 25 Apr 2008 10:51:07 +0100,
Mark Brown wrote:
quoted
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.
quoted
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
From: Jaroslav Kysela <perex@perex.cz> Date: 2008-04-25 10:18:14
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
quoted
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.
quoted
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 [off-list ref]
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.
From: Jaroslav Kysela <perex@perex.cz> Date: 2008-04-25 10:20:31
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 [off-list ref]
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.
From: Mark Brown <hidden> Date: 2008-04-25 10:28:33
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.
From: Sebastian Siewior <hidden> Date: 2008-04-25 10:54:32
* Jaroslav Kysela | 2008-04-25 10:23:51 [+0200]:
quoted hunk
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
At Fri, 25 Apr 2008 09:35:47 +0200 (CEST),
Jaroslav Kysela wrote:
quoted
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
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.
quoted
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
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).
At Fri, 25 Apr 2008 12:54:29 +0200,
Sebastian Siewior wrote:
* Jaroslav Kysela | 2008-04-25 10:23:51 [+0200]:
quoted
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
At Fri, 25 Apr 2008 09:35:47 +0200 (CEST),
Jaroslav Kysela wrote:
quoted
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
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.
quoted
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
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
From: Jaroslav Kysela <perex@perex.cz> Date: 2008-04-25 11:22:13
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 [off-list ref]
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.
From: Sebastian Siewior <hidden> Date: 2008-04-25 12:49:18
* Takashi Iwai | 2008-04-25 13:10:31 [+0200]:
quoted
Why is it a problem to keep an anonymous struct?
You describe in your text below :)
Right :D
quoted
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 :)
quoted
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.
quoted
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.
At Fri, 25 Apr 2008 14:49:08 +0200,
Sebastian Siewior wrote:
quoted
quoted
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
At Fri, 25 Apr 2008 13:22:11 +0200 (CEST),
Jaroslav Kysela wrote:
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
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.
On Fri, Apr 25, 2008 at 09:35:47AM +0200, Jaroslav Kysela wrote:
On Fri, 25 Apr 2008, Takashi Iwai wrote:
quoted
quoted
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.
quoted
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