Re: [PATCH 3/7] ALSA: add shell for Intel HDMI LPE audio driver

From: Takashi Iwai <tiwai@suse.de>
To: "Anand, Jerome" <jerome.anand@intel.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"Ughreja, Rakesh A" <rakesh.a.ughreja@intel.com>
Subject: Re: [PATCH 3/7] ALSA: add shell for Intel HDMI LPE audio driver
Date: Thu, 15 Dec 2016 12:14:30 +0100	[thread overview]
Message-ID: <s5htwa5bg15.wl-tiwai@suse.de> (raw)
In-Reply-To: <F6867404AABACB4D8EC371BF802A4B7C14934689@fmsmsx101.amr.corp.intel.com>

On Thu, 15 Dec 2016 11:55:23 +0100,
Anand, Jerome wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, December 14, 2016 6:16 PM
> > To: Anand, Jerome <jerome.anand@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org;
> > ville.syrjala@linux.intel.com; broonie@kernel.org; pierre-
> > louis.bossart@linux.intel.com; Ughreja, Rakesh A
> > <rakesh.a.ughreja@intel.com>
> > Subject: Re: [PATCH 3/7] ALSA: add shell for Intel HDMI LPE audio driver
> > 
> > On Mon, 12 Dec 2016 19:10:39 +0100,
> > Jerome Anand wrote:
> > >
> > > On Baytrail and Cherrytrail, HDaudio may be fused out or disabled by
> > > the BIOS. This driver enables an alternate path to the i915 display
> > > registers and DMA.
> > >
> > > Although there is no hardware path between i915 display and LPE/SST
> > > audio clusters, this HDMI capability is referred to in the
> > > documentation as "HDMI LPE Audio" so we keep the name for consistency.
> > > There is no hardware path or control dependencies with the LPE/SST DSP
> > functionality.
> > >
> > > The hdmi-lpe-audio driver will be probed when the i915 driver creates
> > > a child platform device.
> > >
> > > Since this driver is neither SoC nor PCI, a new x86 folder is added
> > >
> > > Signed-off-by: Pierre-Louis Bossart
> > > <pierre-louis.bossart@linux.intel.com>
> > > Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> > 
> > 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.
> > 
> 
> 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.)

> > > +static inline struct hdmi_lpe_audio_ctx *get_hdmi_context(void) {
> > > +	struct hdmi_lpe_audio_ctx *ctx;
> > > +
> > > +	ctx = platform_get_drvdata(gpdev);
> > > +	return ctx;
> > > +}
> > 
> > Hrm...  Why not storing the audio pdev in i915 pdata?
> > Then the notify callback can extract it easily.
> > 
> 
> The current audio driver interface implementation treats the input pdata as
> Read-only and I don't want to store any audio info in that.

Well, it's not read-only, you already write it to register the
notifier callback :)

thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx