-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, January 23, 2017 4:24 PM To: Anand, Jerome jerome.anand@intel.com; intel- gfx@lists.freedesktop.org; alsa-devel@alsa-project.org Cc: tiwai@suse.de; broonie@kernel.org; Ughreja, Rakesh A rakesh.a.ughreja@intel.com Subject: Re: [Intel-gfx] [PATCH v4 1/5] drm/i915: setup bridge for HDMI LPE audio driver
On Sat, 21 Jan 2017, Jerome Anand jerome.anand@intel.com wrote:
Enable support for HDMI LPE audio mode on Baytrail and Cherrytrail when HDaudio controller is not detected
Setup minimum required resources during i915_driver_load:
- Create a platform device to share MMIO/IRQ resources 2. Make the
platform device child of i915 device for runtime PM. 3. Create IRQ chip to forward HDMI LPE audio irqs.
HDMI LPE audio driver (a standalone sound driver) probes the LPE audio device and creates a new sound card.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Jerome Anand jerome.anand@intel.com
Some comments inline. This is not a detailed technical review, more a high level glance at interfacing with the rest of i915.
Sure - thanks for the quick review Jani
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 4d22b4b..70d728b 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1131,7 +1131,8 @@ static void i915_driver_register(struct
drm_i915_private *dev_priv)
if (IS_GEN5(dev_priv)) intel_gpu_ips_init(dev_priv);
- i915_audio_component_init(dev_priv);
- if (intel_lpe_audio_init(dev_priv) < 0)
i915_audio_component_init(dev_priv);I'd like this abstracted behind a single intel_audio_init() in intel_audio.c, which would do the right thing.
Ok - I'll create two new API's in intel_audio.c
/* * Some ports require correctly set-up hpd registers for detection to @@ -1149,7 +1150,10 @@ static void i915_driver_register(struct
drm_i915_private *dev_priv)
*/ static void i915_driver_unregister(struct drm_i915_private *dev_priv) {
- i915_audio_component_cleanup(dev_priv);
- if (HAS_LPE_AUDIO(dev_priv))
intel_lpe_audio_teardown(dev_priv);- else
i915_audio_component_cleanup(dev_priv);Same here for cleanup.
OK
intel_gpu_ips_teardown(); acpi_video_unregister(); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f66eeede..cc7033a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2449,6 +2449,12 @@ struct drm_i915_private { /* Used to save the pipe-to-encoder mapping for audio */ struct intel_encoder *av_enc_map[I915_MAX_PIPES];
- /* necessary resource sharing with HDMI LPE audio driver. */
- struct {
struct platform_device *platdev;int irq;- } lpe_audio;
I think the av_enc_map array is misplaced within dev_priv, and these should really be closer to the audio_component etc. members. But could be cleaned up later too.
Will cleanup later.
/* * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your
patch
* will be rejected. Instead look for a better place.@@ -2848,6 +2854,8 @@ intel_info(const struct drm_i915_private *dev_priv)
#define HAS_POOLED_EU(dev_priv) ((dev_priv)->info.has_pooled_eu)
+#define HAS_LPE_AUDIO(dev_priv) ((dev_priv)->lpe_audio.platdev != +NULL)
Please note that *all* of our HAS_ macros check for static features of the hardware, not dynamic state of the driver. Let's keep it that way.
OK - will remove it from here.
If you abstract the init/cleanup in intel_audio.c like I suggested, I think you can just check for dev_priv->lpe_audio.platdev directly, and get rid of HAS_LPE_AUDIO() altogether. Then it'll all be within intel_audio.c or intel_lpe_audio.c, and it's pretty obvious what it means.
OK
+/**
- intel_lpe_audio_detect() - check & setup lpe audio if present
- @dev_priv: the i915 drm device private data
- Detect if lpe audio is present
- Return: true if lpe audio present else Return = false */ bool
+intel_lpe_audio_detect(struct drm_i915_private *dev_priv)
Could this not be static?
OK - wanted it to be as part of the interface. Not used anywhere now, so will make it static
+/**
- intel_lpe_audio_setup() - setup the bridge between HDMI LPE Audio
- driver and i915
- @dev_priv: the i915 drm device private data
- set up the minimum required resources for the bridge: irq chip,
- platform resource and platform device. i915 device is set as
+parent
- of the new platform device.
- Return: 0 if successful. non-zero if allocation/initialization
+fails */ int intel_lpe_audio_setup(struct drm_i915_private +*dev_priv)
Could this not be static?
OK
BR, Jani.
-- Jani Nikula, Intel Open Source Technology Center