[PATCH] drm: adv7511: really enable interrupts for EDID detection

[PATCH] 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 the chip was powered down
and interrupts were used. Fix this and remove clearing the interrupt flags,
since they are cleared in POWER_DOWN mode anyhow (according to docs and my
tests).

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

With this patch, I can read EDID in power-down mode reliably with my Lager
board. Tried with and without interrupts with two different monitors. I think
this patch should go to stable.

Note that I could not yet read EDID with Magnus' Koelsch. However, the
registers show that the interrupt flag and interrupt enable flag are both
correctly set (they were not before this patch). I assume a pinmuxing problem
or similar.

 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] 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 the chip was powered down
and interrupts were used. Fix this and remove clearing the interrupt flags,
since they are cleared in POWER_DOWN mode anyhow (according to docs and my
tests).

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

With this patch, I can read EDID in power-down mode reliably with my Lager
board. Tried with and without interrupts with two different monitors. I think
this patch should go to stable.

Note that I could not yet read EDID with Magnus' Koelsch. However, the
registers show that the interrupt flag and interrupt enable flag are both
correctly set (they were not before this patch). I assume a pinmuxing problem
or similar.

 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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

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


> Note that I could not yet read EDID with Magnus' Koelsch.

This has been tackled as well now. The clock for the GPIO controller was
off, so no interrupt was passed through.


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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

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


> Note that I could not yet read EDID with Magnus' Koelsch.

This has been tackled as well now. The clock for the GPIO controller was
off, so no interrupt was passed through.


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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Magnus Damm]

Hi Wolfram,

On Wed, Nov 25, 2015 at 6:05 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> Note that I could not yet read EDID with Magnus' Koelsch.
>
> This has been tackled as well now. The clock for the GPIO controller was
> off, so no interrupt was passed through.

Thanks a lot for your efforts! When you have time, can you please show
me the patch that fixes that GPIO interrupt problem? I'm asking
because I may have ran into a similar issue on Gose or Alt.

Cheers,

/ magnus

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Magnus Damm]

Hi Wolfram,

On Wed, Nov 25, 2015 at 6:05 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> Note that I could not yet read EDID with Magnus' Koelsch.
>
> This has been tackled as well now. The clock for the GPIO controller was
> off, so no interrupt was passed through.

Thanks a lot for your efforts! When you have time, can you please show
me the patch that fixes that GPIO interrupt problem? I'm asking
because I may have ran into a similar issue on Gose or Alt.

Cheers,

/ magnus

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

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


> > This has been tackled as well now. The clock for the GPIO controller was
> > off, so no interrupt was passed through.
> 
> Thanks a lot for your efforts! When you have time, can you please show
> me the patch that fixes that GPIO interrupt problem? I'm asking
> because I may have ran into a similar issue on Gose or Alt.

For Koelsch, this simply meant I reverted Geert's clocks-off patch. But
there is a fundamental problem below: GPIO interrupts described with the
"interrupts" property in DT do not get the underlying GPIO requested.
And if noone else is using the GPIO block, its clock may be turned off.

I have an idea how to fix it, need to check priorities of the other
tasks, though.

BTW, the continous EDID read test on your board is currently at >360000
successful reads, a not a single failure :)


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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

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


> > This has been tackled as well now. The clock for the GPIO controller was
> > off, so no interrupt was passed through.
> 
> Thanks a lot for your efforts! When you have time, can you please show
> me the patch that fixes that GPIO interrupt problem? I'm asking
> because I may have ran into a similar issue on Gose or Alt.

For Koelsch, this simply meant I reverted Geert's clocks-off patch. But
there is a fundamental problem below: GPIO interrupts described with the
"interrupts" property in DT do not get the underlying GPIO requested.
And if noone else is using the GPIO block, its clock may be turned off.

I have an idea how to fix it, need to check priorities of the other
tasks, though.

BTW, the continous EDID read test on your board is currently at >360000
successful reads, a not a single failure :)


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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Magnus Damm]

Hi Wolfram,

On Wed, Nov 25, 2015 at 3:48 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > This has been tackled as well now. The clock for the GPIO controller was
>> > off, so no interrupt was passed through.
>>
>> Thanks a lot for your efforts! When you have time, can you please show
>> me the patch that fixes that GPIO interrupt problem? I'm asking
>> because I may have ran into a similar issue on Gose or Alt.
>
> For Koelsch, this simply meant I reverted Geert's clocks-off patch. But
> there is a fundamental problem below: GPIO interrupts described with the
> "interrupts" property in DT do not get the underlying GPIO requested.
> And if noone else is using the GPIO block, its clock may be turned off.
>
> I have an idea how to fix it, need to check priorities of the other
> tasks, though.

I guess you mean that the GPIO callbacks include Runtime PM handling
however for irq_chip Runtime PM may not be hooked up so the GPIO block
is in such case is not powered on / get clock enabled?

> BTW, the continous EDID read test on your board is currently at >360000
> successful reads, a not a single failure :)

Excellent, thank you!

/ magnus

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Magnus Damm]

Hi Wolfram,

On Wed, Nov 25, 2015 at 3:48 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > This has been tackled as well now. The clock for the GPIO controller was
>> > off, so no interrupt was passed through.
>>
>> Thanks a lot for your efforts! When you have time, can you please show
>> me the patch that fixes that GPIO interrupt problem? I'm asking
>> because I may have ran into a similar issue on Gose or Alt.
>
> For Koelsch, this simply meant I reverted Geert's clocks-off patch. But
> there is a fundamental problem below: GPIO interrupts described with the
> "interrupts" property in DT do not get the underlying GPIO requested.
> And if noone else is using the GPIO block, its clock may be turned off.
>
> I have an idea how to fix it, need to check priorities of the other
> tasks, though.

I guess you mean that the GPIO callbacks include Runtime PM handling
however for irq_chip Runtime PM may not be hooked up so the GPIO block
is in such case is not powered on / get clock enabled?

> BTW, the continous EDID read test on your board is currently at >360000
> successful reads, a not a single failure :)

Excellent, thank you!

/ magnus

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

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


> I guess you mean that the GPIO callbacks include Runtime PM handling
> however for irq_chip Runtime PM may not be hooked up so the GPIO block
> is in such case is not powered on / get clock enabled?

Yes. There is another drawback when GPIOs are not properly requested. It
is still possible to request them from userspace although a kernel
driver is using them. I am playing with the idea that the GPIO core
auto-requests GPIOs which are not already requested but still set up as
interrupts.


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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

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


> I guess you mean that the GPIO callbacks include Runtime PM handling
> however for irq_chip Runtime PM may not be hooked up so the GPIO block
> is in such case is not powered on / get clock enabled?

Yes. There is another drawback when GPIOs are not properly requested. It
is still possible to request them from userspace although a kernel
driver is using them. I am playing with the idea that the GPIO core
auto-requests GPIOs which are not already requested but still set up as
interrupts.


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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Lars-Peter Clausen]

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

On 11/25/2015 09:27 AM, Wolfram Sang wrote:
> 
>> I guess you mean that the GPIO callbacks include Runtime PM handling
>> however for irq_chip Runtime PM may not be hooked up so the GPIO block
>> is in such case is not powered on / get clock enabled?
> 
> Yes. There is another drawback when GPIOs are not properly requested. It
> is still possible to request them from userspace although a kernel
> driver is using them. I am playing with the idea that the GPIO core
> auto-requests GPIOs which are not already requested but still set up as
> interrupts.

I think the GPIO core already reserves the pins that are requested as IRQs.
See gpiochip_lock_as_irq().

As for PM see this discussion
http://lkml.iu.edu/hypermail/linux/kernel/1511.1/01645.html

- Lars



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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Lars-Peter Clausen]

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

On 11/25/2015 09:27 AM, Wolfram Sang wrote:
> 
>> I guess you mean that the GPIO callbacks include Runtime PM handling
>> however for irq_chip Runtime PM may not be hooked up so the GPIO block
>> is in such case is not powered on / get clock enabled?
> 
> Yes. There is another drawback when GPIOs are not properly requested. It
> is still possible to request them from userspace although a kernel
> driver is using them. I am playing with the idea that the GPIO core
> auto-requests GPIOs which are not already requested but still set up as
> interrupts.

I think the GPIO core already reserves the pins that are requested as IRQs.
See gpiochip_lock_as_irq().

As for PM see this discussion
http://lkml.iu.edu/hypermail/linux/kernel/1511.1/01645.html

- Lars



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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

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

On Thu, Nov 26, 2015 at 01:43:33PM +0100, Lars-Peter Clausen wrote:
> On 11/25/2015 09:27 AM, Wolfram Sang wrote:
> > 
> >> I guess you mean that the GPIO callbacks include Runtime PM handling
> >> however for irq_chip Runtime PM may not be hooked up so the GPIO block
> >> is in such case is not powered on / get clock enabled?
> > 
> > Yes. There is another drawback when GPIOs are not properly requested. It
> > is still possible to request them from userspace although a kernel
> > driver is using them. I am playing with the idea that the GPIO core
> > auto-requests GPIOs which are not already requested but still set up as
> > interrupts.
> 
> I think the GPIO core already reserves the pins that are requested as IRQs.
> See gpiochip_lock_as_irq().

I could still export them via sysfs. They were also not showing up in
/sys/kernel/debug/gpio.

> As for PM see this discussion
> http://lkml.iu.edu/hypermail/linux/kernel/1511.1/01645.html

Nice! Thanks for this pointer.


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

Re: [PATCH] drm: adv7511: really enable interrupts for EDID detection

[Wolfram Sang]

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

On Thu, Nov 26, 2015 at 01:43:33PM +0100, Lars-Peter Clausen wrote:
> On 11/25/2015 09:27 AM, Wolfram Sang wrote:
> > 
> >> I guess you mean that the GPIO callbacks include Runtime PM handling
> >> however for irq_chip Runtime PM may not be hooked up so the GPIO block
> >> is in such case is not powered on / get clock enabled?
> > 
> > Yes. There is another drawback when GPIOs are not properly requested. It
> > is still possible to request them from userspace although a kernel
> > driver is using them. I am playing with the idea that the GPIO core
> > auto-requests GPIOs which are not already requested but still set up as
> > interrupts.
> 
> I think the GPIO core already reserves the pins that are requested as IRQs.
> See gpiochip_lock_as_irq().

I could still export them via sysfs. They were also not showing up in
/sys/kernel/debug/gpio.

> As for PM see this discussion
> http://lkml.iu.edu/hypermail/linux/kernel/1511.1/01645.html

Nice! Thanks for this pointer.


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