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(-)
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; }
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...
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:
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)
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!
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.
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(-)
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Wolfram Sang