Skip to contentSkip to conversation

adv7511 EDID probing improvements

PATCH Applied John Stultz 3 participants 13 messages Jan 17, 2017 – Jan 19, 2017

Subsystems (3)
DRM drivers DRM misc DRM drivers for bridge chips
lore
Share & tools
lore

John Stultz

author
#0 lore raw

v4 cover adv7511 EDID probing improvements

Wanted to re-send out v4 of this patch set, integrating some changes suggested by Laurent, for consideration for merging for v4.11

The first three patches are fixups that are hopefully straight forward, integrating feedback I got from Laurant, with minimal change from the previous versions.

The last three patches try to clean up and rework the EDID probing code, to avoid issues seen on HiKey. I've reworked these more significantly since v3 to address feedback from Laurent.

Thoughts and feedback would be appreciated!

thanks
-john

New in v4:
* Tweaked connector.status assignment to avoid race, as suggested by Laurent
* Reworked the __adv7511_power_on helpers to avoid calling regcache_sync in the EDID probe path
* Added new patch to set EDID_I2C_ADDR register before doing EDID read.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]

Archit Taneja (1):
drm/bridge: adv7511: Enable HPD interrupts to support hotplug and
improve monitor detection

John Stultz (5):
drm/bridge: adv7511: Use work_struct to defer hotplug handing to out
of irq context
drm/bridge: adv7511: Switch to using drm_kms_helper_hotplug_event() drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be
reused internally
drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID drm/bridge: adv7511: Re-write the i2c address before EDID probing

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 +
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 67 ++++++++++++++++++++--------
 2 files changed, 51 insertions(+), 18 deletions(-)
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#1 lore raw

PATCH v4 1/6 drm/bridge: adv7511: Use work_struct to defer hotplug handing to out of irq context

I was recently seeing issues with EDID probing, where the logic to wait for the EDID read bit to be set by the IRQ wasn't happening and the code would time out and fail.

Digging deeper, I found this was due to the fact that IRQs were disabled as we were running in IRQ context from the HPD signal.

Thus this patch changes the logic to handle the HPD signal via a work_struct so we can be out of irq context.

With this patch, the EDID probing on hotplug does not time out.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]
Reviewed-by Laurent Pinchart <[email protected]>
Tested-by Laurent Pinchart <[email protected]>
Signed-off-by John Stultz <[email protected]>

---
v3: Rename irq_work to hpd_work and remove extra whitespace, as
suggested by Laurent

---
 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 ++
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)
drivers/gpu/drm/bridge/adv7511/adv7511.h +2 -0
index 992d76c..0396791 100644
@@ -317,6 +317,8 @@ struct adv7511 {
 	bool edid_read;
 
 	wait_queue_head_t wq;
+	struct work_struct hpd_work;
+
 	struct drm_bridge bridge;
 	struct drm_connector connector;
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +10 -1
index 8dba729..4fcea44 100644
@@ -402,6 +402,13 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
 	return false;
 }
 
+static void adv7511_hpd_work(struct work_struct *work)
+{
+	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
+
+	drm_helper_hpd_irq_event(adv7511->connector.dev);
+}
+
 static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
 {
 	unsigned int irq0, irq1;

@@ -419,7 +426,7 @@ static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
 	regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
 
 	if (process_hpd && irq0 & ADV7511_INT0_HPD && adv7511->bridge.encoder)
-		drm_helper_hpd_irq_event(adv7511->connector.dev);
+		schedule_work(&adv7511->hpd_work);
 
 	if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
 		adv7511->edid_read = true;

@@ -1006,6 +1013,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 			goto err_i2c_unregister_edid;
 	}
 
+	INIT_WORK(&adv7511->hpd_work, adv7511_hpd_work);
+
 	if (i2c->irq) {
 		init_waitqueue_head(&adv7511->wq);
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#2 lore raw

PATCH v4 2/6 drm/bridge: adv7511: Switch to using drm_kms_helper_hotplug_event()

In reply to John Stultz

In chasing down a previous issue with EDID probing from calling drm_helper_hpd_irq_event() from irq context, Laurent noticed that the DRM documentation suggests that drm_kms_helper_hotplug_event() should be used instead.

Thus this patch replaces drm_helper_hpd_irq_event() with drm_kms_helper_hotplug_event(), which requires we update the connector.status entry and only call _hotplug_event() when the status changes.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]
Reviewed-by Laurent Pinchart <[email protected]>
Tested-by Laurent Pinchart <[email protected]>
Signed-off-by John Stultz <[email protected]>

---
v3: Update connector.status value and only call __hotplug_event()
when that status changes, as suggested by Laurent.
v4: Set adv7511->connector.status before calling

    adv7511->connector.status, as suggested by Laurent.
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +15 -1
index 4fcea44..7b2b5af 100644
@@ -405,8 +405,22 @@ static bool adv7511_hpd(struct adv7511 *adv7511)
 static void adv7511_hpd_work(struct work_struct *work)
 {
 	struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work);
+	enum drm_connector_status status;
+	unsigned int val;
+	int ret;
 
-	drm_helper_hpd_irq_event(adv7511->connector.dev);
+	ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS, &val);
+	if (ret < 0)
+		status = connector_status_disconnected;
+	else if (val & ADV7511_STATUS_HPD)
+		status = connector_status_connected;
+	else
+		status = connector_status_disconnected;
+
+	if (adv7511->connector.status != status) {
+		adv7511->connector.status = status;
+		drm_kms_helper_hotplug_event(adv7511->connector.dev);
+	}
 }
 
 static int adv7511_irq_process(struct adv7511 *adv7511, bool process_hpd)
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#3 lore raw

PATCH v4 3/6 drm/bridge: adv7511: Enable HPD interrupts to support hotplug and improve monitor detection

In reply to John Stultz

From: Archit Taneja <[email protected]>

On some adv7511 implementations, we can get some spurious disconnect signals which can cause monitor probing to fail.

This patch enables HPD (hot plug detect) interrupt support which allows the monitor to be properly re-initialized when the spurious disconnect signal goes away.

This also enables proper hotplug support.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]
Acked-by Laurent Pinchart <[email protected]>
Tested-by Laurent Pinchart <[email protected]>
Originally-by Archit Taneja <[email protected]>
[jstultz: Added proper commit message]
Signed-off-by John Stultz <[email protected]>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +5 -1
index 7b2b5af..405e460 100644
@@ -338,7 +338,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 		 * Still, let's be safe and stick to the documentation.
 		 */
 		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
-			     ADV7511_INT0_EDID_READY);
+			     ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD);
 		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
 			     ADV7511_INT1_DDC_ERROR);
 	}

@@ -846,6 +846,10 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
 	if (adv->type == ADV7533)
 		ret = adv7533_attach_dsi(adv);
 
+	if (adv->i2c_main->irq)
+		regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0),
+				ADV7511_INT0_HPD);
+
 	return ret;
 }
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#4 lore raw

PATCH v4 4/6 drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

In reply to John Stultz

In chasing down issues with EDID probing, I found some duplicated but incomplete logic used to power the chip on and off.

This patch refactors the adv7511_power_on/off functions, so they can be used for internal needs.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>

---
v4: Move the regcache_sync() call outside the shared
implementation as we don't want to call it on EDID probing, as suggested by Laurent

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +11 -4
index 405e460..12f2876 100644
@@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511 *adv7511,
 	adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
 }
 
-static void adv7511_power_on(struct adv7511 *adv7511)
+static void __adv7511_power_on(struct adv7511 *adv7511)
 {
 	adv7511->current_edid_segment = -1;
 

@@ -354,6 +354,11 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
 			   ADV7511_REG_POWER2_HPD_SRC_MASK,
 			   ADV7511_REG_POWER2_HPD_SRC_NONE);
+}
+
+static void adv7511_power_on(struct adv7511 *adv7511)
+{
+	__adv7511_power_on(adv7511);
 
 	/*
 	 * Most of the registers are reset during power down or when HPD is low.

@@ -362,21 +367,23 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_on(adv7511);
-
 	adv7511->powered = true;
 }
 
-static void adv7511_power_off(struct adv7511 *adv7511)
+static void __adv7511_power_off(struct adv7511 *adv7511)
 {
 	/* TODO: setup additional power down modes */
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
 			   ADV7511_POWER_POWER_DOWN,
 			   ADV7511_POWER_POWER_DOWN);
 	regcache_mark_dirty(adv7511->regmap);
+}
 
+static void adv7511_power_off(struct adv7511 *adv7511)
+{
+	__adv7511_power_off(adv7511);
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_off(adv7511);
-
 	adv7511->powered = false;
 }
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
reviewer Reviewed-by on v4 4/6
#5 lore raw

Re: [PATCH 4/6] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

Hi John,

Thank you for the patch.

Quoted 7 lines · On Monday 16 Jan 2017 16:52:50 John Stultz wrote:

In chasing down issues with EDID probing, I found some duplicated but incomplete logic used to power the chip on and off.

This patch refactors the adv7511_power_on/off functions, so they can be used for internal needs.

Quoted 7 lines
Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>
Reviewed-by Laurent Pinchart <[email protected]>
Quoted 24 lines

---
v4: Move the regcache_sync() call outside the shared
implementation as we don't want to call it on EDID probing, as suggested by Laurent

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 405e460..12f2876 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +1 -1
@@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511
 *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
 }
 
-static void adv7511_power_on(struct adv7511 *adv7511)
+static void __adv7511_power_on(struct adv7511 *adv7511)
 {
 	adv7511->current_edid_segment = -1;
 

@@ -354,6 +354,11 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static void adv7511_power_on(struct adv7511 *adv7511) · line 364

Quoted 10 lines
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +5 -0
 			   ADV7511_REG_POWER2_HPD_SRC_MASK,
 			   ADV7511_REG_POWER2_HPD_SRC_NONE);
+}
+
+static void adv7511_power_on(struct adv7511 *adv7511)
+{
+	__adv7511_power_on(adv7511);
 
 	/*
 	 * Most of the registers are reset during power down or when HPD is

low.

Quoted 26 lines
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +5 -3
@@ -362,21 +367,23 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_on(adv7511);
-
 	adv7511->powered = true;
 }
 
-static void adv7511_power_off(struct adv7511 *adv7511)
+static void __adv7511_power_off(struct adv7511 *adv7511)
 {
 	/* TODO: setup additional power down modes */
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
 			   ADV7511_POWER_POWER_DOWN,
 			   ADV7511_POWER_POWER_DOWN);
 	regcache_mark_dirty(adv7511->regmap);
+}
 
+static void adv7511_power_off(struct adv7511 *adv7511)
+{
+	__adv7511_power_off(adv7511);
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_off(adv7511);
-
 	adv7511->powered = false;
 }
Signature · 8 lines
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#6 lore raw

PATCH v4 5/6 drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID

In reply to John Stultz

Thus this patch changes the EDID probing logic so that we re-use the __adv7511_power_on/off() calls instead of duplciating logic.

This does change behavior slightly as it adds the HPD signal pulse to the EDID probe path, but Archit has had a patch to add HPD signal pulse to the EDID probe path before, so this should address the cases where that helped.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>

---
v4: Reworded commit message, focusing on re-use and HPD pulse
behavior change.

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +3 -14
index 12f2876..d216f61 100644
@@ -573,24 +573,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 	unsigned int count;
 
 	/* Reading the EDID only works if the device is powered */
-	if (!adv7511->powered) {
-		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;
-	}
+	if (!adv7511->powered)
+		__adv7511_power_on(adv7511);
 
 	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
 
 	if (!adv7511->powered)
-		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-				   ADV7511_POWER_POWER_DOWN,
-				   ADV7511_POWER_POWER_DOWN);
+		__adv7511_power_off(adv7511);
 
 	kfree(adv7511->edid);
 	adv7511->edid = edid;
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
reviewer Reviewed-by on v4 5/6
#7 lore raw

Re: [PATCH 5/6] drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID

Hi John,

Thank you for the patch.

Quoted 8 lines · On Monday 16 Jan 2017 16:52:51 John Stultz wrote:

Thus this patch changes the EDID probing logic so that we re-use the __adv7511_power_on/off() calls instead of duplciating logic.

This does change behavior slightly as it adds the HPD signal pulse to the EDID probe path, but Archit has had a patch to add HPD signal pulse to the EDID probe path before, so this should address the cases where that helped.

You should also mentioned that the power off path now calls regcache_mark_dirty(). Apart from that,

Reviewed-by Laurent Pinchart <[email protected]>
Quoted 20 lines
Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>

---
v4: Reworded commit message, focusing on re-use and HPD pulse
behavior change.

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 12f2876..d216f61 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static int adv7511_get_modes(struct adv7511 *adv7511, · line 580

Quoted 8 lines
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +0 -5
@@ -573,24 +573,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 	unsigned int count;
 
 	/* Reading the EDID only works if the device is powered */
-	if (!adv7511->powered) {
-		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),

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c static int adv7511_get_modes(struct adv7511 *adv7511, · line 582

Quoted 2 lines
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +0 -2
-				     ADV7511_INT0_EDID_READY);
-			regmap_write(adv7511->regmap,

ADV7511_REG_INT_ENABLE(1),

Quoted 17 lines
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +3 -7
-				     ADV7511_INT1_DDC_ERROR);
-		}
-		adv7511->current_edid_segment = -1;
-	}
+	if (!adv7511->powered)
+		__adv7511_power_on(adv7511);
 
 	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
 
 	if (!adv7511->powered)
-		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-				   ADV7511_POWER_POWER_DOWN,
-				   ADV7511_POWER_POWER_DOWN);
+		__adv7511_power_off(adv7511);
 
 	kfree(adv7511->edid);
 	adv7511->edid = edid;
Signature · 8 lines
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#8 lore raw

PATCH v4 6/6 drm/bridge: adv7511: Re-write the i2c address before EDID probing

In reply to John Stultz

I've found that by just turning the chip on and off via the POWER_DOWN register, I end up getting i2c_transfer errors on HiKey.

Investigating further, it seems some of the register state in the regmap cache is getting lost, likely as the device registers were reset during power off.

Thus this patch simply re-writes the i2c address to the ADV7511_REG_EDID_I2C_ADDR register to ensure its properly set before we try to read the EDID data.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>

---
v4: New approach to make the EDID_I2C_ADDR register
sane, as suggested by Laurent

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +9 -1
index d216f61..0ed89ea 100644
@@ -573,9 +573,17 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 	unsigned int count;
 
 	/* Reading the EDID only works if the device is powered */
-	if (!adv7511->powered)
+	if (!adv7511->powered) {
+		unsigned int edid_i2c_addr =
+					(adv7511->i2c_main->addr << 1) + 4;
+
 		__adv7511_power_on(adv7511);
 
+		/* Reset the EDID_I2C_ADDR register as it might be cleared */
+		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
+			     edid_i2c_addr);
+	}
+
 	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
 
 	if (!adv7511->powered)
Signature · 6 lines
2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
reviewer Reviewed-by on v4 6/6
#9 lore raw

Re: [PATCH 6/6] drm/bridge: adv7511: Re-write the i2c address before EDID probing

Hi John,

Thank you for the patch.

Quoted 7 lines · On Monday 16 Jan 2017 16:52:52 John Stultz wrote:

I've found that by just turning the chip on and off via the POWER_DOWN register, I end up getting i2c_transfer errors on HiKey.

Investigating further, it seems some of the register state in the regmap cache is getting lost, likely as the device registers were reset during power off.

It's not the state in the regmap cache that is lost, but the state in the hardware, indeed because the registers contents are lost when the chip is powered down.

Reviewed-by Laurent Pinchart <[email protected]>
Quoted 42 lines

Thus this patch simply re-writes the i2c address to the ADV7511_REG_EDID_I2C_ADDR register to ensure its properly set before we try to read the EDID data.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]
Signed-off-by John Stultz <[email protected]>

---
v4: New approach to make the EDID_I2C_ADDR register
sane, as suggested by Laurent

---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index d216f61..0ed89ea 100644

drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +9 -1
@@ -573,9 +573,17 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 	unsigned int count;
 
 	/* Reading the EDID only works if the device is powered */
-	if (!adv7511->powered)
+	if (!adv7511->powered) {
+		unsigned int edid_i2c_addr =
+					(adv7511->i2c_main->addr << 1) + 4;
+
 		__adv7511_power_on(adv7511);
 
+		/* Reset the EDID_I2C_ADDR register as it might be cleared */
+		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR,
+			     edid_i2c_addr);
+	}
+
 	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
 
 	if (!adv7511->powered)
Signature · 8 lines
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
reviewer Tested-by on the v4 series
#10 lore raw
In reply to John Stultz

Hi John,

Thank you for the patches.

Quoted 8 lines · On Monday 16 Jan 2017 16:52:46 John Stultz wrote:

Wanted to re-send out v4 of this patch set, integrating some changes suggested by Laurent, for consideration for merging for v4.11

The first three patches are fixups that are hopefully straight forward, integrating feedback I got from Laurant, with minimal change from the previous versions.

Quoted 5 lines

The last three patches try to clean up and rework the EDID probing code, to avoid issues seen on HiKey. I've reworked these more significantly since v3 to address feedback from Laurent.

Thoughts and feedback would be appreciated!

For the whole series,

Tested-by Laurent Pinchart <[email protected]>
Quoted 32 lines

New in v4:
* Tweaked connector.status assignment to avoid race, as suggested by Laurent
* Reworked the __adv7511_power_on helpers to avoid calling regcache_sync in the EDID probe path
* Added new patch to set EDID_I2C_ADDR register before doing EDID read.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]

Archit Taneja (1):
drm/bridge: adv7511: Enable HPD interrupts to support hotplug and
improve monitor detection

John Stultz (5):
drm/bridge: adv7511: Use work_struct to defer hotplug handing to out
of irq context
drm/bridge: adv7511: Switch to using drm_kms_helper_hotplug_event() drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be
reused internally
drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID drm/bridge: adv7511: Re-write the i2c address before EDID probing

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 +
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 67

++++++++++++++++++++-------- 2 files changed, 51 insertions(+), 18 deletions(-)

Signature · 8 lines
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Applied
#11 lore raw
Quoted 17 lines · Laurent Pinchart

Hi John,

Thank you for the patches.

Quoted 12 lines · On Monday 16 Jan 2017 16:52:46 John Stultz wrote:

Wanted to re-send out v4 of this patch set, integrating some changes suggested by Laurent, for consideration for merging for v4.11

The first three patches are fixups that are hopefully straight forward, integrating feedback I got from Laurant, with minimal change from the previous versions.

The last three patches try to clean up and rework the EDID probing code, to avoid issues seen on HiKey. I've reworked these more significantly since v3 to address feedback from Laurent.

Quoted 5 lines
Quoted 1 line

Thoughts and feedback would be appreciated!

For the whole series,

Tested-by Laurent Pinchart <[email protected]>

Queued to drm-misc-next. Updated the commit messages for patches 5 and 6 as suggested by Laurent.

Thanks,
Archit

Quoted 34 lines
Quoted 33 lines

New in v4:
* Tweaked connector.status assignment to avoid race, as suggested by Laurent
* Reworked the __adv7511_power_on helpers to avoid calling regcache_sync in the EDID probe path
* Added new patch to set EDID_I2C_ADDR register before doing EDID read.

Cc David Airlie <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Wolfram Sang <[email protected]>
Cc Lars-Peter Clausen <[email protected]>
Cc Laurent Pinchart <[email protected]>
Cc [email protected]

Archit Taneja (1):
drm/bridge: adv7511: Enable HPD interrupts to support hotplug and
improve monitor detection

John Stultz (5):
drm/bridge: adv7511: Use work_struct to defer hotplug handing to out
of irq context
drm/bridge: adv7511: Switch to using drm_kms_helper_hotplug_event() drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be
reused internally
drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID drm/bridge: adv7511: Re-write the i2c address before EDID probing

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 +
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 67

++++++++++++++++++++-------- 2 files changed, 51 insertions(+), 18 deletions(-)

Signature · 6 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#12 lore raw
Quoted 25 lines · Archit Taneja
Quoted 22 lines · Laurent Pinchart

Hi John,

Thank you for the patches.

Quoted 14 lines · On Monday 16 Jan 2017 16:52:46 John Stultz wrote:

Wanted to re-send out v4 of this patch set, integrating some changes suggested by Laurent, for consideration for merging for v4.11

The first three patches are fixups that are hopefully straight forward, integrating feedback I got from Laurant, with minimal change from the previous versions.

The last three patches try to clean up and rework the EDID probing code, to avoid issues seen on HiKey. I've reworked these more significantly since v3 to address feedback from Laurent.

Thoughts and feedback would be appreciated!

Quoted 7 lines
Quoted 3 lines

For the whole series,

Tested-by Laurent Pinchart <[email protected]>

Queued to drm-misc-next. Updated the commit messages for patches 5 and 6 as suggested by Laurent.

Ah! Many thanks Archit! I was going to respin the patches tomorrow but you beat me to it. :)

The help is much appreciated!
-john
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel