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

["Péter Ujfalusi" <peter.ujfalusi@ti.com>]

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).

-- 
Péter