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

[Liam Girdwood <lrg@ti.com>]

On 19/08/11 14:13, Lars-Peter Clausen wrote:
> On 08/19/2011 02:38 PM, Liam Girdwood wrote:
>> On 19/08/11 09:49, Lars-Peter Clausen wrote:
>>> On 08/19/2011 10:38 AM, Lars-Peter Clausen wrote:
>>>> On 08/19/2011 09:41 AM, Peter Ujfalusi wrote:
>>>>> From: Misael Lopez Cruz <misael.lopez@ti.com>
>>>>>
>>>>> Reasons for the replacement:
>>>>> The current driver for McPDM was developed to support the legacy mode only.
>>>>> In preparation for the ABE support the current driver stack need the be
>>>>> replaced.
>>>>> The new driver is much simpler, easier to extend, and it also fixes some of the
>>>>> issues with the old stack.
>>>>>
>>>>> Main changes:
>>>>> - single file for omap-mcpdm (mcpdm.c/h removed)
>>>>> - Define names for registers, bits cleaned up, prefixed
>>>>> - Full-duplex audio operation (arecord | aplay) has been fixed
>>>>> - Less code
>>>>>
>>>>> McPDM need to be turned off _after_ the codec's DAC/ADC has been powered down.
>>>>> The solution for this without extensive change in core:
>>>>> Use DAPM_SUPPLY to turn off the McPDM interface.
>>>>> The machine driver connects the "OMAP McPDM Interface" to the codec's DAC/ADC,
>>>>> so after audio activity we can be sure that the host side McPDM is turned off
>>>>> at the correct time.
>>>>>
>>>>> Signed-off-by: Misael Lopez Cruz <misael.lopez@ti.com>
>>>>> Signed-off-by: Liam Girdwood <lrg@ti.com>
>>>>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>>>> ---
>>>>>  sound/soc/omap/Makefile     |    2 +-
>>>>>  sound/soc/omap/mcpdm.c      |  472 -------------------------
>>>>>  sound/soc/omap/mcpdm.h      |  152 --------
>>>>>  sound/soc/omap/omap-mcpdm.c |  823 ++++++++++++++++++++++++++++---------------
>>>>>  sound/soc/omap/omap-mcpdm.h |   97 +++++
>>>>>  sound/soc/omap/sdp4430.c    |   14 +-
>>>>>  6 files changed, 655 insertions(+), 905 deletions(-)
>>>>>  delete mode 100644 sound/soc/omap/mcpdm.c
>>>>>  delete mode 100644 sound/soc/omap/mcpdm.h
>>>>>  rewrite sound/soc/omap/omap-mcpdm.c (46%)
>>>>>  create mode 100644 sound/soc/omap/omap-mcpdm.h
>>>>>
>>>>> diff --git a/sound/soc/omap/Makefile b/sound/soc/omap/Makefile
>>>>> index 59e2c8d..052fd75 100644
>>>>> --- a/sound/soc/omap/Makefile
>>>>> +++ b/sound/soc/omap/Makefile
>>>>> @@ -1,7 +1,7 @@
>>>>>  # OMAP Platform Support
>>>>>  snd-soc-omap-objs := omap-pcm.o
>>>>>  snd-soc-omap-mcbsp-objs := omap-mcbsp.o
>>>>> -snd-soc-omap-mcpdm-objs := omap-mcpdm.o mcpdm.o
>>>>> +snd-soc-omap-mcpdm-objs := omap-mcpdm.o
>>>>>  snd-soc-omap-hdmi-objs := omap-hdmi.o
>>>>>  
>>>>>  obj-$(CONFIG_SND_OMAP_SOC) += snd-soc-omap.o
>>>>> diff --git a/sound/soc/omap/mcpdm.c b/sound/soc/omap/mcpdm.c
>>>>> deleted file mode 100644
>>>>> index 9746a49..0000000
>>>>> --- a/sound/soc/omap/mcpdm.c
>>>>> +++ /dev/null
>>>>> @@ -1,472 +0,0 @@
>>>>> [...]
>>>>> +static int omap_mcpdm_interface_event(struct snd_soc_dapm_widget *w,
>>>>> +		struct snd_kcontrol *kcontrol, int event)
>>>>> +{
>>>>> +	struct omap_mcpdm *mcpdm = w->private_data;
>>>>
>>>> Can't you use
>>>> struct omap_mcpdm *mcpdm = snd_soc_codec_get_drvdata(codec)?
>>>
>>> sorry:
>>> struct omap_mcpdm *mcpdm =  snd_soc_platform_get_drvdata(w->platform);
>>>
>>>
>>>>
>>>> This would avoid having to add the private_data field to the widget struct. In
>>>> it's current form it will only really work, if there is just one instance of
>>>> the driver using the widget. And if that's the case you can use a global
>>>> variable directly anyway.
>>
>> This wont work as the McPDM DAI is neither a codec or a platform driver. It's best to give the widgets some private data since we may have other uses for it too.
>>
> oops, sorry. I somehow though this was a platform driver, don't know why. But
> still the way you use private_data in this driver, you use it to cleverly hide
> a global variable.
> 
> The whole thing is sort of a hack.  I mean, if it turns out that we want DAPM
> on the SoC DAI side, we should probably give it is own DAPM context, instead of
> using the cards context for this.
> 

A SoC DAI can be represented within DAPM already with the AIF widget, so adding DAI context should be fine.

Liam

> - Lars