Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3573 bytes --]

On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
> Implement the DSS device driver audio support interface in the HDMI
> panel driver and generic driver. The implementation relies on the
> IP-specific functions that are defined at DSS probe time.
> 
> A HW-safe spinlock is used to protect the audio functions. This is because

What is a "HW-safe spinlock"?

> the audio functions may be called while holding a lock; in such case,
> the panel's driver mutex is not suitable. Functions should be used
> to set registers and should not wait for any other event.

Are you sure this is the only option? What lock is being held? While a
spinlock may be ok for now, quite often enabling/disabling things do not
happen immediately, and it's much easier to do the wait synchronously.

> Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
> ---
>  drivers/video/omap2/dss/dss.h        |    7 +++
>  drivers/video/omap2/dss/hdmi.c       |   33 +++++++++++++++
>  drivers/video/omap2/dss/hdmi_panel.c |   76 ++++++++++++++++++++++++++++++++++
>  3 files changed, 116 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index 32ff69f..fca4490 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -520,6 +520,13 @@ int omapdss_hdmi_read_edid(u8 *buf, int len);
>  bool omapdss_hdmi_detect(void);
>  int hdmi_panel_init(void);
>  void hdmi_panel_exit(void);
> +#ifdef CONFIG_OMAP4_DSS_HDMI_AUDIO
> +int hdmi_audio_enable(bool enable);
> +int hdmi_audio_start(bool start);
> +bool hdmi_mode_has_audio(void);
> +int hdmi_audio_config(struct snd_aes_iec958 *iec,
> +		struct snd_cea_861_aud_if *aud_if);
> +#endif
>  
>  /* RFBI */
>  #ifdef CONFIG_OMAP2_DSS_RFBI
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index bd44891..880509d 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -698,6 +698,39 @@ int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
>  
>  	return 0;
>  }
> +
> +int hdmi_audio_enable(bool enable)
> +{
> +	DSSDBG("audio_enable\n");
> +
> +	hdmi.ip_data.ops->audio_enable(&hdmi.ip_data, enable);

Shouldn't this, and the others below, return the value from the called
function, instead of always returning 0?

> +
> +	return 0;
> +}
> +
> +int hdmi_audio_start(bool start)
> +{
> +	DSSDBG("audio_enable\n");
> +
> +	hdmi.ip_data.ops->audio_start(&hdmi.ip_data, start);
> +
> +	return 0;
> +}
> +
> +bool hdmi_mode_has_audio(void)
> +{
> +	if (hdmi.ip_data.cfg.cm.mode == HDMI_HDMI)
> +		return true;
> +	else
> +		return false;
> +}
> +
> +int hdmi_audio_config(struct snd_aes_iec958 *iec,
> +		struct snd_cea_861_aud_if *aud_if)
> +{
> +	return hdmi.ip_data.ops->audio_config(&hdmi.ip_data, iec, aud_if);
> +}
> +
>  #endif
>  
>  /* HDMI HW IP initialisation */
> diff --git a/drivers/video/omap2/dss/hdmi_panel.c b/drivers/video/omap2/dss/hdmi_panel.c
> index 533d5dc..dac1ac2 100644
> --- a/drivers/video/omap2/dss/hdmi_panel.c
> +++ b/drivers/video/omap2/dss/hdmi_panel.c
> @@ -31,6 +31,10 @@
>  
>  static struct {
>  	struct mutex hdmi_lock;
> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
> +	/* protects calls to HDMI driver audio functionality */
> +	spinlock_t hdmi_sp_lock;

What does "sp" stand for? Spinlock? Perhaps a better name would be
"audio_lock", if it's audio specific. And probably no reason to prefix
it with "hdmi", as it's inside "hdmi" struct already.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]