Re: [PATCH 1/2] OMAPDSS: hdmi: Reconfigure and restart audio when display is enabled

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

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

Hi,

On 26/08/15 16:11, Jyri Sarha wrote:

I few comments, for the parts I had time to review:

> diff --git a/drivers/video/fbdev/omap2/dss/hdmi5.c b/drivers/video/fbdev/omap2/dss/hdmi5.c
> index 7f87578..f352c4b 100644
> --- a/drivers/video/fbdev/omap2/dss/hdmi5.c
> +++ b/drivers/video/fbdev/omap2/dss/hdmi5.c
> @@ -352,6 +352,7 @@ static int read_edid(u8 *buf, int len)
>  static int hdmi_display_enable(struct omap_dss_device *dssdev)
>  {
>  	struct omap_dss_device *out = &hdmi.output;
> +	unsigned long flags;
>  	int r = 0;
>  
>  	DSSDBG("ENTER hdmi_display_enable\n");
> @@ -370,7 +371,37 @@ static int hdmi_display_enable(struct omap_dss_device *dssdev)
>  		goto err0;
>  	}
>  

I think you could refactor parts of the code below into small helper
functions:

> +	if (hdmi.audio_configured) {
> +		spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
> +		hdmi_wp_audio_core_req_enable(&hdmi.wp, false);
> +		hdmi_wp_audio_enable(&hdmi.wp, false);
> +		if (hdmi.wp_idlemode > 0)
> +			REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG,
> +				    hdmi.wp_idlemode, 3, 2);
> +		hdmi.wp_idlemode = -1;

The above looks like it's disabling audio output, the same that's done
in hdmi_audio_stop(). Adding a helper func for that makes the code more
readable.

For the wp_idlemode, I think hdmi.wp_idlemode could be initialized to a
valid value, so that it can be set at any time without the if check above.

> +		spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
> +
> +		r = hdmi5_audio_config(&hdmi.core, &hdmi.wp, &hdmi.audio_config,
> +				       hdmi.cfg.timings.pixelclock);
> +		if (r) {
> +			DSSERR("Error restoring audio configuration: %d", r);
> +			hdmi.audio_abort_cb(&hdmi.pdev->dev);
> +			hdmi.audio_configured = false;
> +		}
> +	}
> +
> +	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
> +	if (hdmi.audio_configured && hdmi.audio_playing) {
> +		/* No-idle while playing audio, store the old value */
> +		hdmi.wp_idlemode =
> +			REG_GET(hdmi.wp.base, HDMI_WP_SYSCONFIG, 3, 2);
> +		REG_FLD_MOD(hdmi.wp.base, HDMI_WP_SYSCONFIG, 1, 3, 2);
> +
> +		hdmi_wp_audio_enable(&hdmi.wp, true);
> +		hdmi_wp_audio_core_req_enable(&hdmi.wp, true);

And here maybe a helper func to enable the audio output.

> +	}

>  	hdmi.display_enabled = true;
> +	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);
>  
>  	mutex_unlock(&hdmi.lock);
>  	return 0;
> @@ -382,17 +413,18 @@ err0:
>  
>  static void hdmi_display_disable(struct omap_dss_device *dssdev)
>  {
> +	unsigned long flags;
> +
>  	DSSDBG("Enter hdmi_display_disable\n");
>  
>  	mutex_lock(&hdmi.lock);
>  
> -	if (hdmi.audio_pdev && hdmi.audio_abort_cb)
> -		hdmi.audio_abort_cb(&hdmi.audio_pdev->dev);
> +	spin_lock_irqsave(&hdmi.audio_playing_lock, flags);
> +	hdmi.display_enabled = false;
> +	spin_unlock_irqrestore(&hdmi.audio_playing_lock, flags);

Shouldn't something be done here in hdmi_display_disable about audio?
You probably want to keep the state flag enabled, so that we know the
user is playing audio, but you could still disable the HDMI audio HW.
I'm sure the audio output dies when the video is disabled, but being
more explicit here makes the code logic easier to follow.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]