Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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().

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.

Thanks so much for the review!
-john