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