| Message ID | 1451874827-2531-1-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } > >
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; > }
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
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?
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; }