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

[PATCH RESEND 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 remove clearing the interrupt flags,
they are cleared in POWER_DOWN mode anyhow (according to docs and my
tests).

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 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 00416f23b5cb5f..85e994796d96a4 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 tests with a 7511w show something else
+		 * but let's 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

[PATCH RESEND 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>
---
 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 85e994796d96a4..50a861b12346c4 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

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

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 04 January 2016 03:33:46 Wolfram Sang wrote:
> 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>

A small note though, even though the patch is correct, it will be of limited 
use as the EDID_READ_CTRL register is never accessed by the driver.

> ---
>  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 85e994796d96a4..50a861b12346c4 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:

-- 
Regards,

Laurent Pinchart

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

[Wolfram Sang]

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


> A small note though, even though the patch is correct, it will be of limited 
> use as the EDID_READ_CTRL register is never accessed by the driver.

It was useful to me when debugging and looking at the register set in
debugfs.


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

[PATCH RESEND 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>
---
 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 50a861b12346c4..c03c1ea53fd042 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 RESEND 3/3] drm: adv7511: it's HPD, not HDP

[Lars-Peter Clausen]

On 01/04/2016 03:33 AM, Wolfram Sang wrote:
> 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>

Thanks.

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

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 04 January 2016 03:33:47 Wolfram Sang wrote:
> 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  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 50a861b12346c4..c03c1ea53fd042 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)

-- 
Regards,

Laurent Pinchart

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

[Archit Taneja]

On 01/04/2016 08:03 AM, 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 remove clearing the interrupt flags,
> they are cleared in POWER_DOWN mode anyhow (according to docs and my
> tests).

I tried this on adv7533 and it works fine. The other patches look good
too.

Tested-by: Archit Taneja <architt@codeaurora.org>

Thanks,
Archit

>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>   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 00416f23b5cb5f..85e994796d96a4 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 tests with a 7511w show something else
> +		 * but let's 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;
>   	}
>
>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation

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

[Laurent Pinchart]

Hi Wolfram,

Thank you for the patch.

On Monday 04 January 2016 03:33:45 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 remove clearing the interrupt flags,
> they are cleared in POWER_DOWN mode anyhow (according to docs and my
> tests).
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  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 00416f23b5cb5f..85e994796d96a4 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 tests with a 7511w show something else
> +		 * but let's stick to the documentation.

This contradicts the commit message, which one is correct ?

> +		 */
> +		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;
>  	}

-- 
Regards,

Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Laurent,

On Mon, Jan 4, 2016 at 3:37 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 04 January 2016 03:33:45 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 remove clearing the interrupt flags,
>> they are cleared in POWER_DOWN mode anyhow (according to docs and my
>> tests).
>>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>  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 00416f23b5cb5f..85e994796d96a4 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 tests with a 7511w show something else
>> +              * but let's stick to the documentation.
>
> This contradicts the commit message, which one is correct ?

Initially I thought the same, until I realized the commit message refers to...

>> +              */
>> +             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);

... this removal ^^^^^.

>>               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;
>>       }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Wolfram Sang]

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

On Mon, Jan 04, 2016 at 04:37:44PM +0200, Laurent Pinchart wrote:
> Hi Wolfram,
> 
> Thank you for the patch.
> 
> On Monday 04 January 2016 03:33:45 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 remove clearing the interrupt flags,
> > they are cleared in POWER_DOWN mode anyhow (according to docs and my
> > tests).
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >  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 00416f23b5cb5f..85e994796d96a4 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 tests with a 7511w show something else
> > +		 * but let's stick to the documentation.
> 
> This contradicts the commit message, which one is correct ?

As Geert mentioned, the commit message refers to the interrupt flags not
the interrupt_enable flags.

Shall I rephrase?


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