[alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA codec driver

[Takashi Iwai]

On Tue, 19 Dec 2017 18:12:03 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai at suse.de]
> >Sent: Tuesday, December 19, 2017 9:54 PM
> >To: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>
> >Cc: alsa-devel at alsa-project.org; Koul, Vinod <vinod.koul at intel.com>; pierre-
> >louis.bossart at linux.intel.com; liam.r.girdwood at linux.intel.com; Patches Audio
> ><patches.audio at intel.com>; broonie at kernel.org
> >Subject: Re: [alsa-devel] [RFC v3 06/11] ASoC: hdac_hda: add ASoC based HDA
> >codec driver
> >
> >> >
> >> >Well, a big unclear question to me is why do we need to bind the stuff
> >> >so differently.  Can't we simply provide the same binding to the
> >> >legacy codec from asoc driver?  In the legacy-support mode, asoc
> >> >driver creates the legacy-compatible codec objects with the
> >> >legacy-compatible hda_bus.
> >> >
> >>
> >> Both the drivers i.e. ASoC and Legacy are registering the driver in
> >> exact same way by filling the hdac_driver fields. There is no difference
> >> in terms of HDA bus interactions. First series unifies even the data
> >> structures hdac_device, hdac_driver and hdac_bus.
> >
> >Yes, that's a good part.
> >
> >> Once the device is enumerated and the hdac_driver->probe
> >> is called the difference starts, primarily because ASoC vs ALSA
> >> codec driver differences.
> >
> >Why so different?  Is it hard to integrate them somehow?
> >
> >What exactly do we need to do in addition to the existing legacy
> >HD-audio codec probe/remove/whatever?
> 
> probe/remove of the ASoC driver takes care of registering/unregistering 
> the snd_soc_codec_driver with ASoC framework. Which would register
> dai, widgets and controls associated with the codec.
> 
> But in this driver probe the card is not created and so nothing else is done.
> It waits until the ASoC framework calls snd_soc_codec probe.
> 
> So the way I see is ASoC driver probe is just a bus level probe and its
> one level before the legacy codec driver probe.
> 
> Legacy codec driver probe is kind of similar to snd_soc_codec probe.
> Again if you think from unification, we are calling the exact same APIs
> which are called in the legacy driver probe. Some legacy APIs are not 
> used e.g. build_pcms.
> 
> snd_soc_codec_driver callbacks are handled by ASoC framework
> and so we can not call it from anywhere else. But within this we call 
> most of the APIs of the legacy codec driver.
> 
> You can look at the ~/sound/soc/codecs/hdac_hda.c file, its just one file
> for the ASoC driver.
> 
> >
> >> Here also if you notice, after taking care of
> >> the ASoC related components, ASoC codec driver calls exact same APIs of the
> >> legacy HDA codec driver which are called by the legacy controller driver.
> >>
> >> The hda_bus and hda_codec data structures are also used by the ASoC
> >> driver as it is to call legacy codec driver APIs.
> >>
> >> So I am not sure if we are doing binding in two different ways, or I
> >> misunderstood you completely ?
> >
> >Well, I still am not sure why do we need two ways, completely
> >switching the whole binding.  If it's a matter of some additional
> >calls on top of the legacy probe, we can add a new optional bus ops,
> >and call it in the probe function, too.  At the time of probe
> >callback, the bus is already present.
> 
> From the bus point of view, there are no differences in the driver.
> The Series 1 has already done that.
> 
> The difference is only due to ASoC vs ALSA framework. 
> 
> In the last patch, instead of checking the bus_type everywhere in the code,
> it uses it to call the individual driver function. That way the hdac_driver 
> callback implementation remains separate and much cleaner for ASoC
> and legacy drivers. So the legacy codec driver APIs are still used but
> without too many if else conditions.

Well, honestly speaking, that complete morphing scares me.  If it's a
simple additional call, it's easier to track the code flow.  But your
patch essentially fakes the legacy codec and bus structs and twists
the control by that switch...  which looks too fragile to me.


thanks,

Takashi