Re: [PATCH 02/14] ASoC: codecs: Add HD-Audio codec driver

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-05-06 3:12 PM, Mark Brown wrote:
> On Wed, Apr 27, 2022 at 10:47:12AM -0500, Pierre-Louis Bossart wrote:
>> On 4/27/22 03:18, Cezary Rojewski wrote:
> 
>>> Add generic ASoC equivalent of ALSA HD-Audio codec. This codec is
>>> designed to follow HDA_DEV_LEGACY convention. Driver wrapps existing
>>> hda_codec.c handlers to prevent code duplication within the newly added
> 
>> I am surprised the explanations don't even mention the existence of hdac_hda.c
> 
>> I thought the series was about adding machine drivers, but this
>> also adds code on the sound/soc/codecs/ side which I didn't see
>> coming.
> 
>> I am not qualified to review this part of the code, I just
>> wonder about duplication of functionality.
> 
>> At the very least an explanation on why you decided to NOT use
>> hdac_hda.c would be useful to reviewers and maintainers.
> 
> Right, why the duplication here?  Can't we fix or extend the
> existing code to do whatever it's not currently doing which
> compels reimplementation?

Sorry for the late response, did not realize there is an unanswered 
comment here.

So, the rough list goes as:
- hdac_hda.c hardcodes codec capabilities rather than aligning with what 
sound/pci/hda/ code does
- merges HDMI (i.e. Intel i915 audio component) and HDA DAIs together 
whereas these are two separate devices
- because of above, implements custom search/matching mechanism for PCM/DAI
- cont. because of above, its header hosts private data struct, 
unnecessary complication
- follows HDA_DEV_ASOC convention rather than HDA_DEV_LEGACY causing 
misalignments between sound/pci/hda and sound/soc/ behaviour
- has basic PM runtime support and does not survive scenarios where 
resume/suspend + denylist + rmmod/modprobe are mixed together or invoked 
in unordered fashion between this module and several others in the audio 
stack

My suggestion is different: have all HD-Audio ASoC users switch to this 
implementation when possible and remove the existing code along with 
skylake-driver.


Regards,
Czarek