[alsa-devel] [PATCH v2 2/2] ASoC: omap-mcpdm: Replace legacy driver
lars at metafoo.de
Tue Aug 23 12:14:58 CEST 2011
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,
>> The fact that you hide your *mcpdm in a void pointer doesn't make it less
> 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 = ...;
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
> 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.
More information about the Alsa-devel