On Thu, 15 Dec 2016 21:37:03 +0100, Pierre-Louis Bossart wrote:
Subject: Re: [PATCH 3/7] ALSA: add shell for Intel HDMI LPE audio driver
Why do we need a "shell" and indirect calls? This is a small driver set, so it's not utterly unacceptable, but it still makes things a bit more complicated than necessary, so I'd like to understand the idea behind it.
The 'shell' comes from an early prototype which didn't do much except create the device. The title of the commit should be changed if it threw you off.
This solution does not use component interface to talk between Audio and i915 driver. The reason for that is the HDMI audio device is created by i915 driver with only one callback pointer passed as pdata. Unlike HDA, the HDMI audio driver does not get loaded if the i915 does not create child device. Since there is only one callback needed we are not using the component interface to make things simpler. This design was co-worked with i915 folks to makes interaction simpler.
The interaction between i915 and audio is simple, yes, it just exposes a few things, mmio ptr, irq, etc. But still I don't understand why multiple layers of indirect accesses are needed *inside* lpe audio driver itself. For example, suspend/resume action is cascaded to yet another ops.
I would understand if this "shell" provides a few thin accessors to the raw i915 registers. But another layering over a single driver implementation makes little sense in regard of abstraction. (If there were multiple class inherits, it's a different story, of course.)
No disagreement here Takashi. I was planning to remove this abstraction in a second incremental pass that would only be on the audio side, for now keeping this layer would allow me to add the DisplayPort changes faster. If we simplify this now then I will have so many differences with the legacy code it'll be a nightmare. I have no emotional attachment to the legacy but it's just pragmatic to avoid changing everything at once.
As you also mentioned it, this second pass could also reuse all the ELD parsing that is still duplicated. These HDMI patches are not a single-shot contribution, you'll have updates coming your way.
OK, if the current structure helps for DP support, it's no problem to keep it. We can refactor the code later easily, too -- it's a single driver code, after all.
thanks,
Takashi