From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751475AbdAPWZX (ORCPT ); Mon, 16 Jan 2017 17:25:23 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:52677 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750865AbdAPWZU (ORCPT ); Mon, 16 Jan 2017 17:25:20 -0500 From: Laurent Pinchart To: John Stultz Cc: "dri-devel@lists.freedesktop.org" , lkml , Wolfram Sang Subject: Re: [PATCH 5/5 v3] drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID Date: Tue, 17 Jan 2017 00:25:31 +0200 Message-ID: <1668382.5d9jBvLc1l@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: References: <1483472502-16403-1-git-send-email-john.stultz@linaro.org> <1965576.h4kdXt2VDW@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi John, On Monday 16 Jan 2017 12:14:48 John Stultz wrote: > On Mon, Jan 16, 2017 at 8:03 AM, Laurent Pinchart wrote: > > On Tuesday 03 Jan 2017 11:41:42 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 somehow getting lost. Using the logic > >> in __adv7511_power_on/off() which syncs and dirtys the cache > >> avoids this issue. > >> > >> Thus this patch changes the EDID probing logic so that we > >> re-use the __adv7511_power_on/off() calls. > > > > regcache_sync() is quite costly as it will write a bunch of registers. > > Wouldn't it be more efficient to only write the registers that are needed > > for EDID access ? > > So yes, you've mentioned this concern before, and I did spend some > time to narrow which lost-register state (0x43 > - ADV7511_REG_EDID_I2C_ADDR) was causing the trouble with i2c > trasnfer errors I was seeing: > https://lkml.org/lkml/2016/11/22/677 > > However, I didn't get much feedback on that, and it seems (to me at > least) concerning that we are losing the underlying state of a > register in the cache, so just syncing that one register back to the > hardware might solve the issue I was seeing, but I worry what other > registers might also be out of sync. > > The comment above the regmap_sync in adv7511_power_on after all states: > "Most of the registers are reset during power down or when HPD is low." You're right that most registers will be out of sync. > So it seems like if we're setting the power down (and setting HPD in > cases where Archit had a patch to add HPD pulsing to the > adv7511_get_modes path), it seems reasonable to do the same > regmap_sync()? It would be if we had to keep the device powered up, but we're powering it down right after reading the EDID. I don't think there's a need to reconfigure it completely, only setting the registers needed to read the EDID should be enough. > But, I'm not really picky here, and I'm very open to other approaches > (including something like the patch in the link above) if you have > suggestions/preferences. I just want it to work reliably on my > hardware. :) > > And just so I can better understand it, can you explain some about the > impact of your efficiency concerns? I'm not too picky either :-) If we can't find a reliable way to read the EDID by just configuring the registers we need, we could go for a full reconfiguration. However, restoring the value of all cached registers will result in lots of I2C writes, which are time-consuming operations. EDID read would be sped up if we could avoid that. -- Regards, Laurent Pinchart