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