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
- Make the platform device child of i915 device for runtime PM.
- 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.
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.
/* * 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.
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.
/* * 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.
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.
+/**
- 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?
+/**
- 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?
BR, Jani.