Re: [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver

[Lars-Peter Clausen <lars@metafoo.de>]

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.