Re: [PATCH v4 1/5] drm/i915: setup bridge for HDMI LPE audio driver

[Jani Nikula <jani.nikula@linux.intel.com>]

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:
> 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@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.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx