[PATCH v2 0/3] drm: adv7511: edid read fixes

[PATCH v2 0/3] drm: adv7511: edid read fixes

[Wolfram Sang]

So, here is my V2 of this series. Changes:

* Added tags. Thanks to Laurent and Lars-Peter for review and to Archit for
  testing. Much appreciated!
* rephrased the comment and commit message in patch 1 to be hopefully less
  confusing :)

No code changes!

Thanks,

   Wolfram


Wolfram Sang (3):
  drm: adv7511: really enable interrupts for EDID detection
  drm: adv7511: mark ADV7511_REG_EDID_READ_CTRL volatile
  drm: adv7511: it's HPD, not HDP

 drivers/gpu/drm/i2c/adv7511.c | 48 ++++++++++++++++++++++++++-----------------
 drivers/gpu/drm/i2c/adv7511.h | 12 +++++------
 2 files changed, 35 insertions(+), 25 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

The interrupts for EDID_READY or DDC_ERROR were never enabled in this
driver, so reading EDID always timed out when chip was powered down and
interrupts were used. Fix this and also remove clearing the interrupt
flags, they are cleared in POWER_DOWN mode anyhow (unlike the interrupt
enable flags) according to docs and my tests.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/i2c/adv7511.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
index 533d1e3d4a999f..db7435a4517fb4 100644
--- a/drivers/gpu/drm/i2c/adv7511.c
+++ b/drivers/gpu/drm/i2c/adv7511.c
@@ -362,12 +362,19 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 {
 	adv7511->current_edid_segment = -1;
 
-	regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
-		     ADV7511_INT0_EDID_READY);
-	regmap_write(adv7511->regmap, ADV7511_REG_INT(1),
-		     ADV7511_INT1_DDC_ERROR);
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
 			   ADV7511_POWER_POWER_DOWN, 0);
+	if (adv7511->i2c_main->irq) {
+		/*
+		 * Documentation says the INT_ENABLE registers are reset in
+		 * POWER_DOWN mode. My 7511w preserved the bits, however.
+		 * Still, let's be safe and stick to the documentation.
+		 */
+		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
+			     ADV7511_INT0_EDID_READY);
+		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
+			     ADV7511_INT1_DDC_ERROR);
+	}
 
 	/*
 	 * Per spec it is allowed to pulse the HDP signal to indicate that the
@@ -567,12 +574,14 @@ static int adv7511_get_modes(struct drm_encoder *encoder,
 
 	/* Reading the EDID only works if the device is powered */
 	if (!adv7511->powered) {
-		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
-			     ADV7511_INT0_EDID_READY);
-		regmap_write(adv7511->regmap, ADV7511_REG_INT(1),
-			     ADV7511_INT1_DDC_ERROR);
 		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
 				   ADV7511_POWER_POWER_DOWN, 0);
+		if (adv7511->i2c_main->irq) {
+			regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
+				     ADV7511_INT0_EDID_READY);
+			regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
+				     ADV7511_INT1_DDC_ERROR);
+		}
 		adv7511->current_edid_segment = -1;
 	}
 
-- 
2.1.4

Re: [PATCH v2 1/3] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

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

On Fri, Jan 08, 2016 at 10:56:48PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> The interrupts for EDID_READY or DDC_ERROR were never enabled in this
> driver, so reading EDID always timed out when chip was powered down and
> interrupts were used. Fix this and also remove clearing the interrupt
> flags, they are cleared in POWER_DOWN mode anyhow (unlike the interrupt
> enable flags) according to docs and my tests.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Archit Taneja <architt@codeaurora.org>

Unsure who is the right person to pick it up, adding Hans to CC...


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

[PATCH v2 2/3] drm: adv7511: mark ADV7511_REG_EDID_READ_CTRL volatile

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

This register includes a counter which is decremented by the chip on I2C
failures. Also, it is reset when powering down.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/i2c/adv7511.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
index db7435a4517fb4..e1756a452d6980 100644
--- a/drivers/gpu/drm/i2c/adv7511.c
+++ b/drivers/gpu/drm/i2c/adv7511.c
@@ -136,6 +136,7 @@ static bool adv7511_register_volatile(struct device *dev, unsigned int reg)
 	case ADV7511_REG_BKSV(3):
 	case ADV7511_REG_BKSV(4):
 	case ADV7511_REG_DDC_STATUS:
+	case ADV7511_REG_EDID_READ_CTRL:
 	case ADV7511_REG_BSTATUS(0):
 	case ADV7511_REG_BSTATUS(1):
 	case ADV7511_REG_CHIP_ID_HIGH:
-- 
2.1.4

[PATCH v2 3/3] drm: adv7511: it's HPD, not HDP

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

Fix this typo, consequently used over both files :)

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Acked-by: Lars-Peter Clausen <lars@metafoo.de>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Archit Taneja <architt@codeaurora.org>
---
 drivers/gpu/drm/i2c/adv7511.c | 22 +++++++++++-----------
 drivers/gpu/drm/i2c/adv7511.h | 12 ++++++------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
index e1756a452d6980..a02112ba1c3df6 100644
--- a/drivers/gpu/drm/i2c/adv7511.c
+++ b/drivers/gpu/drm/i2c/adv7511.c
@@ -378,16 +378,16 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 	}
 
 	/*
-	 * Per spec it is allowed to pulse the HDP signal to indicate that the
+	 * Per spec it is allowed to pulse the HPD signal to indicate that the
 	 * EDID information has changed. Some monitors do this when they wakeup
-	 * from standby or are enabled. When the HDP goes low the adv7511 is
+	 * from standby or are enabled. When the HPD goes low the adv7511 is
 	 * reset and the outputs are disabled which might cause the monitor to
-	 * go to standby again. To avoid this we ignore the HDP pin for the
+	 * go to standby again. To avoid this we ignore the HPD pin for the
 	 * first few seconds after enabling the output.
 	 */
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
-			   ADV7511_REG_POWER2_HDP_SRC_MASK,
-			   ADV7511_REG_POWER2_HDP_SRC_NONE);
+			   ADV7511_REG_POWER2_HPD_SRC_MASK,
+			   ADV7511_REG_POWER2_HPD_SRC_NONE);
 
 	/*
 	 * Most of the registers are reset during power down or when HPD is low.
@@ -421,9 +421,9 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
 	if (ret < 0)
 		return false;
 
-	if (irq0 & ADV7511_INT0_HDP) {
+	if (irq0 & ADV7511_INT0_HPD) {
 		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
-			     ADV7511_INT0_HDP);
+			     ADV7511_INT0_HPD);
 		return true;
 	}
 
@@ -446,7 +446,7 @@ static int adv7511_irq_process(struct adv7511 *adv7511)
 	regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
 	regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
 
-	if (irq0 & ADV7511_INT0_HDP && adv7511->encoder)
+	if (irq0 & ADV7511_INT0_HPD && adv7511->encoder)
 		drm_helper_hpd_irq_event(adv7511->encoder->dev);
 
 	if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
@@ -648,10 +648,10 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
 		if (adv7511->status = connector_status_connected)
 			status = connector_status_disconnected;
 	} else {
-		/* Renable HDP sensing */
+		/* Renable HPD sensing */
 		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
-				   ADV7511_REG_POWER2_HDP_SRC_MASK,
-				   ADV7511_REG_POWER2_HDP_SRC_BOTH);
+				   ADV7511_REG_POWER2_HPD_SRC_MASK,
+				   ADV7511_REG_POWER2_HPD_SRC_BOTH);
 	}
 
 	adv7511->status = status;
diff --git a/drivers/gpu/drm/i2c/adv7511.h b/drivers/gpu/drm/i2c/adv7511.h
index 6599ed538426d6..38515b30cedfc8 100644
--- a/drivers/gpu/drm/i2c/adv7511.h
+++ b/drivers/gpu/drm/i2c/adv7511.h
@@ -90,7 +90,7 @@
 #define ADV7511_CSC_ENABLE			BIT(7)
 #define ADV7511_CSC_UPDATE_MODE			BIT(5)
 
-#define ADV7511_INT0_HDP			BIT(7)
+#define ADV7511_INT0_HPD			BIT(7)
 #define ADV7511_INT0_VSYNC			BIT(5)
 #define ADV7511_INT0_AUDIO_FIFO_FULL		BIT(4)
 #define ADV7511_INT0_EDID_READY			BIT(2)
@@ -157,11 +157,11 @@
 #define ADV7511_PACKET_ENABLE_SPARE2		BIT(1)
 #define ADV7511_PACKET_ENABLE_SPARE1		BIT(0)
 
-#define ADV7511_REG_POWER2_HDP_SRC_MASK		0xc0
-#define ADV7511_REG_POWER2_HDP_SRC_BOTH		0x00
-#define ADV7511_REG_POWER2_HDP_SRC_HDP		0x40
-#define ADV7511_REG_POWER2_HDP_SRC_CEC		0x80
-#define ADV7511_REG_POWER2_HDP_SRC_NONE		0xc0
+#define ADV7511_REG_POWER2_HPD_SRC_MASK		0xc0
+#define ADV7511_REG_POWER2_HPD_SRC_BOTH		0x00
+#define ADV7511_REG_POWER2_HPD_SRC_HPD		0x40
+#define ADV7511_REG_POWER2_HPD_SRC_CEC		0x80
+#define ADV7511_REG_POWER2_HPD_SRC_NONE		0xc0
 #define ADV7511_REG_POWER2_TDMS_ENABLE		BIT(4)
 #define ADV7511_REG_POWER2_GATE_INPUT_CLK	BIT(0)
 
-- 
2.1.4

Re: [PATCH v2 0/3] drm: adv7511: edid read fixes

[Wolfram Sang]

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

On Fri, Jan 08, 2016 at 10:56:47PM +0100, Wolfram Sang wrote:
> So, here is my V2 of this series. Changes:
> 
> * Added tags. Thanks to Laurent and Lars-Peter for review and to Archit for
>   testing. Much appreciated!
> * rephrased the comment and commit message in patch 1 to be hopefully less
>   confusing :)
> 
> No code changes!
> 
> Thanks,
> 
>    Wolfram
> 
> 
> Wolfram Sang (3):
>   drm: adv7511: really enable interrupts for EDID detection
>   drm: adv7511: mark ADV7511_REG_EDID_READ_CTRL volatile
>   drm: adv7511: it's HPD, not HDP
> 
>  drivers/gpu/drm/i2c/adv7511.c | 48 ++++++++++++++++++++++++++-----------------
>  drivers/gpu/drm/i2c/adv7511.h | 12 +++++------
>  2 files changed, 35 insertions(+), 25 deletions(-)

Adding Hans to CC of the proper mail, not patch 1. Sorry for the noise!


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

Re: [PATCH v2 0/3] drm: adv7511: edid read fixes

[Laurent Pinchart]

Hi Wolfram,

On Monday 01 February 2016 14:31:20 Wolfram Sang wrote:
> On Fri, Jan 08, 2016 at 10:56:47PM +0100, Wolfram Sang wrote:
> > So, here is my V2 of this series. Changes:
> > 
> > * Added tags. Thanks to Laurent and Lars-Peter for review and to Archit
> > for
> > 
> >   testing. Much appreciated!
> > 
> > * rephrased the comment and commit message in patch 1 to be hopefully less
> > 
> >   confusing :)
> > 
> > No code changes!
> > 
> > Thanks,
> > 
> >    Wolfram
> > 
> > Wolfram Sang (3):
> >   drm: adv7511: really enable interrupts for EDID detection
> >   drm: adv7511: mark ADV7511_REG_EDID_READ_CTRL volatile
> >   drm: adv7511: it's HPD, not HDP
> >  
> >  drivers/gpu/drm/i2c/adv7511.c | 48 ++++++++++++++++++++++----------------
> >  drivers/gpu/drm/i2c/adv7511.h | 12 +++++------
> >  2 files changed, 35 insertions(+), 25 deletions(-)
> 
> Adding Hans to CC of the proper mail, not patch 1. Sorry for the noise!

My bad, I got the drivers mistaken when we talked about it, I thought you 
meant the V4L2 driver, not the DRM driver. This series should be merged by 
Dave Airlie. The easiest is to send him a pull request.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] drm: adv7511: edid read fixes

[Wolfram Sang]

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

David,

can you pull in these patches which have been around for some time now
and have been acked/reviewed by relevant people? The branch is based on
your next branch as of yesterday.

Thanks,

   Wolfram

The following changes since commit 1df59b8497f47495e873c23abd6d3d290c730505:

  Merge tag 'drm-intel-next-fixes-2016-01-14' of git://anongit.freedesktop.org/drm-intel into drm-next (2016-01-18 07:02:19 +1000)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git drm/adv7511

for you to fetch changes up to 29ce4ed441d04a8931150f291c0f7d961690ab81:

  drm: adv7511: it's HPD, not HDP (2016-02-02 15:37:55 +0100)

----------------------------------------------------------------
Wolfram Sang (3):
      drm: adv7511: really enable interrupts for EDID detection
      drm: adv7511: mark ADV7511_REG_EDID_READ_CTRL volatile
      drm: adv7511: it's HPD, not HDP

 drivers/gpu/drm/i2c/adv7511.c | 48 +++++++++++++++++++++++++++++-------------------
 drivers/gpu/drm/i2c/adv7511.h | 12 ++++++------
 2 files changed, 35 insertions(+), 25 deletions(-)


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