On 08/23/2011 08:49 AM, Péter Ujfalusi wrote:
On Monday 22 August 2011 15:39:15 Lars-Peter Clausen wrote:
omap_mcpdm_widgets is a global variable.
Yeah, as most of the snd_soc_dapm_widget.
The point is, you use it to pass runtime specific data around, while the others are constant compile time data, which are used as a template.
You assign to it in asoc_mcpdm_probe
Since at compile time I don't have the pointer for the mcpdm (it is allocated earlier in the same function), I need to assign it somewhere.
and read from it in omap_mcpdm_add_dapm_widget.
I don't see any reference to the omap_mcpdm_widgets in there.
+ return snd_soc_dapm_new_controls(dapm, omap_mcpdm_widgets, + ARRAY_SIZE(omap_mcpdm_widgets));
The fact that you hide your *mcpdm in a void pointer doesn't make it less hackish.
Well, from that point of view most of the kernel is hackish. We tend to have void pointers for various things, like platform_data, device_data, driver_data, private_data, etc.
I'm not arguing against such constructs. I'm arguing against your usage of them.
Let me give you an example which is analogous to yours:
static struct platform_device foo;
static void bar_probe(struct platform_device *pdev) { foo.dev.platform_data = ...; }
void bar_some_global_func(void) { platform_device_add(&foo); }
You'll rarely see this in driver code.
If that doesn't convince you, ask yourself what would happen if you had two instances of the mcpdm driver.
You see, the point here is that this private_data for the widget can be used for others as well, if needed. It would make no sense to put "struct omap_mcpdm *mcpdm", just because I have this requirement first, does it?
For sure, I could have chosen to create one global pointer for this event handler:
static struct omap_mcpdm *mcpdm_global;
Use the mcpdm_global within omap_mcpdm_interface_event function, and assign it at asoc_mcpdm_probe time.
Would that look better? IMHO it is not.
Your current solution might look better on the surface, but it is deep ugly on the inside. You've hidden your mcpdm_global in a construct that is normally present in a ASoC driver. You've just slightly changed it in subtitle way, apparently so subtitle you don't even see it yourself.