Re: [PATCH 2/7] drm/i915: Add support for audio driver notifications

[Takashi Iwai <tiwai@suse.de>]

On Mon, 12 Dec 2016 19:10:38 +0100,
Jerome Anand wrote:
> 
> Notifiations like mode change, hot plug and edid to
> the audio driver are added. This is inturn used by the
> audio driver for its functionality.
> 
> A new interface file capturing the notifications needed by the
> audio driver is added
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Jerome Anand <jerome.anand@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  3 +++
>  drivers/gpu/drm/i915/intel_audio.c     |  8 ++++++
>  drivers/gpu/drm/i915/intel_hdmi.c      |  1 +
>  drivers/gpu/drm/i915/intel_lpe_audio.c | 46 ++++++++++++++++++++++++++++++++++
>  include/drm/intel_lpe_audio.h          |  1 +
>  5 files changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1317834..0dbe91a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3696,6 +3696,9 @@ int  intel_lpe_audio_setup(struct drm_i915_private *dev_priv);
>  void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv);
>  void intel_lpe_audio_irq_handler(struct drm_i915_private *dev_priv);
>  bool intel_lpe_audio_detect(struct drm_i915_private *dev_priv);
> +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> +			void *eld, int port, int tmds_clk_speed,
> +			bool connected);
>  
>  /* intel_i2c.c */
>  extern int intel_setup_gmbus(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 3bbc96c..77cd086 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -24,6 +24,7 @@
>  #include <linux/kernel.h>
>  #include <linux/component.h>
>  #include <drm/i915_component.h>
> +#include <drm/intel_lpe_audio.h>
>  #include "intel_drv.h"
>  
>  #include <drm/drmP.h>
> @@ -630,6 +631,10 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder,
>  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
>  						 (int) port, (int) pipe);
> +
> +	if (HAS_LPE_AUDIO(dev_priv))
> +		intel_lpe_audio_notify(dev_priv, connector->eld, port,
> +			crtc_state->port_clock, true);
>  }
>  
>  /**
> @@ -663,6 +668,9 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
>  	if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
>  		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
>  						 (int) port, (int) pipe);
> +
> +	if (HAS_LPE_AUDIO(dev_priv))
> +		intel_lpe_audio_notify(dev_priv, NULL, port, 0, true);

Does it still notify as connected?


>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 0bcfead..377584e1 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -36,6 +36,7 @@
>  #include <drm/drm_edid.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
> +#include <drm/intel_lpe_audio.h>
>  #include "i915_drv.h"
>  
>  static struct drm_device *intel_hdmi_to_dev(struct intel_hdmi *intel_hdmi)
> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> index e12e5f7..a141a9c 100644
> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> @@ -361,3 +361,49 @@ void intel_lpe_audio_teardown(struct drm_i915_private *dev_priv)
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
> +
> +
> +/**
> + * intel_lpe_audio_notify() - notify lpe audio event
> + * audio driver and i915
> + * @dev_priv: the i915 drm device private data
> + * @eld : ELD data
> + * @port: port id
> + * @tmds_clk_speed: tmds clock frequency in Hz
> + * @connected: hdmi connected/disconnected
> + *
> + * Notify lpe audio driver of eld change.
> + */
> +void intel_lpe_audio_notify(struct drm_i915_private *dev_priv,
> +			void *eld, int port, int tmds_clk_speed,
> +			bool connected)
> +{
> +	unsigned long irq_flags;
> +
> +	if (HAS_LPE_AUDIO(dev_priv)) {

Just bail out here when !HAS_LPE_AUDIO().  Then the rest needs less
indentation and will be more readable.

> +		struct intel_hdmi_lpe_audio_pdata *pdata = dev_get_platdata(
> +			&(dev_priv->lpe_audio.platdev->dev));
> +
> +		spin_lock_irqsave(&pdata->lpe_audio_slock,
> +			irq_flags);
> +
> +		if (eld != NULL) {
> +			memcpy(pdata->eld.eld_data, eld,
> +				HDMI_MAX_ELD_BYTES);
> +			pdata->eld.port_id = port;
> +
> +			if (tmds_clk_speed)
> +				pdata->tmds_clock_speed =
> +					tmds_clk_speed;
> +		}

If eld==NULL means that no ELD is found (or disconnected), it's better
to clear pdata eld as well, so that we don't leak the previous ELD.


> +		pdata->hdmi_connected = connected;
> +		if (pdata->notify_audio_lpe)
> +			pdata->notify_audio_lpe(
> +				(eld != NULL) ? &pdata->eld : NULL);
> +		else
> +			pdata->notify_pending = true;
> +
> +		spin_unlock_irqrestore(&pdata->lpe_audio_slock,
> +			irq_flags);
> +	}
> +}
> diff --git a/include/drm/intel_lpe_audio.h b/include/drm/intel_lpe_audio.h
> index a64c449..952de05 100644
> --- a/include/drm/intel_lpe_audio.h
> +++ b/include/drm/intel_lpe_audio.h
> @@ -25,6 +25,7 @@
>  #define _INTEL_LPE_AUDIO_H_
>  
>  #include <linux/types.h>
> +#include <linux/spinlock_types.h>

Why do we need to add a header at this point out of sudden?


thanks,

Takashi


>  
>  #define HDMI_MAX_ELD_BYTES	128
>  
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx