Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1550128 > unrolled thread

[PATCH 0/5 v3] adv7511 EDID probing improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5 v3] adv7511 EDID probing improvements John Stultz <john.stultz@linaro.org> - 2017-01-03 20:50 +0100
    [PATCH 3/5 v3] drm/bridge: adv7511: Enable HPD interrupts to support hotplug and improve monitor detection John Stultz <john.stultz@linaro.org> - 2017-01-03 20:50 +0100
    [PATCH 1/5 v3] drm/bridge: adv7511: Use work_struct to defer hotplug handing to out of irq context John Stultz <john.stultz@linaro.org> - 2017-01-03 20:50 +0100
    [PATCH 4/5 v3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally John Stultz <john.stultz@linaro.org> - 2017-01-03 20:50 +0100
      Re: [PATCH 4/5 v3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-01-16 17:00 +0100
    Re: [PATCH 0/5 v3] adv7511 EDID probing improvements Archit Taneja <architt@codeaurora.org> - 2017-01-11 09:50 +0100
      Re: [PATCH 0/5 v3] adv7511 EDID probing improvements John Stultz <john.stultz@linaro.org> - 2017-01-12 01:10 +0100
        Re: [PATCH 0/5 v3] adv7511 EDID probing improvements Archit Taneja <architt@codeaurora.org> - 2017-01-12 05:30 +0100
    Re: [PATCH 0/5 v3] adv7511 EDID probing improvements Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-01-16 17:00 +0100

#1550128 — [PATCH 0/5 v3] adv7511 EDID probing improvements

Hope everyone had a good newyears!

Wanted to re-send out v3 of this patch set improving the EDID
probing on the adv7511 used on HiKey, for consideration for
merging for 4.11

The first three patches are fixups that are hopefully straight
forward, integrating feedback I got from Laurant.

The last two patches try to clean up and resue code, which as
a side effect avoids an issue I'm seeing where something is
going wrong with the regmap cache state for the
ADV7511_REG_EDID_I2C_ADDR(0x43) register which results in
i2c_transfer errors if we don't do the
regcache_sync/_mark_dirty() calls. I suspect there might be a
better solution there, but have not gotten any other suggestions
so I wanted to go ahead and submit these.

Thoughts and feedback would be appreciated!

thanks
-john

New in v3:
* Addressed naming improvements and drm_kms_helper_hotplug_event
  usage corrections as suggested by Laurent.

Cc: David Airlie <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org

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

John Stultz (4):
  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

 drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 +
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 62 +++++++++++++++++++---------
 2 files changed, 44 insertions(+), 20 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1550129 — [PATCH 3/5 v3] drm/bridge: adv7511: Enable HPD interrupts to support hotplug and improve monitor detection

From: Archit Taneja <architt@codeaurora.org>

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 <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Originally-by: Archit Taneja <architt@codeaurora.org>
[jstultz: Added proper commit message]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index d93d66f..4b90975 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -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;
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1550133 — [PATCH 1/5 v3] 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 <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
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(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 992d76c..0396791 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -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;
 
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8dba729..4fcea44 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -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);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1550135 — [PATCH 4/5 v3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

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 <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 4b90975..dbdb71c 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -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;
 
@@ -359,24 +359,30 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 	 * Most of the registers are reset during power down or when HPD is low.
 	 */
 	regcache_sync(adv7511->regmap);
+}
 
+static void adv7511_power_on(struct adv7511 *adv7511)
+{
+	__adv7511_power_on(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;
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1559856 — Re: [PATCH 4/5 v3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

Hi John,

Thank you for the patch.

On Tuesday 03 Jan 2017 11:41:41 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.
> 
> Cc: David Airlie <airlied@linux.ie>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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

> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 4b90975..dbdb71c
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -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;
> 
> @@ -359,24 +359,30 @@ static void adv7511_power_on(struct adv7511 *adv7511)
>  	 * Most of the registers are reset during power down or when HPD is 
low.
>  	 */
>  	regcache_sync(adv7511->regmap);
> +}
> 
> +static void adv7511_power_on(struct adv7511 *adv7511)
> +{
> +	__adv7511_power_on(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;
>  }

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1556285

Hi,

On 01/04/2017 01:11 AM, John Stultz wrote:
> Hope everyone had a good newyears!
>
> Wanted to re-send out v3 of this patch set improving the EDID
> probing on the adv7511 used on HiKey, for consideration for
> merging for 4.11
>
> The first three patches are fixups that are hopefully straight
> forward, integrating feedback I got from Laurant.
>
> The last two patches try to clean up and resue code, which as
> a side effect avoids an issue I'm seeing where something is
> going wrong with the regmap cache state for the
> ADV7511_REG_EDID_I2C_ADDR(0x43) register which results in
> i2c_transfer errors if we don't do the
> regcache_sync/_mark_dirty() calls. I suspect there might be a
> better solution there, but have not gotten any other suggestions
> so I wanted to go ahead and submit these.
>
> Thoughts and feedback would be appreciated!

Tested on DB410c on 4.10-rc3. Works well for me.

Thanks,
Archit

>
> thanks
> -john
>
> New in v3:
> * Addressed naming improvements and drm_kms_helper_hotplug_event
>   usage corrections as suggested by Laurent.
>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
>
> Archit Taneja (1):
>   drm/bridge: adv7511: Enable HPD interrupts to support hotplug and
>     improve monitor detection
>
> John Stultz (4):
>   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
>
>  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 +
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 62 +++++++++++++++++++---------
>  2 files changed, 44 insertions(+), 20 deletions(-)
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1557019

On Wed, Jan 11, 2017 at 12:48 AM, Archit Taneja <architt@codeaurora.org> wrote:
> Hi,
>
> On 01/04/2017 01:11 AM, John Stultz wrote:
>>
>> Hope everyone had a good newyears!
>>
>> Wanted to re-send out v3 of this patch set improving the EDID
>> probing on the adv7511 used on HiKey, for consideration for
>> merging for 4.11
>>
>> The first three patches are fixups that are hopefully straight
>> forward, integrating feedback I got from Laurant.
>>
>> The last two patches try to clean up and resue code, which as
>> a side effect avoids an issue I'm seeing where something is
>> going wrong with the regmap cache state for the
>> ADV7511_REG_EDID_I2C_ADDR(0x43) register which results in
>> i2c_transfer errors if we don't do the
>> regcache_sync/_mark_dirty() calls. I suspect there might be a
>> better solution there, but have not gotten any other suggestions
>> so I wanted to go ahead and submit these.
>>
>> Thoughts and feedback would be appreciated!
>
>
> Tested on DB410c on 4.10-rc3. Works well for me.

Archit: Thanks for the testing! Is there anything else you need from
me (or others) to queue this for 4.11? Or should I be submitting it to
someone else?

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1557114

On 01/12/2017 05:36 AM, John Stultz wrote:
> On Wed, Jan 11, 2017 at 12:48 AM, Archit Taneja <architt@codeaurora.org> wrote:
>> Hi,
>>
>> On 01/04/2017 01:11 AM, John Stultz wrote:
>>>
>>> Hope everyone had a good newyears!
>>>
>>> Wanted to re-send out v3 of this patch set improving the EDID
>>> probing on the adv7511 used on HiKey, for consideration for
>>> merging for 4.11
>>>
>>> The first three patches are fixups that are hopefully straight
>>> forward, integrating feedback I got from Laurant.
>>>
>>> The last two patches try to clean up and resue code, which as
>>> a side effect avoids an issue I'm seeing where something is
>>> going wrong with the regmap cache state for the
>>> ADV7511_REG_EDID_I2C_ADDR(0x43) register which results in
>>> i2c_transfer errors if we don't do the
>>> regcache_sync/_mark_dirty() calls. I suspect there might be a
>>> better solution there, but have not gotten any other suggestions
>>> so I wanted to go ahead and submit these.
>>>
>>> Thoughts and feedback would be appreciated!
>>
>>
>> Tested on DB410c on 4.10-rc3. Works well for me.
>
> Archit: Thanks for the testing! Is there anything else you need from
> me (or others) to queue this for 4.11? Or should I be submitting it to
> someone else?

A final Ack/Test from Laurent for ADV7511 would be nice. I'll be the one
queuing it to drm-misc for 4.11.

Thanks,
Archit


>
> thanks
> -john
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1559858

Hi John,

Thank you for the patches.

On Tuesday 03 Jan 2017 11:41:37 John Stultz wrote:
> Hope everyone had a good newyears!
> 
> Wanted to re-send out v3 of this patch set improving the EDID
> probing on the adv7511 used on HiKey, for consideration for
> merging for 4.11
> 
> The first three patches are fixups that are hopefully straight
> forward, integrating feedback I got from Laurant.
> 
> The last two patches try to clean up and resue code, which as
> a side effect avoids an issue I'm seeing where something is
> going wrong with the regmap cache state for the
> ADV7511_REG_EDID_I2C_ADDR(0x43) register which results in
> i2c_transfer errors if we don't do the
> regcache_sync/_mark_dirty() calls. I suspect there might be a
> better solution there, but have not gotten any other suggestions
> so I wanted to go ahead and submit these.
> 
> Thoughts and feedback would be appreciated!

For the whole series,

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

> New in v3:
> * Addressed naming improvements and drm_kms_helper_hotplug_event
>   usage corrections as suggested by Laurent.
> 
> Cc: David Airlie <airlied@linux.ie>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> 
> Archit Taneja (1):
>   drm/bridge: adv7511: Enable HPD interrupts to support hotplug and
>     improve monitor detection
> 
> John Stultz (4):
>   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
> 
>  drivers/gpu/drm/bridge/adv7511/adv7511.h     |  2 +
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 62
> +++++++++++++++++++--------- 2 files changed, 44 insertions(+), 20
> deletions(-)

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web