Re: [RFC][PATCH] drm/bridge: adv7511: Re-write the i2c address as it may have been lost

[Laurent Pinchart]

Hi John,

Thank you for the patch.

On Monday 16 Jan 2017 15:16:51 John Stultz wrote:
> Laurent: Would something like the following be preferred? Seems
> to work as well for me..

That looks good to me. Feel free to still de-duplicate the power on/off code 
if you want (but of course without adding the regcache_sync to the common 
power on function this time).

Please see below for an additional comment.

> 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, likely as the device
> registers were reset during power off.
> 
> Thus this patch simply re-writes the i2c address to the
> ADV7511_REG_EDID_I2C_ADDR register to ensure its properly set
> before we try to read the EDID data.
> 
> Cc: David Airlie <airlied@xxxxxxxx>
> Cc: Archit Taneja <architt@xxxxxxxxxxxxxx>
> Cc: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 405e460..32c59cb
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -567,6 +567,8 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
> 
>  	/* Reading the EDID only works if the device is powered */
>  	if (!adv7511->powered) {
> +		unsigned int edid_i2c_addr = (adv7511->i2c_main->addr << 1) + 
4;
> +
>  		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>  				   ADV7511_POWER_POWER_DOWN, 0);
>  		if (adv7511->i2c_main->irq) {
> @@ -576,6 +578,9 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>  				     ADV7511_INT1_DDC_ERROR);
>  		}
>  		adv7511->current_edid_segment = -1;
> +
> +		/* Reset the EDID_I2C_ADDR register as it may have been 
cleared */
> +		regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, 
edid_i2c_addr);

As powering the device off called regcache_mark_dirty(), this will perform an 
I2C write and cache the value. If we then try to read the EDID a second time 
without going through a full power on/off sequence, I believe that regmap will 
skip the write the second time, as the cache will contain the same value and 
won't be marked as dirty. Should we call regcache_mark_dirty() when powering 
the device down further down this function ?

>  	}
> 
>  	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);

-- 
Regards,

Laurent Pinchart