Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932930AbcKVRiW (ORCPT ); Tue, 22 Nov 2016 12:38:22 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:47812 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755328AbcKVRiV (ORCPT ); Tue, 22 Nov 2016 12:38:21 -0500 From: Laurent Pinchart To: John Stultz Cc: lkml , David Airlie , Archit Taneja , Wolfram Sang , Lars-Peter Clausen , "dri-devel@lists.freedesktop.org" Subject: Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally Date: Tue, 22 Nov 2016 19:38:37 +0200 Message-ID: <1661379.b8y5I25JKx@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: References: <1479775052-28194-1-git-send-email-john.stultz@linaro.org> <1961666.AZMl081OYz@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 Content-Length: 2140 Lines: 52 Hi John, 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: > @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511, > >> unsigned int count; > >> > >> /* Reading the EDID only works if the device is powered */ > >> > >> - if (!adv7511->powered) { > >> - 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; > >> - } > >> + if (!adv7511->powered) > >> + __adv7511_power_on(adv7511); > > > > The __adv7511_power_on() function does more than the above, in particular > > it performs an expensive regcache_sync() and calls adv7533_dsi_power_on() > > for the ADV7533. Don't those operations have side effects that are either > > not wanted or not needed here ? In any case this patch modifies the > > behaviour of the driver, which needs to be documented in the kernel > > message. > > So yes, while the adv7533 bits aren't needed in the internal function, > I'm finding the logic to pulse the HPD and the regcache_sync call seem > to be needed side effects, as without that logic, I get i2c_transfer() > errors in adv7511_get_edid_block(). Does this patch fix the problem without requiring the 200ms delay ? > 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 ? > Thanks so much for the review! -- Regards, Laurent Pinchart