Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753966AbcK1SpJ (ORCPT ); Mon, 28 Nov 2016 13:45:09 -0500 Received: from mail-oi0-f53.google.com ([209.85.218.53]:34066 "EHLO mail-oi0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751539AbcK1SpB (ORCPT ); Mon, 28 Nov 2016 13:45:01 -0500 MIME-Version: 1.0 In-Reply-To: <1235475f-891d-ea13-4c8a-dcbe21c57a47@codeaurora.org> References: <1479775052-28194-1-git-send-email-john.stultz@linaro.org> <1961666.AZMl081OYz@avalon> <1661379.b8y5I25JKx@avalon> <1235475f-891d-ea13-4c8a-dcbe21c57a47@codeaurora.org> From: John Stultz Date: Mon, 28 Nov 2016 10:44:59 -0800 Message-ID: Subject: Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally To: Archit Taneja Cc: Laurent Pinchart , lkml , David Airlie , Wolfram Sang , Lars-Peter Clausen , "dri-devel@lists.freedesktop.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3682 Lines: 93 On Tue, Nov 22, 2016 at 7:50 PM, Archit Taneja wrote: > On 11/23/2016 01:16 AM, John Stultz wrote: >> On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart >> wrote: >>> On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote: >>>> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote: >>>>> On Monday 21 Nov 2016 16:37:30 John Stultz wrote: >>>> >>>> I'll try to rework this patch to split the two changes of reworking >>>> the power_on/off function to be re-used (with no logic chage), and the >>>> patch to reuse it in get_modes() which resolves a bug. >>> >>> >>> Have you identified which register write fixes your problem here ? >> >> >> So I basically used regmap_sync_region() to bisect through the >> registers to try to figure out which one calling sync on helps avoid >> the issue, and I've narrowed it down to 0x43 >> (ADV7511_REG_EDID_I2C_ADDR). > > > I guess if this register loses its state, then i2c errors are expected. > >> >> If instead of the proposed patch here, I use the following patch >> (copy/paste whitespace damage, apologies) seems to make things work >> reliably: >> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> index 8dba729..87403d7 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> >> @@ -555,14 +557,18 @@ static int adv7511_get_modes(struct adv7511 >> *adv7511, >> ADV7511_INT1_DDC_ERROR); >> } >> adv7511->current_edid_segment = -1; >> + regcache_sync_region(adv7511->regmap, 0x43, 0x43); > > > So, we're losing register state when get_modes() is called, or sometime > before it. > Could you try to read a register with a known programmed value at the > beginning of > adv7511_get_modes(), and check if it has already lost the state or not? > > It's also possible that, in adv7511_get_modes(), when the chip is powered > on, i.e, > when we call: > > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, > ADV7511_POWER_POWER_DOWN, 0); > > the monitor wakes up, but there is some additional hpd toggling, which > results > in registers to lose their state. > > The patch below is something that was originally there in Lars's initial > patch > for ADV7533 support. I'd dropped it since it didn't have much to do with > ADV7533 > itself. It should at least discard any HPD toggling caused by powering on > the > chip in the next line. > > > diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c > index ed6d25d..5ee40a4 100644 > --- a/drivers/gpu/drm/i2c/adv7511.c > +++ b/drivers/gpu/drm/i2c/adv7511.c > @@ -654,6 +654,9 @@ static int adv7511_get_modes(struct adv7511 *adv7511, > > /* Reading the EDID only works if the device is powered */ > if (!adv7511->powered) { > + regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, > + ADV7511_REG_POWER2_HPD_SRC_MASK, > + ADV7511_REG_POWER2_HPD_SRC_NONE); > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, > ADV7511_POWER_POWER_DOWN, 0); > if (adv7511->i2c_main->irq) { So this patch is what got me started on the re-using the adv7511_power_on() logic, since it already had the bit to pulse the HPD signal. It might be helpful, but seems like a separate issue from the register state being lost. Unless I'm just not getting at something more subtle that you're suggesting. thanks -john