On Sat, 21 Jan 2017, Jerome Anand <jerome.anand@xxxxxxxxx> 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:
> 1. 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Jerome Anand <jerome.anand@xxxxxxxxx>
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.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
