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.
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.
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. 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.
As Mark suggested, we should have accessor for this, if we are going to have such a thing.
The reason for this (as I described in the commit message): OMAP McPDM has the requirement (along with the twl6040), that we need to stop the cpu dai _after_ the DAC/ADC has been stopped on the codec side. Now, I do not want to hard wire the twl6040, nor the omap-mcpdm with each other. The omap-mcpdm implements the event callback, and the machine driver is responsible to create the DAPM route to make sure we follow the correct sequence. I could as well implemented these things in the machine driver (+global pointer for mcpdm), but that can lead duplicated code in the future (new machine driver, etc).