All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC 00/12] tda998x updates
@ 2016-11-08 12:24 ` Russell King - ARM Linux
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 12:24 UTC (permalink / raw)
  To: linux-arm-kernel

As no one responded to the previous round, I'm not spending soo much
time writing up a description of these changes again.  It's also been
quite a long time, so I've forgotten all the details of the changes,
so I'll do my best.

Changes from the previous series include:
- reorder the initial three patches
- change the (now third patch)... I think to increase the size of the
  locked region.
- fix edid parsing for infoframe generation - as was pointed out for
  dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
  will not be called when an override-edid is in effect.  We need to
  parse the override-edid.  Moreover, infoframe generation should not
  be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
  HDMI to send infoframes.
- only send audio if audio and infoframes are supported.

Otherwise, these are very much like the previous posting of the series,
except rebased upon the mali/hdlcd/tda998x change to remove the
drm_connector_register() call.

https://www.spinics.net/lists/dri-devel/msg121495.html

It'd be nice to have other tda998x users ack and test these patches,
I've tried to test on Juno, but the Juno situation seems to be a huge
fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
seems no way to get SCPI support on it - adding the BL0 executable
start address in the SCC registers seems to be incompatible with the
devchip, causing the PLLs to fail.  In discussion with Sudeep over
these issues, but no idea where things are with it at the moment, other
than Sudeep needs to investigate.  All Linaro firmware releases are
broken on HBI0282B.)

 drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
 1 file changed, 429 insertions(+), 397 deletions(-)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
@ 2016-11-08 12:24 ` Russell King - ARM Linux
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 12:24 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

As no one responded to the previous round, I'm not spending soo much
time writing up a description of these changes again.  It's also been
quite a long time, so I've forgotten all the details of the changes,
so I'll do my best.

Changes from the previous series include:
- reorder the initial three patches
- change the (now third patch)... I think to increase the size of the
  locked region.
- fix edid parsing for infoframe generation - as was pointed out for
  dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
  will not be called when an override-edid is in effect.  We need to
  parse the override-edid.  Moreover, infoframe generation should not
  be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
  HDMI to send infoframes.
- only send audio if audio and infoframes are supported.

Otherwise, these are very much like the previous posting of the series,
except rebased upon the mali/hdlcd/tda998x change to remove the
drm_connector_register() call.

https://www.spinics.net/lists/dri-devel/msg121495.html

It'd be nice to have other tda998x users ack and test these patches,
I've tried to test on Juno, but the Juno situation seems to be a huge
fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
seems no way to get SCPI support on it - adding the BL0 executable
start address in the SCC registers seems to be incompatible with the
devchip, causing the PLLs to fail.  In discussion with Sudeep over
these issues, but no idea where things are with it at the moment, other
than Sudeep needs to investigate.  All Linaro firmware releases are
broken on HBI0282B.)

 drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
 1 file changed, 429 insertions(+), 397 deletions(-)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 01/12] drm/i2c: tda998x: avoid race in tda998x_encoder_mode_set()
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

As priv->audio_params can now be changed at run time, we need to be more
careful about how we deal with a mode set.  We must take the audio lock
while checking if there's a valid audio configuration.

However, it's slightly worse than that - during mode set, we mute the
audio, and it must not be unmuted until we have finished the mode set.
It is possible that the audio side may start while a mode set is in
progress, so take the audio_mutex lock around the whole mode setting
procedure.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 088900d78ceb..1cc0433ce9d5 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1074,13 +1074,12 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		if (priv->audio_params.format != AFMT_UNUSED) {
-			mutex_lock(&priv->audio_mutex);
+		mutex_lock(&priv->audio_mutex);
+		if (priv->audio_params.format != AFMT_UNUSED)
 			tda998x_configure_audio(priv,
 						&priv->audio_params,
 						adjusted_mode->clock);
-			mutex_unlock(&priv->audio_mutex);
-		}
+		mutex_unlock(&priv->audio_mutex);
 	}
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 01/12] drm/i2c: tda998x: avoid race in tda998x_encoder_mode_set()
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

As priv->audio_params can now be changed at run time, we need to be more
careful about how we deal with a mode set.  We must take the audio lock
while checking if there's a valid audio configuration.

However, it's slightly worse than that - during mode set, we mute the
audio, and it must not be unmuted until we have finished the mode set.
It is possible that the audio side may start while a mode set is in
progress, so take the audio_mutex lock around the whole mode setting
procedure.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 088900d78ceb..1cc0433ce9d5 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1074,13 +1074,12 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		if (priv->audio_params.format != AFMT_UNUSED) {
-			mutex_lock(&priv->audio_mutex);
+		mutex_lock(&priv->audio_mutex);
+		if (priv->audio_params.format != AFMT_UNUSED)
 			tda998x_configure_audio(priv,
 						&priv->audio_params,
 						adjusted_mode->clock);
-			mutex_unlock(&priv->audio_mutex);
-		}
+		mutex_unlock(&priv->audio_mutex);
 	}
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 02/12] drm/i2c: tda998x: avoid racy access to mode clock
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

Avoid a racy access to the mode clock by storing the current mode clock
during a mode set under the audio mutex.  This allows us to access it
from the audio path in a safe way.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 1cc0433ce9d5..ab58a13d815e 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -47,6 +47,7 @@ struct tda998x_priv {
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
+	unsigned long tmds_clock;
 	struct tda998x_audio_params audio_params;
 
 	struct platform_device *audio_pdev;
@@ -713,8 +714,7 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 
 static int
 tda998x_configure_audio(struct tda998x_priv *priv,
-			struct tda998x_audio_params *params,
-			unsigned mode_clock)
+			struct tda998x_audio_params *params)
 {
 	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv;
 	u32 n;
@@ -771,7 +771,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	 * assume 100MHz requires larger divider.
 	 */
 	adiv = AUDIO_DIV_SERCLK_8;
-	if (mode_clock > 100000)
+	if (priv->tmds_clock > 100000)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 */
 
 	/* S/PDIF asks for a larger divider */
@@ -1064,6 +1064,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	/* must be last register set: */
 	reg_write(priv, REG_TBG_CNTRL_0, 0);
 
+	mutex_lock(&priv->audio_mutex);
+
+	priv->tmds_clock = adjusted_mode->clock;
+
 	/* Only setup the info frames if the sink is HDMI */
 	if (priv->is_hdmi_sink) {
 		/* We need to turn HDMI HDCP stuff on to get audio through */
@@ -1074,13 +1078,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		mutex_lock(&priv->audio_mutex);
 		if (priv->audio_params.format != AFMT_UNUSED)
-			tda998x_configure_audio(priv,
-						&priv->audio_params,
-						adjusted_mode->clock);
-		mutex_unlock(&priv->audio_mutex);
+			tda998x_configure_audio(priv, &priv->audio_params);
 	}
+
+	mutex_unlock(&priv->audio_mutex);
 }
 
 static enum drm_connector_status
@@ -1226,9 +1228,6 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 		.cea = params->cea,
 	};
 
-	if (!priv->encoder.crtc)
-		return -ENODEV;
-
 	memcpy(audio.status, params->iec.status,
 	       min(sizeof(audio.status), sizeof(params->iec.status)));
 
@@ -1264,9 +1263,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 	}
 
 	mutex_lock(&priv->audio_mutex);
-	ret = tda998x_configure_audio(priv,
-				      &audio,
-				      priv->encoder.crtc->hwmode.clock);
+	ret = tda998x_configure_audio(priv, &audio);
 
 	if (ret == 0)
 		priv->audio_params = audio;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 02/12] drm/i2c: tda998x: avoid racy access to mode clock
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

Avoid a racy access to the mode clock by storing the current mode clock
during a mode set under the audio mutex.  This allows us to access it
from the audio path in a safe way.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 1cc0433ce9d5..ab58a13d815e 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -47,6 +47,7 @@ struct tda998x_priv {
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
+	unsigned long tmds_clock;
 	struct tda998x_audio_params audio_params;
 
 	struct platform_device *audio_pdev;
@@ -713,8 +714,7 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 
 static int
 tda998x_configure_audio(struct tda998x_priv *priv,
-			struct tda998x_audio_params *params,
-			unsigned mode_clock)
+			struct tda998x_audio_params *params)
 {
 	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv;
 	u32 n;
@@ -771,7 +771,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	 * assume 100MHz requires larger divider.
 	 */
 	adiv = AUDIO_DIV_SERCLK_8;
-	if (mode_clock > 100000)
+	if (priv->tmds_clock > 100000)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 */
 
 	/* S/PDIF asks for a larger divider */
@@ -1064,6 +1064,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	/* must be last register set: */
 	reg_write(priv, REG_TBG_CNTRL_0, 0);
 
+	mutex_lock(&priv->audio_mutex);
+
+	priv->tmds_clock = adjusted_mode->clock;
+
 	/* Only setup the info frames if the sink is HDMI */
 	if (priv->is_hdmi_sink) {
 		/* We need to turn HDMI HDCP stuff on to get audio through */
@@ -1074,13 +1078,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		mutex_lock(&priv->audio_mutex);
 		if (priv->audio_params.format != AFMT_UNUSED)
-			tda998x_configure_audio(priv,
-						&priv->audio_params,
-						adjusted_mode->clock);
-		mutex_unlock(&priv->audio_mutex);
+			tda998x_configure_audio(priv, &priv->audio_params);
 	}
+
+	mutex_unlock(&priv->audio_mutex);
 }
 
 static enum drm_connector_status
@@ -1226,9 +1228,6 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 		.cea = params->cea,
 	};
 
-	if (!priv->encoder.crtc)
-		return -ENODEV;
-
 	memcpy(audio.status, params->iec.status,
 	       min(sizeof(audio.status), sizeof(params->iec.status)));
 
@@ -1264,9 +1263,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 	}
 
 	mutex_lock(&priv->audio_mutex);
-	ret = tda998x_configure_audio(priv,
-				      &audio,
-				      priv->encoder.crtc->hwmode.clock);
+	ret = tda998x_configure_audio(priv, &audio);
 
 	if (ret == 0)
 		priv->audio_params = audio;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 03/12] drm/i2c: tda998x: avoid race when programming audio
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

Avoid a race between programming audio and an in-progress mode set.
A mode set is complex, and disables the ability to send infoframes
to the sink, and is disruptive to audio - we have to mute the audio
FIFO while doing a mode set.

If an attempt is made to start up the audio side, we will undo the
audio FIFO mute before the mode set has completed.

Move the lock so that we prevent audio interfering with an in-progress
mode set.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index ab58a13d815e..bafe3c7ad84b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -971,6 +971,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			div = 3;
 	}
 
+	mutex_lock(&priv->audio_mutex);
+
 	/* mute the audio FIFO: */
 	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 
@@ -1064,8 +1066,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	/* must be last register set: */
 	reg_write(priv, REG_TBG_CNTRL_0, 0);
 
-	mutex_lock(&priv->audio_mutex);
-
 	priv->tmds_clock = adjusted_mode->clock;
 
 	/* Only setup the info frames if the sink is HDMI */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 03/12] drm/i2c: tda998x: avoid race when programming audio
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

Avoid a race between programming audio and an in-progress mode set.
A mode set is complex, and disables the ability to send infoframes
to the sink, and is disruptive to audio - we have to mute the audio
FIFO while doing a mode set.

If an attempt is made to start up the audio side, we will undo the
audio FIFO mute before the mode set has completed.

Move the lock so that we prevent audio interfering with an in-progress
mode set.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index ab58a13d815e..bafe3c7ad84b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -971,6 +971,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			div = 3;
 	}
 
+	mutex_lock(&priv->audio_mutex);
+
 	/* mute the audio FIFO: */
 	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 
@@ -1064,8 +1066,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	/* must be last register set: */
 	reg_write(priv, REG_TBG_CNTRL_0, 0);
 
-	mutex_lock(&priv->audio_mutex);
-
 	priv->tmds_clock = adjusted_mode->clock;
 
 	/* Only setup the info frames if the sink is HDMI */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 04/12] drm/i2c: tda998x: only configure infoframes and audio if supported
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

The CEA 861B specification indicates the situations when we are able to
send each infoframe based on the version of the EDID's CEA extension.
Update the tda998x driver to follow the CEA specification wrt sending
of infoframes.

Since we only support the generation of AVI version 2, this limits us
to CEA extension version 3, so we treat CEA extension version 2 as
CEA 861 (no infoframes, no audio.)

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index bafe3c7ad84b..75833a4650e8 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -43,7 +43,7 @@ struct tda998x_priv {
 	u16 rev;
 	u8 current_page;
 	int dpms;
-	bool is_hdmi_sink;
+	bool supports_infoframes;
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
@@ -1068,8 +1068,20 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 	priv->tmds_clock = adjusted_mode->clock;
 
-	/* Only setup the info frames if the sink is HDMI */
-	if (priv->is_hdmi_sink) {
+	/* CEA-861B section 6 says that:
+	 * CEA version 1 (CEA-861) has no support for infoframes.
+	 * CEA version 2 (CEA-861A) supports version 1 AVI infoframes,
+	 * and optional basic audio.
+	 * CEA version 3 (CEA-861B) supports version 1 and 2 AVI infoframes,
+	 * and optional digital audio, with audio infoframes.
+	 *
+	 * Since we only support generation of version 2 AVI infoframes,
+	 * ignore CEA version 2 and below (iow, behave as if we're a
+	 * CEA-861 source.)
+	 */
+	priv->supports_infoframes = priv->connector.display_info.cea_rev >= 3;
+
+	if (priv->supports_infoframes) {
 		/* We need to turn HDMI HDCP stuff on to get audio through */
 		reg &= ~TBG_CNTRL_1_DWIN_DIS;
 		reg_write(priv, REG_TBG_CNTRL_1, reg);
@@ -1180,7 +1192,6 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
 
 	drm_mode_connector_update_edid_property(connector, edid);
 	n = drm_add_edid_modes(connector, edid);
-	priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
 	drm_edid_to_eld(connector, edid);
 
 	kfree(edid);
@@ -1263,7 +1274,10 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 	}
 
 	mutex_lock(&priv->audio_mutex);
-	ret = tda998x_configure_audio(priv, &audio);
+	if (priv->supports_infoframes)
+		ret = tda998x_configure_audio(priv, &audio);
+	else
+		ret = 0;
 
 	if (ret == 0)
 		priv->audio_params = audio;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 04/12] drm/i2c: tda998x: only configure infoframes and audio if supported
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

The CEA 861B specification indicates the situations when we are able to
send each infoframe based on the version of the EDID's CEA extension.
Update the tda998x driver to follow the CEA specification wrt sending
of infoframes.

Since we only support the generation of AVI version 2, this limits us
to CEA extension version 3, so we treat CEA extension version 2 as
CEA 861 (no infoframes, no audio.)

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index bafe3c7ad84b..75833a4650e8 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -43,7 +43,7 @@ struct tda998x_priv {
 	u16 rev;
 	u8 current_page;
 	int dpms;
-	bool is_hdmi_sink;
+	bool supports_infoframes;
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
@@ -1068,8 +1068,20 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 	priv->tmds_clock = adjusted_mode->clock;
 
-	/* Only setup the info frames if the sink is HDMI */
-	if (priv->is_hdmi_sink) {
+	/* CEA-861B section 6 says that:
+	 * CEA version 1 (CEA-861) has no support for infoframes.
+	 * CEA version 2 (CEA-861A) supports version 1 AVI infoframes,
+	 * and optional basic audio.
+	 * CEA version 3 (CEA-861B) supports version 1 and 2 AVI infoframes,
+	 * and optional digital audio, with audio infoframes.
+	 *
+	 * Since we only support generation of version 2 AVI infoframes,
+	 * ignore CEA version 2 and below (iow, behave as if we're a
+	 * CEA-861 source.)
+	 */
+	priv->supports_infoframes = priv->connector.display_info.cea_rev >= 3;
+
+	if (priv->supports_infoframes) {
 		/* We need to turn HDMI HDCP stuff on to get audio through */
 		reg &= ~TBG_CNTRL_1_DWIN_DIS;
 		reg_write(priv, REG_TBG_CNTRL_1, reg);
@@ -1180,7 +1192,6 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
 
 	drm_mode_connector_update_edid_property(connector, edid);
 	n = drm_add_edid_modes(connector, edid);
-	priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
 	drm_edid_to_eld(connector, edid);
 
 	kfree(edid);
@@ -1263,7 +1274,10 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 	}
 
 	mutex_lock(&priv->audio_mutex);
-	ret = tda998x_configure_audio(priv, &audio);
+	if (priv->supports_infoframes)
+		ret = tda998x_configure_audio(priv, &audio);
+	else
+		ret = 0;
 
 	if (ret == 0)
 		priv->audio_params = audio;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 05/12] drm/i2c: tda998x: only enable audio if supported by sink
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

Check for audio support by the attached sink by consulting the EDID
prior to enabling audio over the TMDS link.  We must consult the EDID
after calling drm_helper_probe_single_connector_modes(), as this can
use an override EDID, or load a replacement EDID.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 75833a4650e8..fa13ac0e8691 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -44,6 +44,7 @@ struct tda998x_priv {
 	u8 current_page;
 	int dpms;
 	bool supports_infoframes;
+	bool sink_has_audio;
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
@@ -1090,13 +1091,33 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		if (priv->audio_params.format != AFMT_UNUSED)
+		if (priv->audio_params.format != AFMT_UNUSED &&
+		    priv->sink_has_audio)
 			tda998x_configure_audio(priv, &priv->audio_params);
 	}
 
 	mutex_unlock(&priv->audio_mutex);
 }
 
+static int tda998x_connector_fill_modes(struct drm_connector *connector,
+					uint32_t maxX, uint32_t maxY)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	int ret;
+
+	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
+
+	if (connector->edid_blob_ptr) {
+		struct edid *edid = (void *)connector->edid_blob_ptr->data;
+
+		priv->sink_has_audio = drm_detect_monitor_audio(edid);
+	} else {
+		priv->sink_has_audio = false;
+	}
+
+	return ret;
+}
+
 static enum drm_connector_status
 tda998x_connector_detect(struct drm_connector *connector, bool force)
 {
@@ -1274,7 +1295,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 	}
 
 	mutex_lock(&priv->audio_mutex);
-	if (priv->supports_infoframes)
+	if (priv->supports_infoframes && priv->sink_has_audio)
 		ret = tda998x_configure_audio(priv, &audio);
 	else
 		ret = 0;
@@ -1608,7 +1629,7 @@ static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
 static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.dpms = tda998x_connector_dpms,
 	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = drm_helper_probe_single_connector_modes,
+	.fill_modes = tda998x_connector_fill_modes,
 	.detect = tda998x_connector_detect,
 	.destroy = tda998x_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 05/12] drm/i2c: tda998x: only enable audio if supported by sink
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

Check for audio support by the attached sink by consulting the EDID
prior to enabling audio over the TMDS link.  We must consult the EDID
after calling drm_helper_probe_single_connector_modes(), as this can
use an override EDID, or load a replacement EDID.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 75833a4650e8..fa13ac0e8691 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -44,6 +44,7 @@ struct tda998x_priv {
 	u8 current_page;
 	int dpms;
 	bool supports_infoframes;
+	bool sink_has_audio;
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
@@ -1090,13 +1091,33 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		if (priv->audio_params.format != AFMT_UNUSED)
+		if (priv->audio_params.format != AFMT_UNUSED &&
+		    priv->sink_has_audio)
 			tda998x_configure_audio(priv, &priv->audio_params);
 	}
 
 	mutex_unlock(&priv->audio_mutex);
 }
 
+static int tda998x_connector_fill_modes(struct drm_connector *connector,
+					uint32_t maxX, uint32_t maxY)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	int ret;
+
+	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
+
+	if (connector->edid_blob_ptr) {
+		struct edid *edid = (void *)connector->edid_blob_ptr->data;
+
+		priv->sink_has_audio = drm_detect_monitor_audio(edid);
+	} else {
+		priv->sink_has_audio = false;
+	}
+
+	return ret;
+}
+
 static enum drm_connector_status
 tda998x_connector_detect(struct drm_connector *connector, bool force)
 {
@@ -1274,7 +1295,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 	}
 
 	mutex_lock(&priv->audio_mutex);
-	if (priv->supports_infoframes)
+	if (priv->supports_infoframes && priv->sink_has_audio)
 		ret = tda998x_configure_audio(priv, &audio);
 	else
 		ret = 0;
@@ -1608,7 +1629,7 @@ static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
 static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.dpms = tda998x_connector_dpms,
 	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = drm_helper_probe_single_connector_modes,
+	.fill_modes = tda998x_connector_fill_modes,
 	.detect = tda998x_connector_detect,
 	.destroy = tda998x_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 06/12] drm/i2c: tda998x: correct function name in comments
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

Correct two references to tda998x_connector_get_modes() which were
incorrectly referring to tda998x_encoder_get_modes().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index fa13ac0e8691..349e04a30210 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -581,9 +581,9 @@ tda998x_reset(struct tda998x_priv *priv)
  * HPD assertion: it needs a delay of 100ms to avoid timing out while
  * trying to read EDID data.
  *
- * However, tda998x_encoder_get_modes() may be called at any moment
+ * However, tda998x_connector_get_modes() may be called at any moment
  * after tda998x_connector_detect() indicates that we are connected, so
- * we need to delay probing modes in tda998x_encoder_get_modes() after
+ * we need to delay probing modes in tda998x_connector_get_modes() after
  * we have seen a HPD inactive->active transition.  This code implements
  * that delay.
  */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 06/12] drm/i2c: tda998x: correct function name in comments
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

Correct two references to tda998x_connector_get_modes() which were
incorrectly referring to tda998x_encoder_get_modes().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index fa13ac0e8691..349e04a30210 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -581,9 +581,9 @@ tda998x_reset(struct tda998x_priv *priv)
  * HPD assertion: it needs a delay of 100ms to avoid timing out while
  * trying to read EDID data.
  *
- * However, tda998x_encoder_get_modes() may be called at any moment
+ * However, tda998x_connector_get_modes() may be called at any moment
  * after tda998x_connector_detect() indicates that we are connected, so
- * we need to delay probing modes in tda998x_encoder_get_modes() after
+ * we need to delay probing modes in tda998x_connector_get_modes() after
  * we have seen a HPD inactive->active transition.  This code implements
  * that delay.
  */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 07/12] drm/i2c: tda998x: move and rename tda998x_encoder_set_config()
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

The naming of tda998x_encoder_set_config() is a left-over from when
TDA998x was a slave encoder.  Since this is part of the initialisation,
drop the _encoder from the name, and move it near tda998x_bind().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 40 +++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 349e04a30210..6b966c19a449 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -822,25 +822,6 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 
 /* DRM encoder functions */
 
-static void tda998x_encoder_set_config(struct tda998x_priv *priv,
-				       const struct tda998x_encoder_params *p)
-{
-	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
-			    (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
-			    VIP_CNTRL_0_SWAP_B(p->swap_b) |
-			    (p->mirr_b ? VIP_CNTRL_0_MIRR_B : 0);
-	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(p->swap_c) |
-			    (p->mirr_c ? VIP_CNTRL_1_MIRR_C : 0) |
-			    VIP_CNTRL_1_SWAP_D(p->swap_d) |
-			    (p->mirr_d ? VIP_CNTRL_1_MIRR_D : 0);
-	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(p->swap_e) |
-			    (p->mirr_e ? VIP_CNTRL_2_MIRR_E : 0) |
-			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
-			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
-
-	priv->audio_params = p->audio_params;
-}
-
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
 	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
@@ -1636,6 +1617,25 @@ static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static void tda998x_set_config(struct tda998x_priv *priv,
+			       const struct tda998x_encoder_params *p)
+{
+	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
+			    (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
+			    VIP_CNTRL_0_SWAP_B(p->swap_b) |
+			    (p->mirr_b ? VIP_CNTRL_0_MIRR_B : 0);
+	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(p->swap_c) |
+			    (p->mirr_c ? VIP_CNTRL_1_MIRR_C : 0) |
+			    VIP_CNTRL_1_SWAP_D(p->swap_d) |
+			    (p->mirr_d ? VIP_CNTRL_1_MIRR_D : 0);
+	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(p->swap_e) |
+			    (p->mirr_e ? VIP_CNTRL_2_MIRR_E : 0) |
+			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
+			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
+
+	priv->audio_params = p->audio_params;
+}
+
 static int tda998x_bind(struct device *dev, struct device *master, void *data)
 {
 	struct tda998x_encoder_params *params = dev->platform_data;
@@ -1668,7 +1668,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 
 	if (!dev->of_node && params)
-		tda998x_encoder_set_config(priv, params);
+		tda998x_set_config(priv, params);
 
 	tda998x_encoder_set_polling(priv, &priv->connector);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 07/12] drm/i2c: tda998x: move and rename tda998x_encoder_set_config()
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

The naming of tda998x_encoder_set_config() is a left-over from when
TDA998x was a slave encoder.  Since this is part of the initialisation,
drop the _encoder from the name, and move it near tda998x_bind().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 40 +++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 349e04a30210..6b966c19a449 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -822,25 +822,6 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 
 /* DRM encoder functions */
 
-static void tda998x_encoder_set_config(struct tda998x_priv *priv,
-				       const struct tda998x_encoder_params *p)
-{
-	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
-			    (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
-			    VIP_CNTRL_0_SWAP_B(p->swap_b) |
-			    (p->mirr_b ? VIP_CNTRL_0_MIRR_B : 0);
-	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(p->swap_c) |
-			    (p->mirr_c ? VIP_CNTRL_1_MIRR_C : 0) |
-			    VIP_CNTRL_1_SWAP_D(p->swap_d) |
-			    (p->mirr_d ? VIP_CNTRL_1_MIRR_D : 0);
-	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(p->swap_e) |
-			    (p->mirr_e ? VIP_CNTRL_2_MIRR_E : 0) |
-			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
-			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
-
-	priv->audio_params = p->audio_params;
-}
-
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
 	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
@@ -1636,6 +1617,25 @@ static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static void tda998x_set_config(struct tda998x_priv *priv,
+			       const struct tda998x_encoder_params *p)
+{
+	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
+			    (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
+			    VIP_CNTRL_0_SWAP_B(p->swap_b) |
+			    (p->mirr_b ? VIP_CNTRL_0_MIRR_B : 0);
+	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(p->swap_c) |
+			    (p->mirr_c ? VIP_CNTRL_1_MIRR_C : 0) |
+			    VIP_CNTRL_1_SWAP_D(p->swap_d) |
+			    (p->mirr_d ? VIP_CNTRL_1_MIRR_D : 0);
+	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(p->swap_e) |
+			    (p->mirr_e ? VIP_CNTRL_2_MIRR_E : 0) |
+			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
+			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
+
+	priv->audio_params = p->audio_params;
+}
+
 static int tda998x_bind(struct device *dev, struct device *master, void *data)
 {
 	struct tda998x_encoder_params *params = dev->platform_data;
@@ -1668,7 +1668,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 
 	if (!dev->of_node && params)
-		tda998x_encoder_set_config(priv, params);
+		tda998x_set_config(priv, params);
 
 	tda998x_encoder_set_polling(priv, &priv->connector);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 08/12] drm/i2c: tda998x: group connector functions and funcs together
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

Group the TDA998x connector functions and funcs structures together
before the encoder support, rather than scattered amongst the rest of
the file.  This keeps like code together.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 350 +++++++++++++++++++-------------------
 1 file changed, 176 insertions(+), 174 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 6b966c19a449..3f3e1f0137f8 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -820,6 +820,182 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	return tda998x_write_aif(priv, &params->cea);
 }
 
+/* DRM connector functions */
+
+static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
+{
+	if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
+		return drm_atomic_helper_connector_dpms(connector, mode);
+	else
+		return drm_helper_connector_dpms(connector, mode);
+}
+
+static int tda998x_connector_fill_modes(struct drm_connector *connector,
+					uint32_t maxX, uint32_t maxY)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	int ret;
+
+	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
+
+	if (connector->edid_blob_ptr) {
+		struct edid *edid = (void *)connector->edid_blob_ptr->data;
+
+		priv->sink_has_audio = drm_detect_monitor_audio(edid);
+	} else {
+		priv->sink_has_audio = false;
+	}
+
+	return ret;
+}
+
+static enum drm_connector_status
+tda998x_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	u8 val = cec_read(priv, REG_CEC_RXSHPDLEV);
+
+	return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
+			connector_status_disconnected;
+}
+
+static void tda998x_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs tda998x_connector_funcs = {
+	.dpms = tda998x_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = tda998x_connector_fill_modes,
+	.detect = tda998x_connector_detect,
+	.destroy = tda998x_connector_destroy,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length)
+{
+	struct tda998x_priv *priv = data;
+	u8 offset, segptr;
+	int ret, i;
+
+	offset = (blk & 1) ? 128 : 0;
+	segptr = blk / 2;
+
+	reg_write(priv, REG_DDC_ADDR, 0xa0);
+	reg_write(priv, REG_DDC_OFFS, offset);
+	reg_write(priv, REG_DDC_SEGM_ADDR, 0x60);
+	reg_write(priv, REG_DDC_SEGM, segptr);
+
+	/* enable reading EDID: */
+	priv->wq_edid_wait = 1;
+	reg_write(priv, REG_EDID_CTRL, 0x1);
+
+	/* flag must be cleared by sw: */
+	reg_write(priv, REG_EDID_CTRL, 0x0);
+
+	/* wait for block read to complete: */
+	if (priv->hdmi->irq) {
+		i = wait_event_timeout(priv->wq_edid,
+					!priv->wq_edid_wait,
+					msecs_to_jiffies(100));
+		if (i < 0) {
+			dev_err(&priv->hdmi->dev, "read edid wait err %d\n", i);
+			return i;
+		}
+	} else {
+		for (i = 100; i > 0; i--) {
+			msleep(1);
+			ret = reg_read(priv, REG_INT_FLAGS_2);
+			if (ret < 0)
+				return ret;
+			if (ret & INT_FLAGS_2_EDID_BLK_RD)
+				break;
+		}
+	}
+
+	if (i == 0) {
+		dev_err(&priv->hdmi->dev, "read edid timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	ret = reg_read_range(priv, REG_EDID_DATA_0, buf, length);
+	if (ret != length) {
+		dev_err(&priv->hdmi->dev, "failed to read edid block %d: %d\n",
+			blk, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tda998x_connector_get_modes(struct drm_connector *connector)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	struct edid *edid;
+	int n;
+
+	/*
+	 * If we get killed while waiting for the HPD timeout, return
+	 * no modes found: we are not in a restartable path, so we
+	 * can't handle signals gracefully.
+	 */
+	if (tda998x_edid_delay_wait(priv))
+		return 0;
+
+	if (priv->rev == TDA19988)
+		reg_clear(priv, REG_TX4, TX4_PD_RAM);
+
+	edid = drm_do_get_edid(connector, read_edid_block, priv);
+
+	if (priv->rev == TDA19988)
+		reg_set(priv, REG_TX4, TX4_PD_RAM);
+
+	if (!edid) {
+		dev_warn(&priv->hdmi->dev, "failed to read EDID\n");
+		return 0;
+	}
+
+	drm_mode_connector_update_edid_property(connector, edid);
+	n = drm_add_edid_modes(connector, edid);
+	drm_edid_to_eld(connector, edid);
+
+	kfree(edid);
+
+	return n;
+}
+
+static int tda998x_connector_mode_valid(struct drm_connector *connector,
+					struct drm_display_mode *mode)
+{
+	/* TDA19988 dotclock can go up to 165MHz */
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+
+	if (mode->clock > ((priv->rev == TDA19988) ? 165000 : 150000))
+		return MODE_CLOCK_HIGH;
+	if (mode->htotal >= BIT(13))
+		return MODE_BAD_HVALUE;
+	if (mode->vtotal >= BIT(11))
+		return MODE_BAD_VVALUE;
+	return MODE_OK;
+}
+
+static struct drm_encoder *
+tda998x_connector_best_encoder(struct drm_connector *connector)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+
+	return &priv->encoder;
+}
+
+static
+const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = {
+	.get_modes = tda998x_connector_get_modes,
+	.mode_valid = tda998x_connector_mode_valid,
+	.best_encoder = tda998x_connector_best_encoder,
+};
+
 /* DRM encoder functions */
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -855,21 +1031,6 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 	priv->dpms = mode;
 }
 
-static int tda998x_connector_mode_valid(struct drm_connector *connector,
-					struct drm_display_mode *mode)
-{
-	/* TDA19988 dotclock can go up to 165MHz */
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-
-	if (mode->clock > ((priv->rev == TDA19988) ? 165000 : 150000))
-		return MODE_CLOCK_HIGH;
-	if (mode->htotal >= BIT(13))
-		return MODE_BAD_HVALUE;
-	if (mode->vtotal >= BIT(11))
-		return MODE_BAD_VVALUE;
-	return MODE_OK;
-}
-
 static void
 tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			 struct drm_display_mode *mode,
@@ -1080,127 +1241,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	mutex_unlock(&priv->audio_mutex);
 }
 
-static int tda998x_connector_fill_modes(struct drm_connector *connector,
-					uint32_t maxX, uint32_t maxY)
-{
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-	int ret;
-
-	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
-
-	if (connector->edid_blob_ptr) {
-		struct edid *edid = (void *)connector->edid_blob_ptr->data;
-
-		priv->sink_has_audio = drm_detect_monitor_audio(edid);
-	} else {
-		priv->sink_has_audio = false;
-	}
-
-	return ret;
-}
-
-static enum drm_connector_status
-tda998x_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-	u8 val = cec_read(priv, REG_CEC_RXSHPDLEV);
-
-	return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
-			connector_status_disconnected;
-}
-
-static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length)
-{
-	struct tda998x_priv *priv = data;
-	u8 offset, segptr;
-	int ret, i;
-
-	offset = (blk & 1) ? 128 : 0;
-	segptr = blk / 2;
-
-	reg_write(priv, REG_DDC_ADDR, 0xa0);
-	reg_write(priv, REG_DDC_OFFS, offset);
-	reg_write(priv, REG_DDC_SEGM_ADDR, 0x60);
-	reg_write(priv, REG_DDC_SEGM, segptr);
-
-	/* enable reading EDID: */
-	priv->wq_edid_wait = 1;
-	reg_write(priv, REG_EDID_CTRL, 0x1);
-
-	/* flag must be cleared by sw: */
-	reg_write(priv, REG_EDID_CTRL, 0x0);
-
-	/* wait for block read to complete: */
-	if (priv->hdmi->irq) {
-		i = wait_event_timeout(priv->wq_edid,
-					!priv->wq_edid_wait,
-					msecs_to_jiffies(100));
-		if (i < 0) {
-			dev_err(&priv->hdmi->dev, "read edid wait err %d\n", i);
-			return i;
-		}
-	} else {
-		for (i = 100; i > 0; i--) {
-			msleep(1);
-			ret = reg_read(priv, REG_INT_FLAGS_2);
-			if (ret < 0)
-				return ret;
-			if (ret & INT_FLAGS_2_EDID_BLK_RD)
-				break;
-		}
-	}
-
-	if (i == 0) {
-		dev_err(&priv->hdmi->dev, "read edid timeout\n");
-		return -ETIMEDOUT;
-	}
-
-	ret = reg_read_range(priv, REG_EDID_DATA_0, buf, length);
-	if (ret != length) {
-		dev_err(&priv->hdmi->dev, "failed to read edid block %d: %d\n",
-			blk, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int tda998x_connector_get_modes(struct drm_connector *connector)
-{
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-	struct edid *edid;
-	int n;
-
-	/*
-	 * If we get killed while waiting for the HPD timeout, return
-	 * no modes found: we are not in a restartable path, so we
-	 * can't handle signals gracefully.
-	 */
-	if (tda998x_edid_delay_wait(priv))
-		return 0;
-
-	if (priv->rev == TDA19988)
-		reg_clear(priv, REG_TX4, TX4_PD_RAM);
-
-	edid = drm_do_get_edid(connector, read_edid_block, priv);
-
-	if (priv->rev == TDA19988)
-		reg_set(priv, REG_TX4, TX4_PD_RAM);
-
-	if (!edid) {
-		dev_warn(&priv->hdmi->dev, "failed to read EDID\n");
-		return 0;
-	}
-
-	drm_mode_connector_update_edid_property(connector, edid);
-	n = drm_add_edid_modes(connector, edid);
-	drm_edid_to_eld(connector, edid);
-
-	kfree(edid);
-
-	return n;
-}
-
 static void tda998x_encoder_set_polling(struct tda998x_priv *priv,
 					struct drm_connector *connector)
 {
@@ -1579,44 +1619,6 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = {
 	.destroy = tda998x_encoder_destroy,
 };
 
-static struct drm_encoder *
-tda998x_connector_best_encoder(struct drm_connector *connector)
-{
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-
-	return &priv->encoder;
-}
-
-static
-const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = {
-	.get_modes = tda998x_connector_get_modes,
-	.mode_valid = tda998x_connector_mode_valid,
-	.best_encoder = tda998x_connector_best_encoder,
-};
-
-static void tda998x_connector_destroy(struct drm_connector *connector)
-{
-	drm_connector_cleanup(connector);
-}
-
-static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
-{
-	if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
-		return drm_atomic_helper_connector_dpms(connector, mode);
-	else
-		return drm_helper_connector_dpms(connector, mode);
-}
-
-static const struct drm_connector_funcs tda998x_connector_funcs = {
-	.dpms = tda998x_connector_dpms,
-	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = tda998x_connector_fill_modes,
-	.detect = tda998x_connector_detect,
-	.destroy = tda998x_connector_destroy,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
 static void tda998x_set_config(struct tda998x_priv *priv,
 			       const struct tda998x_encoder_params *p)
 {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 08/12] drm/i2c: tda998x: group connector functions and funcs together
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

Group the TDA998x connector functions and funcs structures together
before the encoder support, rather than scattered amongst the rest of
the file.  This keeps like code together.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 350 +++++++++++++++++++-------------------
 1 file changed, 176 insertions(+), 174 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 6b966c19a449..3f3e1f0137f8 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -820,6 +820,182 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	return tda998x_write_aif(priv, &params->cea);
 }
 
+/* DRM connector functions */
+
+static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
+{
+	if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
+		return drm_atomic_helper_connector_dpms(connector, mode);
+	else
+		return drm_helper_connector_dpms(connector, mode);
+}
+
+static int tda998x_connector_fill_modes(struct drm_connector *connector,
+					uint32_t maxX, uint32_t maxY)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	int ret;
+
+	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
+
+	if (connector->edid_blob_ptr) {
+		struct edid *edid = (void *)connector->edid_blob_ptr->data;
+
+		priv->sink_has_audio = drm_detect_monitor_audio(edid);
+	} else {
+		priv->sink_has_audio = false;
+	}
+
+	return ret;
+}
+
+static enum drm_connector_status
+tda998x_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	u8 val = cec_read(priv, REG_CEC_RXSHPDLEV);
+
+	return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
+			connector_status_disconnected;
+}
+
+static void tda998x_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs tda998x_connector_funcs = {
+	.dpms = tda998x_connector_dpms,
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = tda998x_connector_fill_modes,
+	.detect = tda998x_connector_detect,
+	.destroy = tda998x_connector_destroy,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length)
+{
+	struct tda998x_priv *priv = data;
+	u8 offset, segptr;
+	int ret, i;
+
+	offset = (blk & 1) ? 128 : 0;
+	segptr = blk / 2;
+
+	reg_write(priv, REG_DDC_ADDR, 0xa0);
+	reg_write(priv, REG_DDC_OFFS, offset);
+	reg_write(priv, REG_DDC_SEGM_ADDR, 0x60);
+	reg_write(priv, REG_DDC_SEGM, segptr);
+
+	/* enable reading EDID: */
+	priv->wq_edid_wait = 1;
+	reg_write(priv, REG_EDID_CTRL, 0x1);
+
+	/* flag must be cleared by sw: */
+	reg_write(priv, REG_EDID_CTRL, 0x0);
+
+	/* wait for block read to complete: */
+	if (priv->hdmi->irq) {
+		i = wait_event_timeout(priv->wq_edid,
+					!priv->wq_edid_wait,
+					msecs_to_jiffies(100));
+		if (i < 0) {
+			dev_err(&priv->hdmi->dev, "read edid wait err %d\n", i);
+			return i;
+		}
+	} else {
+		for (i = 100; i > 0; i--) {
+			msleep(1);
+			ret = reg_read(priv, REG_INT_FLAGS_2);
+			if (ret < 0)
+				return ret;
+			if (ret & INT_FLAGS_2_EDID_BLK_RD)
+				break;
+		}
+	}
+
+	if (i == 0) {
+		dev_err(&priv->hdmi->dev, "read edid timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	ret = reg_read_range(priv, REG_EDID_DATA_0, buf, length);
+	if (ret != length) {
+		dev_err(&priv->hdmi->dev, "failed to read edid block %d: %d\n",
+			blk, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tda998x_connector_get_modes(struct drm_connector *connector)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	struct edid *edid;
+	int n;
+
+	/*
+	 * If we get killed while waiting for the HPD timeout, return
+	 * no modes found: we are not in a restartable path, so we
+	 * can't handle signals gracefully.
+	 */
+	if (tda998x_edid_delay_wait(priv))
+		return 0;
+
+	if (priv->rev == TDA19988)
+		reg_clear(priv, REG_TX4, TX4_PD_RAM);
+
+	edid = drm_do_get_edid(connector, read_edid_block, priv);
+
+	if (priv->rev == TDA19988)
+		reg_set(priv, REG_TX4, TX4_PD_RAM);
+
+	if (!edid) {
+		dev_warn(&priv->hdmi->dev, "failed to read EDID\n");
+		return 0;
+	}
+
+	drm_mode_connector_update_edid_property(connector, edid);
+	n = drm_add_edid_modes(connector, edid);
+	drm_edid_to_eld(connector, edid);
+
+	kfree(edid);
+
+	return n;
+}
+
+static int tda998x_connector_mode_valid(struct drm_connector *connector,
+					struct drm_display_mode *mode)
+{
+	/* TDA19988 dotclock can go up to 165MHz */
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+
+	if (mode->clock > ((priv->rev == TDA19988) ? 165000 : 150000))
+		return MODE_CLOCK_HIGH;
+	if (mode->htotal >= BIT(13))
+		return MODE_BAD_HVALUE;
+	if (mode->vtotal >= BIT(11))
+		return MODE_BAD_VVALUE;
+	return MODE_OK;
+}
+
+static struct drm_encoder *
+tda998x_connector_best_encoder(struct drm_connector *connector)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+
+	return &priv->encoder;
+}
+
+static
+const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = {
+	.get_modes = tda998x_connector_get_modes,
+	.mode_valid = tda998x_connector_mode_valid,
+	.best_encoder = tda998x_connector_best_encoder,
+};
+
 /* DRM encoder functions */
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -855,21 +1031,6 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 	priv->dpms = mode;
 }
 
-static int tda998x_connector_mode_valid(struct drm_connector *connector,
-					struct drm_display_mode *mode)
-{
-	/* TDA19988 dotclock can go up to 165MHz */
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-
-	if (mode->clock > ((priv->rev == TDA19988) ? 165000 : 150000))
-		return MODE_CLOCK_HIGH;
-	if (mode->htotal >= BIT(13))
-		return MODE_BAD_HVALUE;
-	if (mode->vtotal >= BIT(11))
-		return MODE_BAD_VVALUE;
-	return MODE_OK;
-}
-
 static void
 tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			 struct drm_display_mode *mode,
@@ -1080,127 +1241,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	mutex_unlock(&priv->audio_mutex);
 }
 
-static int tda998x_connector_fill_modes(struct drm_connector *connector,
-					uint32_t maxX, uint32_t maxY)
-{
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-	int ret;
-
-	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
-
-	if (connector->edid_blob_ptr) {
-		struct edid *edid = (void *)connector->edid_blob_ptr->data;
-
-		priv->sink_has_audio = drm_detect_monitor_audio(edid);
-	} else {
-		priv->sink_has_audio = false;
-	}
-
-	return ret;
-}
-
-static enum drm_connector_status
-tda998x_connector_detect(struct drm_connector *connector, bool force)
-{
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-	u8 val = cec_read(priv, REG_CEC_RXSHPDLEV);
-
-	return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
-			connector_status_disconnected;
-}
-
-static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length)
-{
-	struct tda998x_priv *priv = data;
-	u8 offset, segptr;
-	int ret, i;
-
-	offset = (blk & 1) ? 128 : 0;
-	segptr = blk / 2;
-
-	reg_write(priv, REG_DDC_ADDR, 0xa0);
-	reg_write(priv, REG_DDC_OFFS, offset);
-	reg_write(priv, REG_DDC_SEGM_ADDR, 0x60);
-	reg_write(priv, REG_DDC_SEGM, segptr);
-
-	/* enable reading EDID: */
-	priv->wq_edid_wait = 1;
-	reg_write(priv, REG_EDID_CTRL, 0x1);
-
-	/* flag must be cleared by sw: */
-	reg_write(priv, REG_EDID_CTRL, 0x0);
-
-	/* wait for block read to complete: */
-	if (priv->hdmi->irq) {
-		i = wait_event_timeout(priv->wq_edid,
-					!priv->wq_edid_wait,
-					msecs_to_jiffies(100));
-		if (i < 0) {
-			dev_err(&priv->hdmi->dev, "read edid wait err %d\n", i);
-			return i;
-		}
-	} else {
-		for (i = 100; i > 0; i--) {
-			msleep(1);
-			ret = reg_read(priv, REG_INT_FLAGS_2);
-			if (ret < 0)
-				return ret;
-			if (ret & INT_FLAGS_2_EDID_BLK_RD)
-				break;
-		}
-	}
-
-	if (i == 0) {
-		dev_err(&priv->hdmi->dev, "read edid timeout\n");
-		return -ETIMEDOUT;
-	}
-
-	ret = reg_read_range(priv, REG_EDID_DATA_0, buf, length);
-	if (ret != length) {
-		dev_err(&priv->hdmi->dev, "failed to read edid block %d: %d\n",
-			blk, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
-static int tda998x_connector_get_modes(struct drm_connector *connector)
-{
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-	struct edid *edid;
-	int n;
-
-	/*
-	 * If we get killed while waiting for the HPD timeout, return
-	 * no modes found: we are not in a restartable path, so we
-	 * can't handle signals gracefully.
-	 */
-	if (tda998x_edid_delay_wait(priv))
-		return 0;
-
-	if (priv->rev == TDA19988)
-		reg_clear(priv, REG_TX4, TX4_PD_RAM);
-
-	edid = drm_do_get_edid(connector, read_edid_block, priv);
-
-	if (priv->rev == TDA19988)
-		reg_set(priv, REG_TX4, TX4_PD_RAM);
-
-	if (!edid) {
-		dev_warn(&priv->hdmi->dev, "failed to read EDID\n");
-		return 0;
-	}
-
-	drm_mode_connector_update_edid_property(connector, edid);
-	n = drm_add_edid_modes(connector, edid);
-	drm_edid_to_eld(connector, edid);
-
-	kfree(edid);
-
-	return n;
-}
-
 static void tda998x_encoder_set_polling(struct tda998x_priv *priv,
 					struct drm_connector *connector)
 {
@@ -1579,44 +1619,6 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = {
 	.destroy = tda998x_encoder_destroy,
 };
 
-static struct drm_encoder *
-tda998x_connector_best_encoder(struct drm_connector *connector)
-{
-	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
-
-	return &priv->encoder;
-}
-
-static
-const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = {
-	.get_modes = tda998x_connector_get_modes,
-	.mode_valid = tda998x_connector_mode_valid,
-	.best_encoder = tda998x_connector_best_encoder,
-};
-
-static void tda998x_connector_destroy(struct drm_connector *connector)
-{
-	drm_connector_cleanup(connector);
-}
-
-static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
-{
-	if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC))
-		return drm_atomic_helper_connector_dpms(connector, mode);
-	else
-		return drm_helper_connector_dpms(connector, mode);
-}
-
-static const struct drm_connector_funcs tda998x_connector_funcs = {
-	.dpms = tda998x_connector_dpms,
-	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = tda998x_connector_fill_modes,
-	.detect = tda998x_connector_detect,
-	.destroy = tda998x_connector_destroy,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
 static void tda998x_set_config(struct tda998x_priv *priv,
 			       const struct tda998x_encoder_params *p)
 {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 09/12] drm/i2c: tda998x: separate connector initialisation
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

Separate out the connector initialisation from the rest of the drivers
initialisation.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 46 ++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 3f3e1f0137f8..af3dc5528663 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -996,6 +996,31 @@ const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = {
 	.best_encoder = tda998x_connector_best_encoder,
 };
 
+static int tda998x_connector_init(struct tda998x_priv *priv,
+				  struct drm_device *drm)
+{
+	struct drm_connector *connector = &priv->connector;
+	int ret;
+
+	connector->interlace_allowed = 1;
+
+	if (priv->hdmi->irq)
+		connector->polled = DRM_CONNECTOR_POLL_HPD;
+	else
+		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			DRM_CONNECTOR_POLL_DISCONNECT;
+
+	drm_connector_helper_add(connector, &tda998x_connector_helper_funcs);
+	ret = drm_connector_init(drm, connector, &tda998x_connector_funcs,
+				 DRM_MODE_CONNECTOR_HDMIA);
+	if (ret)
+		return ret;
+
+	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
+
+	return 0;
+}
+
 /* DRM encoder functions */
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -1241,16 +1266,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	mutex_unlock(&priv->audio_mutex);
 }
 
-static void tda998x_encoder_set_polling(struct tda998x_priv *priv,
-					struct drm_connector *connector)
-{
-	if (priv->hdmi->irq)
-		connector->polled = DRM_CONNECTOR_POLL_HPD;
-	else
-		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			DRM_CONNECTOR_POLL_DISCONNECT;
-}
-
 static void tda998x_destroy(struct tda998x_priv *priv)
 {
 	/* disable all IRQs and free the IRQ handler */
@@ -1662,7 +1677,6 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 		crtcs = 1 << 0;
 	}
 
-	priv->connector.interlace_allowed = 1;
 	priv->encoder.possible_crtcs = crtcs;
 
 	ret = tda998x_create(client, priv);
@@ -1672,24 +1686,16 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	if (!dev->of_node && params)
 		tda998x_set_config(priv, params);
 
-	tda998x_encoder_set_polling(priv, &priv->connector);
-
 	drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs);
 	ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs,
 			       DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret)
 		goto err_encoder;
 
-	drm_connector_helper_add(&priv->connector,
-				 &tda998x_connector_helper_funcs);
-	ret = drm_connector_init(drm, &priv->connector,
-				 &tda998x_connector_funcs,
-				 DRM_MODE_CONNECTOR_HDMIA);
+	ret = tda998x_connector_init(priv, drm);
 	if (ret)
 		goto err_connector;
 
-	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
-
 	return 0;
 
 err_connector:
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 09/12] drm/i2c: tda998x: separate connector initialisation
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

Separate out the connector initialisation from the rest of the drivers
initialisation.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 46 ++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 3f3e1f0137f8..af3dc5528663 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -996,6 +996,31 @@ const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = {
 	.best_encoder = tda998x_connector_best_encoder,
 };
 
+static int tda998x_connector_init(struct tda998x_priv *priv,
+				  struct drm_device *drm)
+{
+	struct drm_connector *connector = &priv->connector;
+	int ret;
+
+	connector->interlace_allowed = 1;
+
+	if (priv->hdmi->irq)
+		connector->polled = DRM_CONNECTOR_POLL_HPD;
+	else
+		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			DRM_CONNECTOR_POLL_DISCONNECT;
+
+	drm_connector_helper_add(connector, &tda998x_connector_helper_funcs);
+	ret = drm_connector_init(drm, connector, &tda998x_connector_funcs,
+				 DRM_MODE_CONNECTOR_HDMIA);
+	if (ret)
+		return ret;
+
+	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
+
+	return 0;
+}
+
 /* DRM encoder functions */
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -1241,16 +1266,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	mutex_unlock(&priv->audio_mutex);
 }
 
-static void tda998x_encoder_set_polling(struct tda998x_priv *priv,
-					struct drm_connector *connector)
-{
-	if (priv->hdmi->irq)
-		connector->polled = DRM_CONNECTOR_POLL_HPD;
-	else
-		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
-			DRM_CONNECTOR_POLL_DISCONNECT;
-}
-
 static void tda998x_destroy(struct tda998x_priv *priv)
 {
 	/* disable all IRQs and free the IRQ handler */
@@ -1662,7 +1677,6 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 		crtcs = 1 << 0;
 	}
 
-	priv->connector.interlace_allowed = 1;
 	priv->encoder.possible_crtcs = crtcs;
 
 	ret = tda998x_create(client, priv);
@@ -1672,24 +1686,16 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	if (!dev->of_node && params)
 		tda998x_set_config(priv, params);
 
-	tda998x_encoder_set_polling(priv, &priv->connector);
-
 	drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs);
 	ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs,
 			       DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret)
 		goto err_encoder;
 
-	drm_connector_helper_add(&priv->connector,
-				 &tda998x_connector_helper_funcs);
-	ret = drm_connector_init(drm, &priv->connector,
-				 &tda998x_connector_funcs,
-				 DRM_MODE_CONNECTOR_HDMIA);
+	ret = tda998x_connector_init(priv, drm);
 	if (ret)
 		goto err_connector;
 
-	drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
-
 	return 0;
 
 err_connector:
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 10/12] drm/i2c: tda998x: group audio functions together
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

Group the TDA998x audio functions together rather than split between
two different locations in the file, keeping like code together.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 276 +++++++++++++++++++-------------------
 1 file changed, 139 insertions(+), 137 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index af3dc5528663..b1ad5ed5d645 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -702,6 +702,8 @@ tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
 	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);
 }
 
+/* Audio support */
+
 static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 {
 	if (on) {
@@ -820,6 +822,143 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	return tda998x_write_aif(priv, &params->cea);
 }
 
+static int tda998x_audio_hw_params(struct device *dev, void *data,
+				   struct hdmi_codec_daifmt *daifmt,
+				   struct hdmi_codec_params *params)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	int i, ret;
+	struct tda998x_audio_params audio = {
+		.sample_width = params->sample_width,
+		.sample_rate = params->sample_rate,
+		.cea = params->cea,
+	};
+
+	memcpy(audio.status, params->iec.status,
+	       min(sizeof(audio.status), sizeof(params->iec.status)));
+
+	switch (daifmt->fmt) {
+	case HDMI_I2S:
+		if (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
+		    daifmt->bit_clk_master || daifmt->frame_clk_master) {
+			dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
+				daifmt->bit_clk_inv, daifmt->frame_clk_inv,
+				daifmt->bit_clk_master,
+				daifmt->frame_clk_master);
+			return -EINVAL;
+		}
+		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+			if (priv->audio_port[i].format == AFMT_I2S)
+				audio.config = priv->audio_port[i].config;
+		audio.format = AFMT_I2S;
+		break;
+	case HDMI_SPDIF:
+		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+			if (priv->audio_port[i].format == AFMT_SPDIF)
+				audio.config = priv->audio_port[i].config;
+		audio.format = AFMT_SPDIF;
+		break;
+	default:
+		dev_err(dev, "%s: Invalid format %d\n", __func__, daifmt->fmt);
+		return -EINVAL;
+	}
+
+	if (audio.config == 0) {
+		dev_err(dev, "%s: No audio configutation found\n", __func__);
+		return -EINVAL;
+	}
+
+	mutex_lock(&priv->audio_mutex);
+	if (priv->supports_infoframes && priv->sink_has_audio)
+		ret = tda998x_configure_audio(priv, &audio);
+	else
+		ret = 0;
+
+	if (ret == 0)
+		priv->audio_params = audio;
+	mutex_unlock(&priv->audio_mutex);
+
+	return ret;
+}
+
+static void tda998x_audio_shutdown(struct device *dev, void *data)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	mutex_lock(&priv->audio_mutex);
+
+	reg_write(priv, REG_ENA_AP, 0);
+
+	priv->audio_params.format = AFMT_UNUSED;
+
+	mutex_unlock(&priv->audio_mutex);
+}
+
+int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	mutex_lock(&priv->audio_mutex);
+
+	tda998x_audio_mute(priv, enable);
+
+	mutex_unlock(&priv->audio_mutex);
+	return 0;
+}
+
+static int tda998x_audio_get_eld(struct device *dev, void *data,
+				 uint8_t *buf, size_t len)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct drm_mode_config *config = &priv->encoder.dev->mode_config;
+	struct drm_connector *connector;
+	int ret = -ENODEV;
+
+	mutex_lock(&config->mutex);
+	list_for_each_entry(connector, &config->connector_list, head) {
+		if (&priv->encoder == connector->encoder) {
+			memcpy(buf, connector->eld,
+			       min(sizeof(connector->eld), len));
+			ret = 0;
+		}
+	}
+	mutex_unlock(&config->mutex);
+
+	return ret;
+}
+
+static const struct hdmi_codec_ops audio_codec_ops = {
+	.hw_params = tda998x_audio_hw_params,
+	.audio_shutdown = tda998x_audio_shutdown,
+	.digital_mute = tda998x_audio_digital_mute,
+	.get_eld = tda998x_audio_get_eld,
+};
+
+static int tda998x_audio_codec_init(struct tda998x_priv *priv,
+				    struct device *dev)
+{
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &audio_codec_ops,
+		.max_i2s_channels = 2,
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) {
+		if (priv->audio_port[i].format == AFMT_I2S &&
+		    priv->audio_port[i].config != 0)
+			codec_data.i2s = 1;
+		if (priv->audio_port[i].format == AFMT_SPDIF &&
+		    priv->audio_port[i].config != 0)
+			codec_data.spdif = 1;
+	}
+
+	priv->audio_pdev = platform_device_register_data(
+		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
+		&codec_data, sizeof(codec_data));
+
+	return PTR_ERR_OR_ZERO(priv->audio_pdev);
+}
+
 /* DRM connector functions */
 
 static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
@@ -1284,143 +1423,6 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 	i2c_unregister_device(priv->cec);
 }
 
-static int tda998x_audio_hw_params(struct device *dev, void *data,
-				   struct hdmi_codec_daifmt *daifmt,
-				   struct hdmi_codec_params *params)
-{
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
-	int i, ret;
-	struct tda998x_audio_params audio = {
-		.sample_width = params->sample_width,
-		.sample_rate = params->sample_rate,
-		.cea = params->cea,
-	};
-
-	memcpy(audio.status, params->iec.status,
-	       min(sizeof(audio.status), sizeof(params->iec.status)));
-
-	switch (daifmt->fmt) {
-	case HDMI_I2S:
-		if (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
-		    daifmt->bit_clk_master || daifmt->frame_clk_master) {
-			dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
-				daifmt->bit_clk_inv, daifmt->frame_clk_inv,
-				daifmt->bit_clk_master,
-				daifmt->frame_clk_master);
-			return -EINVAL;
-		}
-		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
-			if (priv->audio_port[i].format == AFMT_I2S)
-				audio.config = priv->audio_port[i].config;
-		audio.format = AFMT_I2S;
-		break;
-	case HDMI_SPDIF:
-		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
-			if (priv->audio_port[i].format == AFMT_SPDIF)
-				audio.config = priv->audio_port[i].config;
-		audio.format = AFMT_SPDIF;
-		break;
-	default:
-		dev_err(dev, "%s: Invalid format %d\n", __func__, daifmt->fmt);
-		return -EINVAL;
-	}
-
-	if (audio.config == 0) {
-		dev_err(dev, "%s: No audio configutation found\n", __func__);
-		return -EINVAL;
-	}
-
-	mutex_lock(&priv->audio_mutex);
-	if (priv->supports_infoframes && priv->sink_has_audio)
-		ret = tda998x_configure_audio(priv, &audio);
-	else
-		ret = 0;
-
-	if (ret == 0)
-		priv->audio_params = audio;
-	mutex_unlock(&priv->audio_mutex);
-
-	return ret;
-}
-
-static void tda998x_audio_shutdown(struct device *dev, void *data)
-{
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
-
-	mutex_lock(&priv->audio_mutex);
-
-	reg_write(priv, REG_ENA_AP, 0);
-
-	priv->audio_params.format = AFMT_UNUSED;
-
-	mutex_unlock(&priv->audio_mutex);
-}
-
-int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
-{
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
-
-	mutex_lock(&priv->audio_mutex);
-
-	tda998x_audio_mute(priv, enable);
-
-	mutex_unlock(&priv->audio_mutex);
-	return 0;
-}
-
-static int tda998x_audio_get_eld(struct device *dev, void *data,
-				 uint8_t *buf, size_t len)
-{
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
-	struct drm_mode_config *config = &priv->encoder.dev->mode_config;
-	struct drm_connector *connector;
-	int ret = -ENODEV;
-
-	mutex_lock(&config->mutex);
-	list_for_each_entry(connector, &config->connector_list, head) {
-		if (&priv->encoder == connector->encoder) {
-			memcpy(buf, connector->eld,
-			       min(sizeof(connector->eld), len));
-			ret = 0;
-		}
-	}
-	mutex_unlock(&config->mutex);
-
-	return ret;
-}
-
-static const struct hdmi_codec_ops audio_codec_ops = {
-	.hw_params = tda998x_audio_hw_params,
-	.audio_shutdown = tda998x_audio_shutdown,
-	.digital_mute = tda998x_audio_digital_mute,
-	.get_eld = tda998x_audio_get_eld,
-};
-
-static int tda998x_audio_codec_init(struct tda998x_priv *priv,
-				    struct device *dev)
-{
-	struct hdmi_codec_pdata codec_data = {
-		.ops = &audio_codec_ops,
-		.max_i2s_channels = 2,
-	};
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) {
-		if (priv->audio_port[i].format == AFMT_I2S &&
-		    priv->audio_port[i].config != 0)
-			codec_data.i2s = 1;
-		if (priv->audio_port[i].format == AFMT_SPDIF &&
-		    priv->audio_port[i].config != 0)
-			codec_data.spdif = 1;
-	}
-
-	priv->audio_pdev = platform_device_register_data(
-		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
-		&codec_data, sizeof(codec_data));
-
-	return PTR_ERR_OR_ZERO(priv->audio_pdev);
-}
-
 /* I2C driver functions */
 
 static int tda998x_get_audio_ports(struct tda998x_priv *priv,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 10/12] drm/i2c: tda998x: group audio functions together
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

Group the TDA998x audio functions together rather than split between
two different locations in the file, keeping like code together.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 276 +++++++++++++++++++-------------------
 1 file changed, 139 insertions(+), 137 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index af3dc5528663..b1ad5ed5d645 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -702,6 +702,8 @@ tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
 	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame);
 }
 
+/* Audio support */
+
 static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 {
 	if (on) {
@@ -820,6 +822,143 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	return tda998x_write_aif(priv, &params->cea);
 }
 
+static int tda998x_audio_hw_params(struct device *dev, void *data,
+				   struct hdmi_codec_daifmt *daifmt,
+				   struct hdmi_codec_params *params)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	int i, ret;
+	struct tda998x_audio_params audio = {
+		.sample_width = params->sample_width,
+		.sample_rate = params->sample_rate,
+		.cea = params->cea,
+	};
+
+	memcpy(audio.status, params->iec.status,
+	       min(sizeof(audio.status), sizeof(params->iec.status)));
+
+	switch (daifmt->fmt) {
+	case HDMI_I2S:
+		if (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
+		    daifmt->bit_clk_master || daifmt->frame_clk_master) {
+			dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
+				daifmt->bit_clk_inv, daifmt->frame_clk_inv,
+				daifmt->bit_clk_master,
+				daifmt->frame_clk_master);
+			return -EINVAL;
+		}
+		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+			if (priv->audio_port[i].format == AFMT_I2S)
+				audio.config = priv->audio_port[i].config;
+		audio.format = AFMT_I2S;
+		break;
+	case HDMI_SPDIF:
+		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+			if (priv->audio_port[i].format == AFMT_SPDIF)
+				audio.config = priv->audio_port[i].config;
+		audio.format = AFMT_SPDIF;
+		break;
+	default:
+		dev_err(dev, "%s: Invalid format %d\n", __func__, daifmt->fmt);
+		return -EINVAL;
+	}
+
+	if (audio.config == 0) {
+		dev_err(dev, "%s: No audio configutation found\n", __func__);
+		return -EINVAL;
+	}
+
+	mutex_lock(&priv->audio_mutex);
+	if (priv->supports_infoframes && priv->sink_has_audio)
+		ret = tda998x_configure_audio(priv, &audio);
+	else
+		ret = 0;
+
+	if (ret == 0)
+		priv->audio_params = audio;
+	mutex_unlock(&priv->audio_mutex);
+
+	return ret;
+}
+
+static void tda998x_audio_shutdown(struct device *dev, void *data)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	mutex_lock(&priv->audio_mutex);
+
+	reg_write(priv, REG_ENA_AP, 0);
+
+	priv->audio_params.format = AFMT_UNUSED;
+
+	mutex_unlock(&priv->audio_mutex);
+}
+
+int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	mutex_lock(&priv->audio_mutex);
+
+	tda998x_audio_mute(priv, enable);
+
+	mutex_unlock(&priv->audio_mutex);
+	return 0;
+}
+
+static int tda998x_audio_get_eld(struct device *dev, void *data,
+				 uint8_t *buf, size_t len)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct drm_mode_config *config = &priv->encoder.dev->mode_config;
+	struct drm_connector *connector;
+	int ret = -ENODEV;
+
+	mutex_lock(&config->mutex);
+	list_for_each_entry(connector, &config->connector_list, head) {
+		if (&priv->encoder == connector->encoder) {
+			memcpy(buf, connector->eld,
+			       min(sizeof(connector->eld), len));
+			ret = 0;
+		}
+	}
+	mutex_unlock(&config->mutex);
+
+	return ret;
+}
+
+static const struct hdmi_codec_ops audio_codec_ops = {
+	.hw_params = tda998x_audio_hw_params,
+	.audio_shutdown = tda998x_audio_shutdown,
+	.digital_mute = tda998x_audio_digital_mute,
+	.get_eld = tda998x_audio_get_eld,
+};
+
+static int tda998x_audio_codec_init(struct tda998x_priv *priv,
+				    struct device *dev)
+{
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &audio_codec_ops,
+		.max_i2s_channels = 2,
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) {
+		if (priv->audio_port[i].format == AFMT_I2S &&
+		    priv->audio_port[i].config != 0)
+			codec_data.i2s = 1;
+		if (priv->audio_port[i].format == AFMT_SPDIF &&
+		    priv->audio_port[i].config != 0)
+			codec_data.spdif = 1;
+	}
+
+	priv->audio_pdev = platform_device_register_data(
+		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
+		&codec_data, sizeof(codec_data));
+
+	return PTR_ERR_OR_ZERO(priv->audio_pdev);
+}
+
 /* DRM connector functions */
 
 static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
@@ -1284,143 +1423,6 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 	i2c_unregister_device(priv->cec);
 }
 
-static int tda998x_audio_hw_params(struct device *dev, void *data,
-				   struct hdmi_codec_daifmt *daifmt,
-				   struct hdmi_codec_params *params)
-{
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
-	int i, ret;
-	struct tda998x_audio_params audio = {
-		.sample_width = params->sample_width,
-		.sample_rate = params->sample_rate,
-		.cea = params->cea,
-	};
-
-	memcpy(audio.status, params->iec.status,
-	       min(sizeof(audio.status), sizeof(params->iec.status)));
-
-	switch (daifmt->fmt) {
-	case HDMI_I2S:
-		if (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
-		    daifmt->bit_clk_master || daifmt->frame_clk_master) {
-			dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
-				daifmt->bit_clk_inv, daifmt->frame_clk_inv,
-				daifmt->bit_clk_master,
-				daifmt->frame_clk_master);
-			return -EINVAL;
-		}
-		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
-			if (priv->audio_port[i].format == AFMT_I2S)
-				audio.config = priv->audio_port[i].config;
-		audio.format = AFMT_I2S;
-		break;
-	case HDMI_SPDIF:
-		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
-			if (priv->audio_port[i].format == AFMT_SPDIF)
-				audio.config = priv->audio_port[i].config;
-		audio.format = AFMT_SPDIF;
-		break;
-	default:
-		dev_err(dev, "%s: Invalid format %d\n", __func__, daifmt->fmt);
-		return -EINVAL;
-	}
-
-	if (audio.config == 0) {
-		dev_err(dev, "%s: No audio configutation found\n", __func__);
-		return -EINVAL;
-	}
-
-	mutex_lock(&priv->audio_mutex);
-	if (priv->supports_infoframes && priv->sink_has_audio)
-		ret = tda998x_configure_audio(priv, &audio);
-	else
-		ret = 0;
-
-	if (ret == 0)
-		priv->audio_params = audio;
-	mutex_unlock(&priv->audio_mutex);
-
-	return ret;
-}
-
-static void tda998x_audio_shutdown(struct device *dev, void *data)
-{
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
-
-	mutex_lock(&priv->audio_mutex);
-
-	reg_write(priv, REG_ENA_AP, 0);
-
-	priv->audio_params.format = AFMT_UNUSED;
-
-	mutex_unlock(&priv->audio_mutex);
-}
-
-int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable)
-{
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
-
-	mutex_lock(&priv->audio_mutex);
-
-	tda998x_audio_mute(priv, enable);
-
-	mutex_unlock(&priv->audio_mutex);
-	return 0;
-}
-
-static int tda998x_audio_get_eld(struct device *dev, void *data,
-				 uint8_t *buf, size_t len)
-{
-	struct tda998x_priv *priv = dev_get_drvdata(dev);
-	struct drm_mode_config *config = &priv->encoder.dev->mode_config;
-	struct drm_connector *connector;
-	int ret = -ENODEV;
-
-	mutex_lock(&config->mutex);
-	list_for_each_entry(connector, &config->connector_list, head) {
-		if (&priv->encoder == connector->encoder) {
-			memcpy(buf, connector->eld,
-			       min(sizeof(connector->eld), len));
-			ret = 0;
-		}
-	}
-	mutex_unlock(&config->mutex);
-
-	return ret;
-}
-
-static const struct hdmi_codec_ops audio_codec_ops = {
-	.hw_params = tda998x_audio_hw_params,
-	.audio_shutdown = tda998x_audio_shutdown,
-	.digital_mute = tda998x_audio_digital_mute,
-	.get_eld = tda998x_audio_get_eld,
-};
-
-static int tda998x_audio_codec_init(struct tda998x_priv *priv,
-				    struct device *dev)
-{
-	struct hdmi_codec_pdata codec_data = {
-		.ops = &audio_codec_ops,
-		.max_i2s_channels = 2,
-	};
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) {
-		if (priv->audio_port[i].format == AFMT_I2S &&
-		    priv->audio_port[i].config != 0)
-			codec_data.i2s = 1;
-		if (priv->audio_port[i].format == AFMT_SPDIF &&
-		    priv->audio_port[i].config != 0)
-			codec_data.spdif = 1;
-	}
-
-	priv->audio_pdev = platform_device_register_data(
-		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
-		&codec_data, sizeof(codec_data));
-
-	return PTR_ERR_OR_ZERO(priv->audio_pdev);
-}
-
 /* I2C driver functions */
 
 static int tda998x_get_audio_ports(struct tda998x_priv *priv,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 11/12] drm/i2c: tda998x: remove complexity from tda998x_audio_get_eld()
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

tda998x_audio_get_eld() is needlessly complex - the connector associated
with the encoder is always our own priv->connector.  Remove this
complexity, but ensure that there are no races when copying out the ELD.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b1ad5ed5d645..226871e34442 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -910,21 +910,13 @@ static int tda998x_audio_get_eld(struct device *dev, void *data,
 				 uint8_t *buf, size_t len)
 {
 	struct tda998x_priv *priv = dev_get_drvdata(dev);
-	struct drm_mode_config *config = &priv->encoder.dev->mode_config;
-	struct drm_connector *connector;
-	int ret = -ENODEV;
-
-	mutex_lock(&config->mutex);
-	list_for_each_entry(connector, &config->connector_list, head) {
-		if (&priv->encoder == connector->encoder) {
-			memcpy(buf, connector->eld,
-			       min(sizeof(connector->eld), len));
-			ret = 0;
-		}
-	}
-	mutex_unlock(&config->mutex);
 
-	return ret;
+	mutex_lock(&priv->audio_mutex);
+	memcpy(buf, priv->connector.eld,
+	       min(sizeof(priv->connector.eld), len));
+	mutex_unlock(&priv->audio_mutex);
+
+	return 0;
 }
 
 static const struct hdmi_codec_ops audio_codec_ops = {
@@ -975,6 +967,7 @@ static int tda998x_connector_fill_modes(struct drm_connector *connector,
 	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
 	int ret;
 
+	mutex_lock(&priv->audio_mutex);
 	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
 
 	if (connector->edid_blob_ptr) {
@@ -984,6 +977,7 @@ static int tda998x_connector_fill_modes(struct drm_connector *connector,
 	} else {
 		priv->sink_has_audio = false;
 	}
+	mutex_unlock(&priv->audio_mutex);
 
 	return ret;
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 11/12] drm/i2c: tda998x: remove complexity from tda998x_audio_get_eld()
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

tda998x_audio_get_eld() is needlessly complex - the connector associated
with the encoder is always our own priv->connector.  Remove this
complexity, but ensure that there are no races when copying out the ELD.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b1ad5ed5d645..226871e34442 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -910,21 +910,13 @@ static int tda998x_audio_get_eld(struct device *dev, void *data,
 				 uint8_t *buf, size_t len)
 {
 	struct tda998x_priv *priv = dev_get_drvdata(dev);
-	struct drm_mode_config *config = &priv->encoder.dev->mode_config;
-	struct drm_connector *connector;
-	int ret = -ENODEV;
-
-	mutex_lock(&config->mutex);
-	list_for_each_entry(connector, &config->connector_list, head) {
-		if (&priv->encoder == connector->encoder) {
-			memcpy(buf, connector->eld,
-			       min(sizeof(connector->eld), len));
-			ret = 0;
-		}
-	}
-	mutex_unlock(&config->mutex);
 
-	return ret;
+	mutex_lock(&priv->audio_mutex);
+	memcpy(buf, priv->connector.eld,
+	       min(sizeof(priv->connector.eld), len));
+	mutex_unlock(&priv->audio_mutex);
+
+	return 0;
 }
 
 static const struct hdmi_codec_ops audio_codec_ops = {
@@ -975,6 +967,7 @@ static int tda998x_connector_fill_modes(struct drm_connector *connector,
 	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
 	int ret;
 
+	mutex_lock(&priv->audio_mutex);
 	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
 
 	if (connector->edid_blob_ptr) {
@@ -984,6 +977,7 @@ static int tda998x_connector_fill_modes(struct drm_connector *connector,
 	} else {
 		priv->sink_has_audio = false;
 	}
+	mutex_unlock(&priv->audio_mutex);
 
 	return ret;
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 12/12] drm/i2c: tda998x: switch to boolean is_on
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 12:25   ` Russell King
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: linux-arm-kernel

Rather than storing the DPMS mode (which will always be on or off) use a
boolean to store this instead.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 226871e34442..3a5e5c466972 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -42,7 +42,7 @@ struct tda998x_priv {
 	struct mutex mutex;
 	u16 rev;
 	u8 current_page;
-	int dpms;
+	bool is_on;
 	bool supports_infoframes;
 	bool sink_has_audio;
 	u8 vip_cntrl_0;
@@ -1159,16 +1159,15 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
 	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+	bool on;
 
 	/* we only care about on or off: */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
+	on = mode == DRM_MODE_DPMS_ON;
 
-	if (mode == priv->dpms)
+	if (on == priv->is_on)
 		return;
 
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
+	if (on) {
 		/* enable video ports, audio will be enabled later */
 		reg_write(priv, REG_ENA_VP_0, 0xff);
 		reg_write(priv, REG_ENA_VP_1, 0xff);
@@ -1177,16 +1176,16 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 		reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
 		reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
 		reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
-		break;
-	case DRM_MODE_DPMS_OFF:
+
+		priv->is_on = true;
+	} else {
 		/* disable video ports */
 		reg_write(priv, REG_ENA_VP_0, 0x00);
 		reg_write(priv, REG_ENA_VP_1, 0x00);
 		reg_write(priv, REG_ENA_VP_2, 0x00);
-		break;
-	}
 
-	priv->dpms = mode;
+		priv->is_on = false;
+	}
 }
 
 static void
@@ -1480,8 +1479,6 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	if (!priv->cec)
 		return -ENODEV;
 
-	priv->dpms = DRM_MODE_DPMS_OFF;
-
 	mutex_init(&priv->mutex);	/* protect the page access */
 	init_waitqueue_head(&priv->edid_delay_waitq);
 	setup_timer(&priv->edid_delay_timer, tda998x_edid_delay_done,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 12/12] drm/i2c: tda998x: switch to boolean is_on
@ 2016-11-08 12:25   ` Russell King
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King @ 2016-11-08 12:25 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: linux-arm-kernel, dri-devel

Rather than storing the DPMS mode (which will always be on or off) use a
boolean to store this instead.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 226871e34442..3a5e5c466972 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -42,7 +42,7 @@ struct tda998x_priv {
 	struct mutex mutex;
 	u16 rev;
 	u8 current_page;
-	int dpms;
+	bool is_on;
 	bool supports_infoframes;
 	bool sink_has_audio;
 	u8 vip_cntrl_0;
@@ -1159,16 +1159,15 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
 	struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+	bool on;
 
 	/* we only care about on or off: */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
+	on = mode == DRM_MODE_DPMS_ON;
 
-	if (mode == priv->dpms)
+	if (on == priv->is_on)
 		return;
 
-	switch (mode) {
-	case DRM_MODE_DPMS_ON:
+	if (on) {
 		/* enable video ports, audio will be enabled later */
 		reg_write(priv, REG_ENA_VP_0, 0xff);
 		reg_write(priv, REG_ENA_VP_1, 0xff);
@@ -1177,16 +1176,16 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 		reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
 		reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
 		reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
-		break;
-	case DRM_MODE_DPMS_OFF:
+
+		priv->is_on = true;
+	} else {
 		/* disable video ports */
 		reg_write(priv, REG_ENA_VP_0, 0x00);
 		reg_write(priv, REG_ENA_VP_1, 0x00);
 		reg_write(priv, REG_ENA_VP_2, 0x00);
-		break;
-	}
 
-	priv->dpms = mode;
+		priv->is_on = false;
+	}
 }
 
 static void
@@ -1480,8 +1479,6 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	if (!priv->cec)
 		return -ENODEV;
 
-	priv->dpms = DRM_MODE_DPMS_OFF;
-
 	mutex_init(&priv->mutex);	/* protect the page access */
 	init_waitqueue_head(&priv->edid_delay_waitq);
 	setup_timer(&priv->edid_delay_timer, tda998x_edid_delay_done,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-08 13:19   ` Robin Murphy
  -1 siblings, 0 replies; 52+ messages in thread
From: Robin Murphy @ 2016-11-08 13:19 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Russell,

On 08/11/16 12:24, Russell King - ARM Linux wrote:
> As no one responded to the previous round, I'm not spending soo much
> time writing up a description of these changes again.  It's also been
> quite a long time, so I've forgotten all the details of the changes,
> so I'll do my best.
> 
> Changes from the previous series include:
> - reorder the initial three patches
> - change the (now third patch)... I think to increase the size of the
>   locked region.
> - fix edid parsing for infoframe generation - as was pointed out for
>   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
>   will not be called when an override-edid is in effect.  We need to
>   parse the override-edid.  Moreover, infoframe generation should not
>   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
>   HDMI to send infoframes.
> - only send audio if audio and infoframes are supported.
> 
> Otherwise, these are very much like the previous posting of the series,
> except rebased upon the mali/hdlcd/tda998x change to remove the
> drm_connector_register() call.
> 
> https://www.spinics.net/lists/dri-devel/msg121495.html
> 
> It'd be nice to have other tda998x users ack and test these patches,

I've just merged your drm-tda998x-devel branch and booted it on my Juno,
and I can at least confirm that DVI still seems to still work as before.
Anything in particular I should be looking out for? (Unfortunately the
only handy HDMI monitor is one of those slightly-out-of-spec ones which
has never really worked with the pixel clocks Juno provides)

Robin.

> I've tried to test on Juno, but the Juno situation seems to be a huge
> fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
> incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
> seems no way to get SCPI support on it - adding the BL0 executable
> start address in the SCC registers seems to be incompatible with the
> devchip, causing the PLLs to fail.  In discussion with Sudeep over
> these issues, but no idea where things are with it at the moment, other
> than Sudeep needs to investigate.  All Linaro firmware releases are
> broken on HBI0282B.)
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
>  1 file changed, 429 insertions(+), 397 deletions(-)
> 

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-08 13:19   ` Robin Murphy
  0 siblings, 0 replies; 52+ messages in thread
From: Robin Murphy @ 2016-11-08 13:19 UTC (permalink / raw)
  To: Russell King - ARM Linux, Brian Starkey, Liviu Dudau, Jyri Sarha
  Cc: dri-devel, linux-arm-kernel

Hi Russell,

On 08/11/16 12:24, Russell King - ARM Linux wrote:
> As no one responded to the previous round, I'm not spending soo much
> time writing up a description of these changes again.  It's also been
> quite a long time, so I've forgotten all the details of the changes,
> so I'll do my best.
> 
> Changes from the previous series include:
> - reorder the initial three patches
> - change the (now third patch)... I think to increase the size of the
>   locked region.
> - fix edid parsing for infoframe generation - as was pointed out for
>   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
>   will not be called when an override-edid is in effect.  We need to
>   parse the override-edid.  Moreover, infoframe generation should not
>   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
>   HDMI to send infoframes.
> - only send audio if audio and infoframes are supported.
> 
> Otherwise, these are very much like the previous posting of the series,
> except rebased upon the mali/hdlcd/tda998x change to remove the
> drm_connector_register() call.
> 
> https://www.spinics.net/lists/dri-devel/msg121495.html
> 
> It'd be nice to have other tda998x users ack and test these patches,

I've just merged your drm-tda998x-devel branch and booted it on my Juno,
and I can at least confirm that DVI still seems to still work as before.
Anything in particular I should be looking out for? (Unfortunately the
only handy HDMI monitor is one of those slightly-out-of-spec ones which
has never really worked with the pixel clocks Juno provides)

Robin.

> I've tried to test on Juno, but the Juno situation seems to be a huge
> fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
> incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
> seems no way to get SCPI support on it - adding the BL0 executable
> start address in the SCC registers seems to be incompatible with the
> devchip, causing the PLLs to fail.  In discussion with Sudeep over
> these issues, but no idea where things are with it at the moment, other
> than Sudeep needs to investigate.  All Linaro firmware releases are
> broken on HBI0282B.)
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
>  1 file changed, 429 insertions(+), 397 deletions(-)
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-08 13:19   ` Robin Murphy
@ 2016-11-08 13:32     ` Russell King - ARM Linux
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 13:32 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 08, 2016 at 01:19:51PM +0000, Robin Murphy wrote:
> Hi Russell,
> 
> On 08/11/16 12:24, Russell King - ARM Linux wrote:
> > As no one responded to the previous round, I'm not spending soo much
> > time writing up a description of these changes again.  It's also been
> > quite a long time, so I've forgotten all the details of the changes,
> > so I'll do my best.
> > 
> > Changes from the previous series include:
> > - reorder the initial three patches
> > - change the (now third patch)... I think to increase the size of the
> >   locked region.
> > - fix edid parsing for infoframe generation - as was pointed out for
> >   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
> >   will not be called when an override-edid is in effect.  We need to
> >   parse the override-edid.  Moreover, infoframe generation should not
> >   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
> >   HDMI to send infoframes.
> > - only send audio if audio and infoframes are supported.
> > 
> > Otherwise, these are very much like the previous posting of the series,
> > except rebased upon the mali/hdlcd/tda998x change to remove the
> > drm_connector_register() call.
> > 
> > https://www.spinics.net/lists/dri-devel/msg121495.html
> > 
> > It'd be nice to have other tda998x users ack and test these patches,
> 
> I've just merged your drm-tda998x-devel branch and booted it on my Juno,
> and I can at least confirm that DVI still seems to still work as before.
> Anything in particular I should be looking out for? (Unfortunately the
> only handy HDMI monitor is one of those slightly-out-of-spec ones which
> has never really worked with the pixel clocks Juno provides)

Unfortunately, my drm-tda998x-devel branch is slightly out of date with
these patches it's the original set of 10 patches.  I've not pushed
these ones out to that branch yet, as I've three additional patches on
top of these which aren't "ready" for pushing out.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-08 13:32     ` Russell King - ARM Linux
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 13:32 UTC (permalink / raw)
  To: Robin Murphy
  Cc: linux-arm-kernel, dri-devel, Liviu Dudau, Brian Starkey, Jyri Sarha

On Tue, Nov 08, 2016 at 01:19:51PM +0000, Robin Murphy wrote:
> Hi Russell,
> 
> On 08/11/16 12:24, Russell King - ARM Linux wrote:
> > As no one responded to the previous round, I'm not spending soo much
> > time writing up a description of these changes again.  It's also been
> > quite a long time, so I've forgotten all the details of the changes,
> > so I'll do my best.
> > 
> > Changes from the previous series include:
> > - reorder the initial three patches
> > - change the (now third patch)... I think to increase the size of the
> >   locked region.
> > - fix edid parsing for infoframe generation - as was pointed out for
> >   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
> >   will not be called when an override-edid is in effect.  We need to
> >   parse the override-edid.  Moreover, infoframe generation should not
> >   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
> >   HDMI to send infoframes.
> > - only send audio if audio and infoframes are supported.
> > 
> > Otherwise, these are very much like the previous posting of the series,
> > except rebased upon the mali/hdlcd/tda998x change to remove the
> > drm_connector_register() call.
> > 
> > https://www.spinics.net/lists/dri-devel/msg121495.html
> > 
> > It'd be nice to have other tda998x users ack and test these patches,
> 
> I've just merged your drm-tda998x-devel branch and booted it on my Juno,
> and I can at least confirm that DVI still seems to still work as before.
> Anything in particular I should be looking out for? (Unfortunately the
> only handy HDMI monitor is one of those slightly-out-of-spec ones which
> has never really worked with the pixel clocks Juno provides)

Unfortunately, my drm-tda998x-devel branch is slightly out of date with
these patches it's the original set of 10 patches.  I've not pushed
these ones out to that branch yet, as I've three additional patches on
top of these which aren't "ready" for pushing out.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-08 13:32     ` Russell King - ARM Linux
@ 2016-11-08 13:34       ` Russell King - ARM Linux
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 13:34 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> these patches it's the original set of 10 patches.  I've not pushed
> these ones out to that branch yet, as I've three additional patches on
> top of these which aren't "ready" for pushing out.

Here's the delta between the branch and what I just posted:

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 1b262a89b7e1..3a5e5c466972 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -43,12 +43,12 @@ struct tda998x_priv {
 	u16 rev;
 	u8 current_page;
 	bool is_on;
-	bool is_hdmi_sink;
-	bool is_hdmi_config;
+	bool supports_infoframes;
+	bool sink_has_audio;
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
-	unsigned long tdms_clock;
+	unsigned long tmds_clock;
 	struct tda998x_audio_params audio_params;
 
 	struct platform_device *audio_pdev;
@@ -774,7 +774,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	 * assume 100MHz requires larger divider.
 	 */
 	adiv = AUDIO_DIV_SERCLK_8;
-	if (priv->tdms_clock > 100000)
+	if (priv->tmds_clock > 100000)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 */
 
 	/* S/PDIF asks for a larger divider */
@@ -869,8 +869,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 	}
 
 	mutex_lock(&priv->audio_mutex);
-	/* We must not program the TDA998x for audio if the sink is DVI. */
-	if (priv->is_hdmi_config)
+	if (priv->supports_infoframes && priv->sink_has_audio)
 		ret = tda998x_configure_audio(priv, &audio);
 	else
 		ret = 0;
@@ -962,6 +961,27 @@ static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
 		return drm_helper_connector_dpms(connector, mode);
 }
 
+static int tda998x_connector_fill_modes(struct drm_connector *connector,
+					uint32_t maxX, uint32_t maxY)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	int ret;
+
+	mutex_lock(&priv->audio_mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
+
+	if (connector->edid_blob_ptr) {
+		struct edid *edid = (void *)connector->edid_blob_ptr->data;
+
+		priv->sink_has_audio = drm_detect_monitor_audio(edid);
+	} else {
+		priv->sink_has_audio = false;
+	}
+	mutex_unlock(&priv->audio_mutex);
+
+	return ret;
+}
+
 static enum drm_connector_status
 tda998x_connector_detect(struct drm_connector *connector, bool force)
 {
@@ -980,7 +1000,7 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
 static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.dpms = tda998x_connector_dpms,
 	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = drm_helper_probe_single_connector_modes,
+	.fill_modes = tda998x_connector_fill_modes,
 	.detect = tda998x_connector_detect,
 	.destroy = tda998x_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
@@ -1072,11 +1092,7 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
 
 	drm_mode_connector_update_edid_property(connector, edid);
 	n = drm_add_edid_modes(connector, edid);
-	priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
-
-	mutex_lock(&priv->audio_mutex);
 	drm_edid_to_eld(connector, edid);
-	mutex_unlock(&priv->audio_mutex);
 
 	kfree(edid);
 
@@ -1350,8 +1366,22 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	/* must be last register set: */
 	reg_write(priv, REG_TBG_CNTRL_0, 0);
 
-	/* Only setup the info frames if the sink is HDMI */
-	if (priv->is_hdmi_sink) {
+	priv->tmds_clock = adjusted_mode->clock;
+
+	/* CEA-861B section 6 says that:
+	 * CEA version 1 (CEA-861) has no support for infoframes.
+	 * CEA version 2 (CEA-861A) supports version 1 AVI infoframes,
+	 * and optional basic audio.
+	 * CEA version 3 (CEA-861B) supports version 1 and 2 AVI infoframes,
+	 * and optional digital audio, with audio infoframes.
+	 *
+	 * Since we only support generation of version 2 AVI infoframes,
+	 * ignore CEA version 2 and below (iow, behave as if we're a
+	 * CEA-861 source.)
+	 */
+	priv->supports_infoframes = priv->connector.display_info.cea_rev >= 3;
+
+	if (priv->supports_infoframes) {
 		/* We need to turn HDMI HDCP stuff on to get audio through */
 		reg &= ~TBG_CNTRL_1_DWIN_DIS;
 		reg_write(priv, REG_TBG_CNTRL_1, reg);
@@ -1360,13 +1390,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		priv->tdms_clock = adjusted_mode->clock;
-
-		if (priv->audio_params.format != AFMT_UNUSED)
+		if (priv->audio_params.format != AFMT_UNUSED &&
+		    priv->sink_has_audio)
 			tda998x_configure_audio(priv, &priv->audio_params);
 	}
 
-	priv->is_hdmi_config = priv->is_hdmi_sink;
 	mutex_unlock(&priv->audio_mutex);
 }
 


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-08 13:34       ` Russell King - ARM Linux
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 13:34 UTC (permalink / raw)
  To: Robin Murphy
  Cc: linux-arm-kernel, dri-devel, Liviu Dudau, Brian Starkey, Jyri Sarha

On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> these patches it's the original set of 10 patches.  I've not pushed
> these ones out to that branch yet, as I've three additional patches on
> top of these which aren't "ready" for pushing out.

Here's the delta between the branch and what I just posted:

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 1b262a89b7e1..3a5e5c466972 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -43,12 +43,12 @@ struct tda998x_priv {
 	u16 rev;
 	u8 current_page;
 	bool is_on;
-	bool is_hdmi_sink;
-	bool is_hdmi_config;
+	bool supports_infoframes;
+	bool sink_has_audio;
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
-	unsigned long tdms_clock;
+	unsigned long tmds_clock;
 	struct tda998x_audio_params audio_params;
 
 	struct platform_device *audio_pdev;
@@ -774,7 +774,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	 * assume 100MHz requires larger divider.
 	 */
 	adiv = AUDIO_DIV_SERCLK_8;
-	if (priv->tdms_clock > 100000)
+	if (priv->tmds_clock > 100000)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 */
 
 	/* S/PDIF asks for a larger divider */
@@ -869,8 +869,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 	}
 
 	mutex_lock(&priv->audio_mutex);
-	/* We must not program the TDA998x for audio if the sink is DVI. */
-	if (priv->is_hdmi_config)
+	if (priv->supports_infoframes && priv->sink_has_audio)
 		ret = tda998x_configure_audio(priv, &audio);
 	else
 		ret = 0;
@@ -962,6 +961,27 @@ static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
 		return drm_helper_connector_dpms(connector, mode);
 }
 
+static int tda998x_connector_fill_modes(struct drm_connector *connector,
+					uint32_t maxX, uint32_t maxY)
+{
+	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
+	int ret;
+
+	mutex_lock(&priv->audio_mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
+
+	if (connector->edid_blob_ptr) {
+		struct edid *edid = (void *)connector->edid_blob_ptr->data;
+
+		priv->sink_has_audio = drm_detect_monitor_audio(edid);
+	} else {
+		priv->sink_has_audio = false;
+	}
+	mutex_unlock(&priv->audio_mutex);
+
+	return ret;
+}
+
 static enum drm_connector_status
 tda998x_connector_detect(struct drm_connector *connector, bool force)
 {
@@ -980,7 +1000,7 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
 static const struct drm_connector_funcs tda998x_connector_funcs = {
 	.dpms = tda998x_connector_dpms,
 	.reset = drm_atomic_helper_connector_reset,
-	.fill_modes = drm_helper_probe_single_connector_modes,
+	.fill_modes = tda998x_connector_fill_modes,
 	.detect = tda998x_connector_detect,
 	.destroy = tda998x_connector_destroy,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
@@ -1072,11 +1092,7 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
 
 	drm_mode_connector_update_edid_property(connector, edid);
 	n = drm_add_edid_modes(connector, edid);
-	priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
-
-	mutex_lock(&priv->audio_mutex);
 	drm_edid_to_eld(connector, edid);
-	mutex_unlock(&priv->audio_mutex);
 
 	kfree(edid);
 
@@ -1350,8 +1366,22 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	/* must be last register set: */
 	reg_write(priv, REG_TBG_CNTRL_0, 0);
 
-	/* Only setup the info frames if the sink is HDMI */
-	if (priv->is_hdmi_sink) {
+	priv->tmds_clock = adjusted_mode->clock;
+
+	/* CEA-861B section 6 says that:
+	 * CEA version 1 (CEA-861) has no support for infoframes.
+	 * CEA version 2 (CEA-861A) supports version 1 AVI infoframes,
+	 * and optional basic audio.
+	 * CEA version 3 (CEA-861B) supports version 1 and 2 AVI infoframes,
+	 * and optional digital audio, with audio infoframes.
+	 *
+	 * Since we only support generation of version 2 AVI infoframes,
+	 * ignore CEA version 2 and below (iow, behave as if we're a
+	 * CEA-861 source.)
+	 */
+	priv->supports_infoframes = priv->connector.display_info.cea_rev >= 3;
+
+	if (priv->supports_infoframes) {
 		/* We need to turn HDMI HDCP stuff on to get audio through */
 		reg &= ~TBG_CNTRL_1_DWIN_DIS;
 		reg_write(priv, REG_TBG_CNTRL_1, reg);
@@ -1360,13 +1390,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		priv->tdms_clock = adjusted_mode->clock;
-
-		if (priv->audio_params.format != AFMT_UNUSED)
+		if (priv->audio_params.format != AFMT_UNUSED &&
+		    priv->sink_has_audio)
 			tda998x_configure_audio(priv, &priv->audio_params);
 	}
 
-	priv->is_hdmi_config = priv->is_hdmi_sink;
 	mutex_unlock(&priv->audio_mutex);
 }
 


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-08 13:34       ` Russell King - ARM Linux
@ 2016-11-08 15:25         ` Robin Murphy
  -1 siblings, 0 replies; 52+ messages in thread
From: Robin Murphy @ 2016-11-08 15:25 UTC (permalink / raw)
  To: linux-arm-kernel

On 08/11/16 13:34, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
>> Unfortunately, my drm-tda998x-devel branch is slightly out of date with
>> these patches it's the original set of 10 patches.  I've not pushed
>> these ones out to that branch yet, as I've three additional patches on
>> top of these which aren't "ready" for pushing out.
> 
> Here's the delta between the branch and what I just posted:

Great, thanks. I committed that on top and my framebuffer console over
DVI is still working. I'm dubious that it's thorough enough to have
value (sadly I have neither the time nor the motivation to fight with
HDMI and I2S audio), but feel free to consider that a tested-by if you wish.

Robin.

> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 1b262a89b7e1..3a5e5c466972 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -43,12 +43,12 @@ struct tda998x_priv {
>  	u16 rev;
>  	u8 current_page;
>  	bool is_on;
> -	bool is_hdmi_sink;
> -	bool is_hdmi_config;
> +	bool supports_infoframes;
> +	bool sink_has_audio;
>  	u8 vip_cntrl_0;
>  	u8 vip_cntrl_1;
>  	u8 vip_cntrl_2;
> -	unsigned long tdms_clock;
> +	unsigned long tmds_clock;
>  	struct tda998x_audio_params audio_params;
>  
>  	struct platform_device *audio_pdev;
> @@ -774,7 +774,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
>  	 * assume 100MHz requires larger divider.
>  	 */
>  	adiv = AUDIO_DIV_SERCLK_8;
> -	if (priv->tdms_clock > 100000)
> +	if (priv->tmds_clock > 100000)
>  		adiv++;			/* AUDIO_DIV_SERCLK_16 */
>  
>  	/* S/PDIF asks for a larger divider */
> @@ -869,8 +869,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
>  	}
>  
>  	mutex_lock(&priv->audio_mutex);
> -	/* We must not program the TDA998x for audio if the sink is DVI. */
> -	if (priv->is_hdmi_config)
> +	if (priv->supports_infoframes && priv->sink_has_audio)
>  		ret = tda998x_configure_audio(priv, &audio);
>  	else
>  		ret = 0;
> @@ -962,6 +961,27 @@ static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
>  		return drm_helper_connector_dpms(connector, mode);
>  }
>  
> +static int tda998x_connector_fill_modes(struct drm_connector *connector,
> +					uint32_t maxX, uint32_t maxY)
> +{
> +	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
> +	int ret;
> +
> +	mutex_lock(&priv->audio_mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
> +
> +	if (connector->edid_blob_ptr) {
> +		struct edid *edid = (void *)connector->edid_blob_ptr->data;
> +
> +		priv->sink_has_audio = drm_detect_monitor_audio(edid);
> +	} else {
> +		priv->sink_has_audio = false;
> +	}
> +	mutex_unlock(&priv->audio_mutex);
> +
> +	return ret;
> +}
> +
>  static enum drm_connector_status
>  tda998x_connector_detect(struct drm_connector *connector, bool force)
>  {
> @@ -980,7 +1000,7 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
>  static const struct drm_connector_funcs tda998x_connector_funcs = {
>  	.dpms = tda998x_connector_dpms,
>  	.reset = drm_atomic_helper_connector_reset,
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.fill_modes = tda998x_connector_fill_modes,
>  	.detect = tda998x_connector_detect,
>  	.destroy = tda998x_connector_destroy,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> @@ -1072,11 +1092,7 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
>  
>  	drm_mode_connector_update_edid_property(connector, edid);
>  	n = drm_add_edid_modes(connector, edid);
> -	priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
> -
> -	mutex_lock(&priv->audio_mutex);
>  	drm_edid_to_eld(connector, edid);
> -	mutex_unlock(&priv->audio_mutex);
>  
>  	kfree(edid);
>  
> @@ -1350,8 +1366,22 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  	/* must be last register set: */
>  	reg_write(priv, REG_TBG_CNTRL_0, 0);
>  
> -	/* Only setup the info frames if the sink is HDMI */
> -	if (priv->is_hdmi_sink) {
> +	priv->tmds_clock = adjusted_mode->clock;
> +
> +	/* CEA-861B section 6 says that:
> +	 * CEA version 1 (CEA-861) has no support for infoframes.
> +	 * CEA version 2 (CEA-861A) supports version 1 AVI infoframes,
> +	 * and optional basic audio.
> +	 * CEA version 3 (CEA-861B) supports version 1 and 2 AVI infoframes,
> +	 * and optional digital audio, with audio infoframes.
> +	 *
> +	 * Since we only support generation of version 2 AVI infoframes,
> +	 * ignore CEA version 2 and below (iow, behave as if we're a
> +	 * CEA-861 source.)
> +	 */
> +	priv->supports_infoframes = priv->connector.display_info.cea_rev >= 3;
> +
> +	if (priv->supports_infoframes) {
>  		/* We need to turn HDMI HDCP stuff on to get audio through */
>  		reg &= ~TBG_CNTRL_1_DWIN_DIS;
>  		reg_write(priv, REG_TBG_CNTRL_1, reg);
> @@ -1360,13 +1390,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  
>  		tda998x_write_avi(priv, adjusted_mode);
>  
> -		priv->tdms_clock = adjusted_mode->clock;
> -
> -		if (priv->audio_params.format != AFMT_UNUSED)
> +		if (priv->audio_params.format != AFMT_UNUSED &&
> +		    priv->sink_has_audio)
>  			tda998x_configure_audio(priv, &priv->audio_params);
>  	}
>  
> -	priv->is_hdmi_config = priv->is_hdmi_sink;
>  	mutex_unlock(&priv->audio_mutex);
>  }
>  
> 
> 

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-08 15:25         ` Robin Murphy
  0 siblings, 0 replies; 52+ messages in thread
From: Robin Murphy @ 2016-11-08 15:25 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: linux-arm-kernel, dri-devel, Liviu Dudau, Jyri Sarha

On 08/11/16 13:34, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
>> Unfortunately, my drm-tda998x-devel branch is slightly out of date with
>> these patches it's the original set of 10 patches.  I've not pushed
>> these ones out to that branch yet, as I've three additional patches on
>> top of these which aren't "ready" for pushing out.
> 
> Here's the delta between the branch and what I just posted:

Great, thanks. I committed that on top and my framebuffer console over
DVI is still working. I'm dubious that it's thorough enough to have
value (sadly I have neither the time nor the motivation to fight with
HDMI and I2S audio), but feel free to consider that a tested-by if you wish.

Robin.

> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 1b262a89b7e1..3a5e5c466972 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -43,12 +43,12 @@ struct tda998x_priv {
>  	u16 rev;
>  	u8 current_page;
>  	bool is_on;
> -	bool is_hdmi_sink;
> -	bool is_hdmi_config;
> +	bool supports_infoframes;
> +	bool sink_has_audio;
>  	u8 vip_cntrl_0;
>  	u8 vip_cntrl_1;
>  	u8 vip_cntrl_2;
> -	unsigned long tdms_clock;
> +	unsigned long tmds_clock;
>  	struct tda998x_audio_params audio_params;
>  
>  	struct platform_device *audio_pdev;
> @@ -774,7 +774,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
>  	 * assume 100MHz requires larger divider.
>  	 */
>  	adiv = AUDIO_DIV_SERCLK_8;
> -	if (priv->tdms_clock > 100000)
> +	if (priv->tmds_clock > 100000)
>  		adiv++;			/* AUDIO_DIV_SERCLK_16 */
>  
>  	/* S/PDIF asks for a larger divider */
> @@ -869,8 +869,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
>  	}
>  
>  	mutex_lock(&priv->audio_mutex);
> -	/* We must not program the TDA998x for audio if the sink is DVI. */
> -	if (priv->is_hdmi_config)
> +	if (priv->supports_infoframes && priv->sink_has_audio)
>  		ret = tda998x_configure_audio(priv, &audio);
>  	else
>  		ret = 0;
> @@ -962,6 +961,27 @@ static int tda998x_connector_dpms(struct drm_connector *connector, int mode)
>  		return drm_helper_connector_dpms(connector, mode);
>  }
>  
> +static int tda998x_connector_fill_modes(struct drm_connector *connector,
> +					uint32_t maxX, uint32_t maxY)
> +{
> +	struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
> +	int ret;
> +
> +	mutex_lock(&priv->audio_mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
> +
> +	if (connector->edid_blob_ptr) {
> +		struct edid *edid = (void *)connector->edid_blob_ptr->data;
> +
> +		priv->sink_has_audio = drm_detect_monitor_audio(edid);
> +	} else {
> +		priv->sink_has_audio = false;
> +	}
> +	mutex_unlock(&priv->audio_mutex);
> +
> +	return ret;
> +}
> +
>  static enum drm_connector_status
>  tda998x_connector_detect(struct drm_connector *connector, bool force)
>  {
> @@ -980,7 +1000,7 @@ static void tda998x_connector_destroy(struct drm_connector *connector)
>  static const struct drm_connector_funcs tda998x_connector_funcs = {
>  	.dpms = tda998x_connector_dpms,
>  	.reset = drm_atomic_helper_connector_reset,
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.fill_modes = tda998x_connector_fill_modes,
>  	.detect = tda998x_connector_detect,
>  	.destroy = tda998x_connector_destroy,
>  	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> @@ -1072,11 +1092,7 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
>  
>  	drm_mode_connector_update_edid_property(connector, edid);
>  	n = drm_add_edid_modes(connector, edid);
> -	priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
> -
> -	mutex_lock(&priv->audio_mutex);
>  	drm_edid_to_eld(connector, edid);
> -	mutex_unlock(&priv->audio_mutex);
>  
>  	kfree(edid);
>  
> @@ -1350,8 +1366,22 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  	/* must be last register set: */
>  	reg_write(priv, REG_TBG_CNTRL_0, 0);
>  
> -	/* Only setup the info frames if the sink is HDMI */
> -	if (priv->is_hdmi_sink) {
> +	priv->tmds_clock = adjusted_mode->clock;
> +
> +	/* CEA-861B section 6 says that:
> +	 * CEA version 1 (CEA-861) has no support for infoframes.
> +	 * CEA version 2 (CEA-861A) supports version 1 AVI infoframes,
> +	 * and optional basic audio.
> +	 * CEA version 3 (CEA-861B) supports version 1 and 2 AVI infoframes,
> +	 * and optional digital audio, with audio infoframes.
> +	 *
> +	 * Since we only support generation of version 2 AVI infoframes,
> +	 * ignore CEA version 2 and below (iow, behave as if we're a
> +	 * CEA-861 source.)
> +	 */
> +	priv->supports_infoframes = priv->connector.display_info.cea_rev >= 3;
> +
> +	if (priv->supports_infoframes) {
>  		/* We need to turn HDMI HDCP stuff on to get audio through */
>  		reg &= ~TBG_CNTRL_1_DWIN_DIS;
>  		reg_write(priv, REG_TBG_CNTRL_1, reg);
> @@ -1360,13 +1390,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  
>  		tda998x_write_avi(priv, adjusted_mode);
>  
> -		priv->tdms_clock = adjusted_mode->clock;
> -
> -		if (priv->audio_params.format != AFMT_UNUSED)
> +		if (priv->audio_params.format != AFMT_UNUSED &&
> +		    priv->sink_has_audio)
>  			tda998x_configure_audio(priv, &priv->audio_params);
>  	}
>  
> -	priv->is_hdmi_config = priv->is_hdmi_sink;
>  	mutex_unlock(&priv->audio_mutex);
>  }
>  
> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-08 15:25         ` Robin Murphy
@ 2016-11-08 15:42           ` Russell King - ARM Linux
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 15:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 08, 2016 at 03:25:45PM +0000, Robin Murphy wrote:
> On 08/11/16 13:34, Russell King - ARM Linux wrote:
> > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> >> Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> >> these patches it's the original set of 10 patches.  I've not pushed
> >> these ones out to that branch yet, as I've three additional patches on
> >> top of these which aren't "ready" for pushing out.
> > 
> > Here's the delta between the branch and what I just posted:
> 
> Great, thanks. I committed that on top and my framebuffer console over
> DVI is still working. I'm dubious that it's thorough enough to have
> value (sadly I have neither the time nor the motivation to fight with
> HDMI and I2S audio), but feel free to consider that a tested-by if you
> wish.

Well, "nothing obviously broken" is good enough, thanks.

I don't have (or use) I2S video on my TDA998x, and that's something
which is in Jyri's interest area.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-08 15:42           ` Russell King - ARM Linux
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 15:42 UTC (permalink / raw)
  To: Robin Murphy
  Cc: linux-arm-kernel, dri-devel, Liviu Dudau, Brian Starkey, Jyri Sarha

On Tue, Nov 08, 2016 at 03:25:45PM +0000, Robin Murphy wrote:
> On 08/11/16 13:34, Russell King - ARM Linux wrote:
> > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> >> Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> >> these patches it's the original set of 10 patches.  I've not pushed
> >> these ones out to that branch yet, as I've three additional patches on
> >> top of these which aren't "ready" for pushing out.
> > 
> > Here's the delta between the branch and what I just posted:
> 
> Great, thanks. I committed that on top and my framebuffer console over
> DVI is still working. I'm dubious that it's thorough enough to have
> value (sadly I have neither the time nor the motivation to fight with
> HDMI and I2S audio), but feel free to consider that a tested-by if you
> wish.

Well, "nothing obviously broken" is good enough, thanks.

I don't have (or use) I2S video on my TDA998x, and that's something
which is in Jyri's interest area.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-08 13:34       ` Russell King - ARM Linux
@ 2016-11-08 17:20         ` Jon Medhurst (Tixy)
  -1 siblings, 0 replies; 52+ messages in thread
From: Jon Medhurst (Tixy) @ 2016-11-08 17:20 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> > Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> > these patches it's the original set of 10 patches.  I've not pushed
> > these ones out to that branch yet, as I've three additional patches on
> > top of these which aren't "ready" for pushing out.
> 
> Here's the delta between the branch and what I just posted:
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
[...]

I have a working setup for HDMI audio on Juno an would like to test this
series but am struggling to work out which patches to apply in what
order to what branch, can you be specific? (I've tried various
combinations of patches series from the list, drm-tda998x-devel, and the
diff you posted)

Thanks

-- 
Tixy

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-08 17:20         ` Jon Medhurst (Tixy)
  0 siblings, 0 replies; 52+ messages in thread
From: Jon Medhurst (Tixy) @ 2016-11-08 17:20 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Liviu Dudau, Jyri Sarha, dri-devel, Robin Murphy, Brian Starkey,
	linux-arm-kernel

On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> > Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> > these patches it's the original set of 10 patches.  I've not pushed
> > these ones out to that branch yet, as I've three additional patches on
> > top of these which aren't "ready" for pushing out.
> 
> Here's the delta between the branch and what I just posted:
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
[...]

I have a working setup for HDMI audio on Juno an would like to test this
series but am struggling to work out which patches to apply in what
order to what branch, can you be specific? (I've tried various
combinations of patches series from the list, drm-tda998x-devel, and the
diff you posted)

Thanks

-- 
Tixy

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-08 17:20         ` Jon Medhurst (Tixy)
@ 2016-11-08 18:24           ` Russell King - ARM Linux
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 18:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote:
> On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> > > these patches it's the original set of 10 patches.  I've not pushed
> > > these ones out to that branch yet, as I've three additional patches on
> > > top of these which aren't "ready" for pushing out.
> > 
> > Here's the delta between the branch and what I just posted:
> > 
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> [...]
> 
> I have a working setup for HDMI audio on Juno an would like to test this
> series but am struggling to work out which patches to apply in what
> order to what branch, can you be specific? (I've tried various
> combinations of patches series from the list, drm-tda998x-devel, and the
> diff you posted)

Hmm, I guess this is going to be annoyingly rather difficult then.
The structure of my git tree is:

v4.8 ---------------- mali patch ------------------ merge --- these patches
v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^

which makes it rather difficult to send out a series that people can
apply as patches without first replicating that merge.  I guess the
answer is... use the _patches_ for review, and I'll push out the
changes into drm-tda998x-devel... should be there soon.  Look for
commit hash d61fa2e50f2a.  (Bah, slow 'net connections.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-08 18:24           ` Russell King - ARM Linux
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-08 18:24 UTC (permalink / raw)
  To: Jon Medhurst (Tixy)
  Cc: Liviu Dudau, Jyri Sarha, dri-devel, Robin Murphy, Brian Starkey,
	linux-arm-kernel

On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote:
> On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> > > these patches it's the original set of 10 patches.  I've not pushed
> > > these ones out to that branch yet, as I've three additional patches on
> > > top of these which aren't "ready" for pushing out.
> > 
> > Here's the delta between the branch and what I just posted:
> > 
> > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> [...]
> 
> I have a working setup for HDMI audio on Juno an would like to test this
> series but am struggling to work out which patches to apply in what
> order to what branch, can you be specific? (I've tried various
> combinations of patches series from the list, drm-tda998x-devel, and the
> diff you posted)

Hmm, I guess this is going to be annoyingly rather difficult then.
The structure of my git tree is:

v4.8 ---------------- mali patch ------------------ merge --- these patches
v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^

which makes it rather difficult to send out a series that people can
apply as patches without first replicating that merge.  I guess the
answer is... use the _patches_ for review, and I'll push out the
changes into drm-tda998x-devel... should be there soon.  Look for
commit hash d61fa2e50f2a.  (Bah, slow 'net connections.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-08 18:24           ` Russell King - ARM Linux
@ 2016-11-09 11:45             ` Jon Medhurst (Tixy)
  -1 siblings, 0 replies; 52+ messages in thread
From: Jon Medhurst (Tixy) @ 2016-11-09 11:45 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, 2016-11-08 at 18:24 +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote:
> > On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote:
> > > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> > > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> > > > these patches it's the original set of 10 patches.  I've not pushed
> > > > these ones out to that branch yet, as I've three additional patches on
> > > > top of these which aren't "ready" for pushing out.
> > > 
> > > Here's the delta between the branch and what I just posted:
> > > 
> > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> > [...]
> > 
> > I have a working setup for HDMI audio on Juno an would like to test this
> > series but am struggling to work out which patches to apply in what
> > order to what branch, can you be specific? (I've tried various
> > combinations of patches series from the list, drm-tda998x-devel, and the
> > diff you posted)
> 
> Hmm, I guess this is going to be annoyingly rather difficult then.
> The structure of my git tree is:
> 
> v4.8 ---------------- mali patch ------------------ merge --- these patches
> v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^
> 
> which makes it rather difficult to send out a series that people can
> apply as patches without first replicating that merge.  I guess the
> answer is... use the _patches_ for review, and I'll push out the
> changes into drm-tda998x-devel... should be there soon.  Look for
> commit hash d61fa2e50f2a.  (Bah, slow 'net connections.)

Testing gets more complicated as I'm using 4.9-rc? which has a DMA fix
needed for audio [1] and breaks hdmi-codec which I hope I fixed [2].

Anyway, I merged in drm-tda998x-devel and audio continued to work on my
HDMI connected monitor. So I guess that's

Tested-by: Jon Medhurst <tixy@linaro.org>

I also reviewed the patches in this series. They look like a mostly
mechanical code organisation improvement, and whilst I'm not very
familiar with the driver and DRM, the other actual code changes look OK
too. So, FWIW:

Acked-by: Jon Medhurst <tixy@linaro.org>

[1] Commit d64e9a2c7509 ("dmaengine: pl330: fix residual for non-running BUSY descriptors")
[2] https://patchwork.kernel.org/patch/9401485/

-- 
Tixy

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-09 11:45             ` Jon Medhurst (Tixy)
  0 siblings, 0 replies; 52+ messages in thread
From: Jon Medhurst (Tixy) @ 2016-11-09 11:45 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Liviu Dudau, dri-devel, Jyri Sarha, Robin Murphy, Brian Starkey,
	linux-arm-kernel

On Tue, 2016-11-08 at 18:24 +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote:
> > On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote:
> > > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> > > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> > > > these patches it's the original set of 10 patches.  I've not pushed
> > > > these ones out to that branch yet, as I've three additional patches on
> > > > top of these which aren't "ready" for pushing out.
> > > 
> > > Here's the delta between the branch and what I just posted:
> > > 
> > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> > [...]
> > 
> > I have a working setup for HDMI audio on Juno an would like to test this
> > series but am struggling to work out which patches to apply in what
> > order to what branch, can you be specific? (I've tried various
> > combinations of patches series from the list, drm-tda998x-devel, and the
> > diff you posted)
> 
> Hmm, I guess this is going to be annoyingly rather difficult then.
> The structure of my git tree is:
> 
> v4.8 ---------------- mali patch ------------------ merge --- these patches
> v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^
> 
> which makes it rather difficult to send out a series that people can
> apply as patches without first replicating that merge.  I guess the
> answer is... use the _patches_ for review, and I'll push out the
> changes into drm-tda998x-devel... should be there soon.  Look for
> commit hash d61fa2e50f2a.  (Bah, slow 'net connections.)

Testing gets more complicated as I'm using 4.9-rc? which has a DMA fix
needed for audio [1] and breaks hdmi-codec which I hope I fixed [2].

Anyway, I merged in drm-tda998x-devel and audio continued to work on my
HDMI connected monitor. So I guess that's

Tested-by: Jon Medhurst <tixy@linaro.org>

I also reviewed the patches in this series. They look like a mostly
mechanical code organisation improvement, and whilst I'm not very
familiar with the driver and DRM, the other actual code changes look OK
too. So, FWIW:

Acked-by: Jon Medhurst <tixy@linaro.org>

[1] Commit d64e9a2c7509 ("dmaengine: pl330: fix residual for non-running BUSY descriptors")
[2] https://patchwork.kernel.org/patch/9401485/

-- 
Tixy

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-09 11:45             ` Jon Medhurst (Tixy)
@ 2016-11-09 22:14               ` Russell King - ARM Linux
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-09 22:14 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Nov 09, 2016 at 11:45:05AM +0000, Jon Medhurst (Tixy) wrote:
> On Tue, 2016-11-08 at 18:24 +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote:
> > > On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote:
> > > > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> > > > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> > > > > these patches it's the original set of 10 patches.  I've not pushed
> > > > > these ones out to that branch yet, as I've three additional patches on
> > > > > top of these which aren't "ready" for pushing out.
> > > > 
> > > > Here's the delta between the branch and what I just posted:
> > > > 
> > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > [...]
> > > 
> > > I have a working setup for HDMI audio on Juno an would like to test this
> > > series but am struggling to work out which patches to apply in what
> > > order to what branch, can you be specific? (I've tried various
> > > combinations of patches series from the list, drm-tda998x-devel, and the
> > > diff you posted)
> > 
> > Hmm, I guess this is going to be annoyingly rather difficult then.
> > The structure of my git tree is:
> > 
> > v4.8 ---------------- mali patch ------------------ merge --- these patches
> > v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^
> > 
> > which makes it rather difficult to send out a series that people can
> > apply as patches without first replicating that merge.  I guess the
> > answer is... use the _patches_ for review, and I'll push out the
> > changes into drm-tda998x-devel... should be there soon.  Look for
> > commit hash d61fa2e50f2a.  (Bah, slow 'net connections.)
> 
> Testing gets more complicated as I'm using 4.9-rc? which has a DMA fix
> needed for audio [1] and breaks hdmi-codec which I hope I fixed [2].

Well, ultimately it's going to be merged during the 4.10 merge window,
so it's going to be combined with lots of other changes along the way.
I'm testing it against 4.8 on the Dove Cubox, and build testing it
against 4.9-rc, so it's good someone is hardware testing with 4.9-rc
merged.

> Anyway, I merged in drm-tda998x-devel and audio continued to work on my
> HDMI connected monitor. So I guess that's
> 
> Tested-by: Jon Medhurst <tixy@linaro.org>

Thanks.

> I also reviewed the patches in this series. They look like a mostly
> mechanical code organisation improvement, and whilst I'm not very
> familiar with the driver and DRM, the other actual code changes look OK
> too. So, FWIW:

Yes, much of it is a code reorganisation, its mainly preparation for
converting the driver to be DRM bridge based, but in reviewing the
initial DRM bridge conversion patch, I spotted some weaknesses in the
existing code which the first few patches of this series address.  I
don't think they're serious enough to warrant submission into -rc
though.

The next three patches I'll post in the coming days (once I've finished
evaluating one of them) should reduce power consumption of the device
by shutting down some blocks we never use, and some preparation to
support CEC (I've had CEC working with Hans Verkil's patches for some
time now, I'm mostly waiting for the CEC core to move out of staging,
at which point it can end up in a saner place other than
drivers/gpu/drm/i2c.)

> Acked-by: Jon Medhurst <tixy@linaro.org>

Thanks, and thanks for testing.  

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-09 22:14               ` Russell King - ARM Linux
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-09 22:14 UTC (permalink / raw)
  To: Jon Medhurst (Tixy)
  Cc: Liviu Dudau, dri-devel, Jyri Sarha, Robin Murphy, Brian Starkey,
	linux-arm-kernel

On Wed, Nov 09, 2016 at 11:45:05AM +0000, Jon Medhurst (Tixy) wrote:
> On Tue, 2016-11-08 at 18:24 +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 08, 2016 at 05:20:36PM +0000, Jon Medhurst (Tixy) wrote:
> > > On Tue, 2016-11-08 at 13:34 +0000, Russell King - ARM Linux wrote:
> > > > On Tue, Nov 08, 2016 at 01:32:15PM +0000, Russell King - ARM Linux wrote:
> > > > > Unfortunately, my drm-tda998x-devel branch is slightly out of date with
> > > > > these patches it's the original set of 10 patches.  I've not pushed
> > > > > these ones out to that branch yet, as I've three additional patches on
> > > > > top of these which aren't "ready" for pushing out.
> > > > 
> > > > Here's the delta between the branch and what I just posted:
> > > > 
> > > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> > > [...]
> > > 
> > > I have a working setup for HDMI audio on Juno an would like to test this
> > > series but am struggling to work out which patches to apply in what
> > > order to what branch, can you be specific? (I've tried various
> > > combinations of patches series from the list, drm-tda998x-devel, and the
> > > diff you posted)
> > 
> > Hmm, I guess this is going to be annoyingly rather difficult then.
> > The structure of my git tree is:
> > 
> > v4.8 ---------------- mali patch ------------------ merge --- these patches
> > v4.7 -- tda998x audio patches (up to df0bd1e8f3c5) --^
> > 
> > which makes it rather difficult to send out a series that people can
> > apply as patches without first replicating that merge.  I guess the
> > answer is... use the _patches_ for review, and I'll push out the
> > changes into drm-tda998x-devel... should be there soon.  Look for
> > commit hash d61fa2e50f2a.  (Bah, slow 'net connections.)
> 
> Testing gets more complicated as I'm using 4.9-rc? which has a DMA fix
> needed for audio [1] and breaks hdmi-codec which I hope I fixed [2].

Well, ultimately it's going to be merged during the 4.10 merge window,
so it's going to be combined with lots of other changes along the way.
I'm testing it against 4.8 on the Dove Cubox, and build testing it
against 4.9-rc, so it's good someone is hardware testing with 4.9-rc
merged.

> Anyway, I merged in drm-tda998x-devel and audio continued to work on my
> HDMI connected monitor. So I guess that's
> 
> Tested-by: Jon Medhurst <tixy@linaro.org>

Thanks.

> I also reviewed the patches in this series. They look like a mostly
> mechanical code organisation improvement, and whilst I'm not very
> familiar with the driver and DRM, the other actual code changes look OK
> too. So, FWIW:

Yes, much of it is a code reorganisation, its mainly preparation for
converting the driver to be DRM bridge based, but in reviewing the
initial DRM bridge conversion patch, I spotted some weaknesses in the
existing code which the first few patches of this series address.  I
don't think they're serious enough to warrant submission into -rc
though.

The next three patches I'll post in the coming days (once I've finished
evaluating one of them) should reduce power consumption of the device
by shutting down some blocks we never use, and some preparation to
support CEC (I've had CEC working with Hans Verkil's patches for some
time now, I'm mostly waiting for the CEC core to move out of staging,
at which point it can end up in a saner place other than
drivers/gpu/drm/i2c.)

> Acked-by: Jon Medhurst <tixy@linaro.org>

Thanks, and thanks for testing.  

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-08 12:24 ` Russell King - ARM Linux
@ 2016-11-11 15:10   ` Jyri Sarha
  -1 siblings, 0 replies; 52+ messages in thread
From: Jyri Sarha @ 2016-11-11 15:10 UTC (permalink / raw)
  To: linux-arm-kernel

On 11/08/16 14:24, Russell King - ARM Linux wrote:
> As no one responded to the previous round, I'm not spending soo much
> time writing up a description of these changes again.  It's also been
> quite a long time, so I've forgotten all the details of the changes,
> so I'll do my best.
> 
> Changes from the previous series include:
> - reorder the initial three patches
> - change the (now third patch)... I think to increase the size of the
>   locked region.
> - fix edid parsing for infoframe generation - as was pointed out for
>   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
>   will not be called when an override-edid is in effect.  We need to
>   parse the override-edid.  Moreover, infoframe generation should not
>   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
>   HDMI to send infoframes.
> - only send audio if audio and infoframes are supported.
> 
> Otherwise, these are very much like the previous posting of the series,
> except rebased upon the mali/hdlcd/tda998x change to remove the
> drm_connector_register() call.
> 
> https://www.spinics.net/lists/dri-devel/msg121495.html
> 
> It'd be nice to have other tda998x users ack and test these patches,
> I've tried to test on Juno, but the Juno situation seems to be a huge
> fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
> incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
> seems no way to get SCPI support on it - adding the BL0 executable
> start address in the SCC registers seems to be incompatible with the
> devchip, causing the PLLs to fail.  In discussion with Sudeep over
> these issues, but no idea where things are with it at the moment, other
> than Sudeep needs to investigate.  All Linaro firmware releases are
> broken on HBI0282B.)
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
>  1 file changed, 429 insertions(+), 397 deletions(-)
> 

Reviewed-by: Jyri Sarha <jsarha@ti.com>

For the whole series. I am also happy to test these patches if I can
fetch them from some git repo.

Best regards,
Jyri

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-11 15:10   ` Jyri Sarha
  0 siblings, 0 replies; 52+ messages in thread
From: Jyri Sarha @ 2016-11-11 15:10 UTC (permalink / raw)
  To: Russell King - ARM Linux, Brian Starkey, Liviu Dudau
  Cc: linux-arm-kernel, dri-devel

On 11/08/16 14:24, Russell King - ARM Linux wrote:
> As no one responded to the previous round, I'm not spending soo much
> time writing up a description of these changes again.  It's also been
> quite a long time, so I've forgotten all the details of the changes,
> so I'll do my best.
> 
> Changes from the previous series include:
> - reorder the initial three patches
> - change the (now third patch)... I think to increase the size of the
>   locked region.
> - fix edid parsing for infoframe generation - as was pointed out for
>   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
>   will not be called when an override-edid is in effect.  We need to
>   parse the override-edid.  Moreover, infoframe generation should not
>   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
>   HDMI to send infoframes.
> - only send audio if audio and infoframes are supported.
> 
> Otherwise, these are very much like the previous posting of the series,
> except rebased upon the mali/hdlcd/tda998x change to remove the
> drm_connector_register() call.
> 
> https://www.spinics.net/lists/dri-devel/msg121495.html
> 
> It'd be nice to have other tda998x users ack and test these patches,
> I've tried to test on Juno, but the Juno situation seems to be a huge
> fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
> incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
> seems no way to get SCPI support on it - adding the BL0 executable
> start address in the SCC registers seems to be incompatible with the
> devchip, causing the PLLs to fail.  In discussion with Sudeep over
> these issues, but no idea where things are with it at the moment, other
> than Sudeep needs to investigate.  All Linaro firmware releases are
> broken on HBI0282B.)
> 
>  drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
>  1 file changed, 429 insertions(+), 397 deletions(-)
> 

Reviewed-by: Jyri Sarha <jsarha@ti.com>

For the whole series. I am also happy to test these patches if I can
fetch them from some git repo.

Best regards,
Jyri
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-11 15:10   ` Jyri Sarha
@ 2016-11-11 15:27     ` Russell King - ARM Linux
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-11 15:27 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Nov 11, 2016 at 05:10:09PM +0200, Jyri Sarha wrote:
> On 11/08/16 14:24, Russell King - ARM Linux wrote:
> > As no one responded to the previous round, I'm not spending soo much
> > time writing up a description of these changes again.  It's also been
> > quite a long time, so I've forgotten all the details of the changes,
> > so I'll do my best.
> > 
> > Changes from the previous series include:
> > - reorder the initial three patches
> > - change the (now third patch)... I think to increase the size of the
> >   locked region.
> > - fix edid parsing for infoframe generation - as was pointed out for
> >   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
> >   will not be called when an override-edid is in effect.  We need to
> >   parse the override-edid.  Moreover, infoframe generation should not
> >   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
> >   HDMI to send infoframes.
> > - only send audio if audio and infoframes are supported.
> > 
> > Otherwise, these are very much like the previous posting of the series,
> > except rebased upon the mali/hdlcd/tda998x change to remove the
> > drm_connector_register() call.
> > 
> > https://www.spinics.net/lists/dri-devel/msg121495.html
> > 
> > It'd be nice to have other tda998x users ack and test these patches,
> > I've tried to test on Juno, but the Juno situation seems to be a huge
> > fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
> > incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
> > seems no way to get SCPI support on it - adding the BL0 executable
> > start address in the SCC registers seems to be incompatible with the
> > devchip, causing the PLLs to fail.  In discussion with Sudeep over
> > these issues, but no idea where things are with it at the moment, other
> > than Sudeep needs to investigate.  All Linaro firmware releases are
> > broken on HBI0282B.)
> > 
> >  drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
> >  1 file changed, 429 insertions(+), 397 deletions(-)
> > 
> 
> Reviewed-by: Jyri Sarha <jsarha@ti.com>
> 
> For the whole series. I am also happy to test these patches if I can
> fetch them from some git repo.

git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-devel

The commit IDs are unstable, because I'll have to recommit them to add
your r-by and any other tags you later give me. :)

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-11 15:27     ` Russell King - ARM Linux
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-11 15:27 UTC (permalink / raw)
  To: Jyri Sarha; +Cc: linux-arm-kernel, Liviu Dudau, Brian Starkey, dri-devel

On Fri, Nov 11, 2016 at 05:10:09PM +0200, Jyri Sarha wrote:
> On 11/08/16 14:24, Russell King - ARM Linux wrote:
> > As no one responded to the previous round, I'm not spending soo much
> > time writing up a description of these changes again.  It's also been
> > quite a long time, so I've forgotten all the details of the changes,
> > so I'll do my best.
> > 
> > Changes from the previous series include:
> > - reorder the initial three patches
> > - change the (now third patch)... I think to increase the size of the
> >   locked region.
> > - fix edid parsing for infoframe generation - as was pointed out for
> >   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
> >   will not be called when an override-edid is in effect.  We need to
> >   parse the override-edid.  Moreover, infoframe generation should not
> >   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
> >   HDMI to send infoframes.
> > - only send audio if audio and infoframes are supported.
> > 
> > Otherwise, these are very much like the previous posting of the series,
> > except rebased upon the mali/hdlcd/tda998x change to remove the
> > drm_connector_register() call.
> > 
> > https://www.spinics.net/lists/dri-devel/msg121495.html
> > 
> > It'd be nice to have other tda998x users ack and test these patches,
> > I've tried to test on Juno, but the Juno situation seems to be a huge
> > fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
> > incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
> > seems no way to get SCPI support on it - adding the BL0 executable
> > start address in the SCC registers seems to be incompatible with the
> > devchip, causing the PLLs to fail.  In discussion with Sudeep over
> > these issues, but no idea where things are with it at the moment, other
> > than Sudeep needs to investigate.  All Linaro firmware releases are
> > broken on HBI0282B.)
> > 
> >  drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
> >  1 file changed, 429 insertions(+), 397 deletions(-)
> > 
> 
> Reviewed-by: Jyri Sarha <jsarha@ti.com>
> 
> For the whole series. I am also happy to test these patches if I can
> fetch them from some git repo.

git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-devel

The commit IDs are unstable, because I'll have to recommit them to add
your r-by and any other tags you later give me. :)

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 00/12] tda998x updates
  2016-11-11 15:27     ` Russell King - ARM Linux
@ 2016-11-11 16:24       ` Jyri Sarha
  -1 siblings, 0 replies; 52+ messages in thread
From: Jyri Sarha @ 2016-11-11 16:24 UTC (permalink / raw)
  To: linux-arm-kernel

On 11/11/16 17:27, Russell King - ARM Linux wrote:
> On Fri, Nov 11, 2016 at 05:10:09PM +0200, Jyri Sarha wrote:
>> On 11/08/16 14:24, Russell King - ARM Linux wrote:
>>> As no one responded to the previous round, I'm not spending soo much
>>> time writing up a description of these changes again.  It's also been
>>> quite a long time, so I've forgotten all the details of the changes,
>>> so I'll do my best.
>>>
>>> Changes from the previous series include:
>>> - reorder the initial three patches
>>> - change the (now third patch)... I think to increase the size of the
>>>   locked region.
>>> - fix edid parsing for infoframe generation - as was pointed out for
>>>   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
>>>   will not be called when an override-edid is in effect.  We need to
>>>   parse the override-edid.  Moreover, infoframe generation should not
>>>   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
>>>   HDMI to send infoframes.
>>> - only send audio if audio and infoframes are supported.
>>>
>>> Otherwise, these are very much like the previous posting of the series,
>>> except rebased upon the mali/hdlcd/tda998x change to remove the
>>> drm_connector_register() call.
>>>
>>> https://www.spinics.net/lists/dri-devel/msg121495.html
>>>
>>> It'd be nice to have other tda998x users ack and test these patches,
>>> I've tried to test on Juno, but the Juno situation seems to be a huge
>>> fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
>>> incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
>>> seems no way to get SCPI support on it - adding the BL0 executable
>>> start address in the SCC registers seems to be incompatible with the
>>> devchip, causing the PLLs to fail.  In discussion with Sudeep over
>>> these issues, but no idea where things are with it at the moment, other
>>> than Sudeep needs to investigate.  All Linaro firmware releases are
>>> broken on HBI0282B.)
>>>
>>>  drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
>>>  1 file changed, 429 insertions(+), 397 deletions(-)
>>>
>>
>> Reviewed-by: Jyri Sarha <jsarha@ti.com>
>>
>> For the whole series. I am also happy to test these patches if I can
>> fetch them from some git repo.
> 
> git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-devel
> 
> The commit IDs are unstable, because I'll have to recommit them to add
> your r-by and any other tags you later give me. :)
> 

I tested the branch with my Beaglebone-black and couple of TVs. I played
audio on different sample-rates while plugging and unplugging the cable
to the TVs and changing video modes. Everything worked as it should. I
let you decide whether or not this adhoc testing qualifies:

Tested-by: Jyri Sarha <jsarha@ti.com>

Best regards,
Jyri

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 00/12] tda998x updates
@ 2016-11-11 16:24       ` Jyri Sarha
  0 siblings, 0 replies; 52+ messages in thread
From: Jyri Sarha @ 2016-11-11 16:24 UTC (permalink / raw)
  To: Russell King - ARM Linux; +Cc: linux-arm-kernel, Liviu Dudau, dri-devel

On 11/11/16 17:27, Russell King - ARM Linux wrote:
> On Fri, Nov 11, 2016 at 05:10:09PM +0200, Jyri Sarha wrote:
>> On 11/08/16 14:24, Russell King - ARM Linux wrote:
>>> As no one responded to the previous round, I'm not spending soo much
>>> time writing up a description of these changes again.  It's also been
>>> quite a long time, so I've forgotten all the details of the changes,
>>> so I'll do my best.
>>>
>>> Changes from the previous series include:
>>> - reorder the initial three patches
>>> - change the (now third patch)... I think to increase the size of the
>>>   locked region.
>>> - fix edid parsing for infoframe generation - as was pointed out for
>>>   dw-hdmi, parsing the EDID in get_modes() is incorrect, as that method
>>>   will not be called when an override-edid is in effect.  We need to
>>>   parse the override-edid.  Moreover, infoframe generation should not
>>>   be keyed to whether the monitor is HDMI or not, CEA-861B allows non-
>>>   HDMI to send infoframes.
>>> - only send audio if audio and infoframes are supported.
>>>
>>> Otherwise, these are very much like the previous posting of the series,
>>> except rebased upon the mali/hdlcd/tda998x change to remove the
>>> drm_connector_register() call.
>>>
>>> https://www.spinics.net/lists/dri-devel/msg121495.html
>>>
>>> It'd be nice to have other tda998x users ack and test these patches,
>>> I've tried to test on Juno, but the Juno situation seems to be a huge
>>> fail.  (HBI0282B completely fails with latest firmware - (a) FPGA image
>>> incompatibilities io_b118 causes all FPGA AMBA devices to vanish, (b)
>>> seems no way to get SCPI support on it - adding the BL0 executable
>>> start address in the SCC registers seems to be incompatible with the
>>> devchip, causing the PLLs to fail.  In discussion with Sudeep over
>>> these issues, but no idea where things are with it at the moment, other
>>> than Sudeep needs to investigate.  All Linaro firmware releases are
>>> broken on HBI0282B.)
>>>
>>>  drivers/gpu/drm/i2c/tda998x_drv.c | 826 ++++++++++++++++++++------------------
>>>  1 file changed, 429 insertions(+), 397 deletions(-)
>>>
>>
>> Reviewed-by: Jyri Sarha <jsarha@ti.com>
>>
>> For the whole series. I am also happy to test these patches if I can
>> fetch them from some git repo.
> 
> git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-devel
> 
> The commit IDs are unstable, because I'll have to recommit them to add
> your r-by and any other tags you later give me. :)
> 

I tested the branch with my Beaglebone-black and couple of TVs. I played
audio on different sample-rates while plugging and unplugging the cable
to the TVs and changing video modes. Everything worked as it should. I
let you decide whether or not this adhoc testing qualifies:

Tested-by: Jyri Sarha <jsarha@ti.com>

Best regards,
Jyri

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH RFC 01/12] drm/i2c: tda998x: avoid race in tda998x_encoder_mode_set()
  2016-11-08 12:25   ` Russell King
@ 2016-11-18  0:01     ` Russell King - ARM Linux
  -1 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-18  0:01 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 08, 2016 at 12:25:00PM +0000, Russell King wrote:
> As priv->audio_params can now be changed at run time, we need to be more
> careful about how we deal with a mode set.  We must take the audio lock
> while checking if there's a valid audio configuration.
> 
> However, it's slightly worse than that - during mode set, we mute the
> audio, and it must not be unmuted until we have finished the mode set.
> It is possible that the audio side may start while a mode set is in
> progress, so take the audio_mutex lock around the whole mode setting
> procedure.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 088900d78ceb..1cc0433ce9d5 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1074,13 +1074,12 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  
>  		tda998x_write_avi(priv, adjusted_mode);
>  
> -		if (priv->audio_params.format != AFMT_UNUSED) {
> -			mutex_lock(&priv->audio_mutex);
> +		mutex_lock(&priv->audio_mutex);
> +		if (priv->audio_params.format != AFMT_UNUSED)
>  			tda998x_configure_audio(priv,
>  						&priv->audio_params,
>  						adjusted_mode->clock);
> -			mutex_unlock(&priv->audio_mutex);
> -		}
> +		mutex_unlock(&priv->audio_mutex);
>  	}
>  }
>  

I've just realised that this (and the following few changes) require this
mutex to be always initialised.  Currently, we only initialise the audio
mutex when in DT mode, which is bad.  So, I'm going to add this patch in
front of this patch set:

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 088900d78ceb..168c0b4b8f47 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1405,6 +1405,8 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	int rev_lo, rev_hi, ret;
 	unsigned short cec_addr;
 
+	mutex_init(&priv->audio_mutex); /* Protect access from audio thread */
+
 	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
 	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
 	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
@@ -1519,8 +1521,6 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 		priv->vip_cntrl_2 = video;
 	}
 
-	mutex_init(&priv->audio_mutex); /* Protect access from audio thread */
-
 	ret = tda998x_get_audio_ports(priv, np);
 	if (ret)
 		goto fail;


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* Re: [PATCH RFC 01/12] drm/i2c: tda998x: avoid race in tda998x_encoder_mode_set()
@ 2016-11-18  0:01     ` Russell King - ARM Linux
  0 siblings, 0 replies; 52+ messages in thread
From: Russell King - ARM Linux @ 2016-11-18  0:01 UTC (permalink / raw)
  To: Brian Starkey, Liviu Dudau, Jyri Sarha; +Cc: dri-devel, linux-arm-kernel

On Tue, Nov 08, 2016 at 12:25:00PM +0000, Russell King wrote:
> As priv->audio_params can now be changed at run time, we need to be more
> careful about how we deal with a mode set.  We must take the audio lock
> while checking if there's a valid audio configuration.
> 
> However, it's slightly worse than that - during mode set, we mute the
> audio, and it must not be unmuted until we have finished the mode set.
> It is possible that the audio side may start while a mode set is in
> progress, so take the audio_mutex lock around the whole mode setting
> procedure.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 088900d78ceb..1cc0433ce9d5 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1074,13 +1074,12 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  
>  		tda998x_write_avi(priv, adjusted_mode);
>  
> -		if (priv->audio_params.format != AFMT_UNUSED) {
> -			mutex_lock(&priv->audio_mutex);
> +		mutex_lock(&priv->audio_mutex);
> +		if (priv->audio_params.format != AFMT_UNUSED)
>  			tda998x_configure_audio(priv,
>  						&priv->audio_params,
>  						adjusted_mode->clock);
> -			mutex_unlock(&priv->audio_mutex);
> -		}
> +		mutex_unlock(&priv->audio_mutex);
>  	}
>  }
>  

I've just realised that this (and the following few changes) require this
mutex to be always initialised.  Currently, we only initialise the audio
mutex when in DT mode, which is bad.  So, I'm going to add this patch in
front of this patch set:

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 088900d78ceb..168c0b4b8f47 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1405,6 +1405,8 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	int rev_lo, rev_hi, ret;
 	unsigned short cec_addr;
 
+	mutex_init(&priv->audio_mutex); /* Protect access from audio thread */
+
 	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
 	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
 	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
@@ -1519,8 +1521,6 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 		priv->vip_cntrl_2 = video;
 	}
 
-	mutex_init(&priv->audio_mutex); /* Protect access from audio thread */
-
 	ret = tda998x_get_audio_ports(priv, np);
 	if (ret)
 		goto fail;


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

^ permalink raw reply related	[flat|nested] 52+ messages in thread
end of thread, other threads:[~2016-11-18  0:01 UTC | newest]

Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-08 12:24 [PATCH RFC 00/12] tda998x updates Russell King - ARM Linux
2016-11-08 12:24 ` Russell King - ARM Linux
2016-11-08 12:25 ` [PATCH RFC 01/12] drm/i2c: tda998x: avoid race in tda998x_encoder_mode_set() Russell King
2016-11-08 12:25   ` Russell King
2016-11-18  0:01   ` Russell King - ARM Linux
2016-11-18  0:01     ` Russell King - ARM Linux
2016-11-08 12:25 ` [PATCH RFC 02/12] drm/i2c: tda998x: avoid racy access to mode clock Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 03/12] drm/i2c: tda998x: avoid race when programming audio Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 04/12] drm/i2c: tda998x: only configure infoframes and audio if supported Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 05/12] drm/i2c: tda998x: only enable audio if supported by sink Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 06/12] drm/i2c: tda998x: correct function name in comments Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 07/12] drm/i2c: tda998x: move and rename tda998x_encoder_set_config() Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 08/12] drm/i2c: tda998x: group connector functions and funcs together Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 09/12] drm/i2c: tda998x: separate connector initialisation Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 10/12] drm/i2c: tda998x: group audio functions together Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 11/12] drm/i2c: tda998x: remove complexity from tda998x_audio_get_eld() Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 12:25 ` [PATCH RFC 12/12] drm/i2c: tda998x: switch to boolean is_on Russell King
2016-11-08 12:25   ` Russell King
2016-11-08 13:19 ` [PATCH RFC 00/12] tda998x updates Robin Murphy
2016-11-08 13:19   ` Robin Murphy
2016-11-08 13:32   ` Russell King - ARM Linux
2016-11-08 13:32     ` Russell King - ARM Linux
2016-11-08 13:34     ` Russell King - ARM Linux
2016-11-08 13:34       ` Russell King - ARM Linux
2016-11-08 15:25       ` Robin Murphy
2016-11-08 15:25         ` Robin Murphy
2016-11-08 15:42         ` Russell King - ARM Linux
2016-11-08 15:42           ` Russell King - ARM Linux
2016-11-08 17:20       ` Jon Medhurst (Tixy)
2016-11-08 17:20         ` Jon Medhurst (Tixy)
2016-11-08 18:24         ` Russell King - ARM Linux
2016-11-08 18:24           ` Russell King - ARM Linux
2016-11-09 11:45           ` Jon Medhurst (Tixy)
2016-11-09 11:45             ` Jon Medhurst (Tixy)
2016-11-09 22:14             ` Russell King - ARM Linux
2016-11-09 22:14               ` Russell King - ARM Linux
2016-11-11 15:10 ` Jyri Sarha
2016-11-11 15:10   ` Jyri Sarha
2016-11-11 15:27   ` Russell King - ARM Linux
2016-11-11 15:27     ` Russell King - ARM Linux
2016-11-11 16:24     ` Jyri Sarha
2016-11-11 16:24       ` Jyri Sarha

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.