[PATCH] drm: adv7511: Refactor power management

[Chris Kohn]

Hi Laurent,

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart+renesas at ideasonboard.com]
> Sent: Monday, March 02, 2015 5:20 AM
> To: dri-devel at lists.freedesktop.org
> Cc: Lars-Peter Clausen; Chris Kohn; Hyun Kwon
> Subject: [PATCH] drm: adv7511: Refactor power management
> 
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Remove the internal dependency on DPMS mode for power management by
> using a by a powered state boolean instead, and use the new power off handler
> at probe time. This ensure that the regmap cache is properly marked as dirty
> when the device is probed, and the registers properly synced during the first
> power up.
> 
> As a side effect this removes the initialization of current_edid_segment at probe
> time, as the field will be initialized when the device is powered on, at the latest
> right before reading EDID data.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Works fine on my platform.

Tested-by: Christian Kohn <christian.kohn at xilinx.com>

Cheers,
Chris

> ---
>  drivers/gpu/drm/i2c/adv7511.c | 97 +++++++++++++++++++++++------------------
> --
>  1 file changed, 51 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
> index fa140e04d5fa..7fb7e22f4ad1 100644
> --- a/drivers/gpu/drm/i2c/adv7511.c
> +++ b/drivers/gpu/drm/i2c/adv7511.c
> @@ -27,7 +27,7 @@ struct adv7511 {
>  	struct regmap *regmap;
>  	struct regmap *packet_memory_regmap;
>  	enum drm_connector_status status;
> -	int dpms_mode;
> +	bool powered;
> 
>  	unsigned int f_tmds;
> 
> @@ -357,6 +357,46 @@ static void adv7511_set_link_config(struct adv7511
> *adv7511,
>  	adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
> }
> 
> +static void adv7511_power_on(struct adv7511 *adv7511) {
> +	adv7511->current_edid_segment = -1;
> +
> +	regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
> +		     ADV7511_INT0_EDID_READY |
> ADV7511_INT1_DDC_ERROR);
> +	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> +			   ADV7511_POWER_POWER_DOWN, 0);
> +
> +	/*
> +	 * Per spec it is allowed to pulse the HDP signal to indicate that the
> +	 * EDID information has changed. Some monitors do this when they
> wakeup
> +	 * from standby or are enabled. When the HDP goes low the adv7511 is
> +	 * reset and the outputs are disabled which might cause the monitor to
> +	 * go to standby again. To avoid this we ignore the HDP pin for the
> +	 * first few seconds after enabling the output.
> +	 */
> +	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
> +			   ADV7511_REG_POWER2_HDP_SRC_MASK,
> +			   ADV7511_REG_POWER2_HDP_SRC_NONE);
> +
> +	/*
> +	 * Most of the registers are reset during power down or when HPD is
> low.
> +	 */
> +	regcache_sync(adv7511->regmap);
> +
> +	adv7511->powered = true;
> +}
> +
> +static void adv7511_power_off(struct adv7511 *adv7511) {
> +	/* TODO: setup additional power down modes */
> +	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> +			   ADV7511_POWER_POWER_DOWN,
> +			   ADV7511_POWER_POWER_DOWN);
> +	regcache_mark_dirty(adv7511->regmap);
> +
> +	adv7511->powered = false;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Interrupt and hotplug detection
>   */
> @@ -526,7 +566,7 @@ static int adv7511_get_modes(struct drm_encoder
> *encoder,
>  	unsigned int count;
> 
>  	/* Reading the EDID only works if the device is powered */
> -	if (adv7511->dpms_mode != DRM_MODE_DPMS_ON) {
> +	if (!adv7511->powered) {
>  		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
>  			     ADV7511_INT0_EDID_READY |
> ADV7511_INT1_DDC_ERROR);
>  		regmap_update_bits(adv7511->regmap,
> ADV7511_REG_POWER, @@ -536,7 +576,7 @@ static int
> adv7511_get_modes(struct drm_encoder *encoder,
> 
>  	edid = drm_do_get_edid(connector, adv7511_get_edid_block,
> adv7511);
> 
> -	if (adv7511->dpms_mode != DRM_MODE_DPMS_ON)
> +	if (!adv7511->powered)
>  		regmap_update_bits(adv7511->regmap,
> ADV7511_REG_POWER,
>  				   ADV7511_POWER_POWER_DOWN,
>  				   ADV7511_POWER_POWER_DOWN);
> @@ -558,41 +598,10 @@ static void adv7511_encoder_dpms(struct
> drm_encoder *encoder, int mode)  {
>  	struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> 
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		adv7511->current_edid_segment = -1;
> -
> -		regmap_write(adv7511->regmap, ADV7511_REG_INT(0),
> -			     ADV7511_INT0_EDID_READY |
> ADV7511_INT1_DDC_ERROR);
> -		regmap_update_bits(adv7511->regmap,
> ADV7511_REG_POWER,
> -				   ADV7511_POWER_POWER_DOWN, 0);
> -		/*
> -		 * Per spec it is allowed to pulse the HDP signal to indicate
> -		 * that the EDID information has changed. Some monitors do
> this
> -		 * when they wakeup from standby or are enabled. When the
> HDP
> -		 * goes low the adv7511 is reset and the outputs are disabled
> -		 * which might cause the monitor to go to standby again. To
> -		 * avoid this we ignore the HDP pin for the first few seconds
> -		 * after enabeling the output.
> -		 */
> -		regmap_update_bits(adv7511->regmap,
> ADV7511_REG_POWER2,
> -				   ADV7511_REG_POWER2_HDP_SRC_MASK,
> -				   ADV7511_REG_POWER2_HDP_SRC_NONE);
> -		/* Most of the registers are reset during power down or
> -		 * when HPD is low
> -		 */
> -		regcache_sync(adv7511->regmap);
> -		break;
> -	default:
> -		/* TODO: setup additional power down modes */
> -		regmap_update_bits(adv7511->regmap,
> ADV7511_REG_POWER,
> -				   ADV7511_POWER_POWER_DOWN,
> -				   ADV7511_POWER_POWER_DOWN);
> -		regcache_mark_dirty(adv7511->regmap);
> -		break;
> -	}
> -
> -	adv7511->dpms_mode = mode;
> +	if (mode == DRM_MODE_DPMS_ON)
> +		adv7511_power_on(adv7511);
> +	else
> +		adv7511_power_off(adv7511);
>  }
> 
>  static enum drm_connector_status
> @@ -620,10 +629,9 @@ adv7511_encoder_detect(struct drm_encoder
> *encoder,
>  	 * there is a pending HPD interrupt and the cable is connected there was
>  	 * at least one transition from disconnected to connected and the chip
>  	 * has to be reinitialized. */
> -	if (status == connector_status_connected && hpd &&
> -	    adv7511->dpms_mode == DRM_MODE_DPMS_ON) {
> +	if (status == connector_status_connected && hpd && adv7511-
> >powered) {
>  		regcache_mark_dirty(adv7511->regmap);
> -		adv7511_encoder_dpms(encoder, adv7511->dpms_mode);
> +		adv7511_power_on(adv7511);
>  		adv7511_get_modes(encoder, connector);
>  		if (adv7511->status == connector_status_connected)
>  			status = connector_status_disconnected; @@ -858,7
> +866,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct
> i2c_device_id *id)
>  	if (!adv7511)
>  		return -ENOMEM;
> 
> -	adv7511->dpms_mode = DRM_MODE_DPMS_OFF;
> +	adv7511->powered = false;
>  	adv7511->status = connector_status_disconnected;
> 
>  	ret = adv7511_parse_dt(dev->of_node, &link_config); @@ -918,10
> +926,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct
> i2c_device_id *id)
>  	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL,
>  		     ADV7511_CEC_CTRL_POWER_DOWN);
> 
> -	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> -			   ADV7511_POWER_POWER_DOWN,
> ADV7511_POWER_POWER_DOWN);
> -
> -	adv7511->current_edid_segment = -1;
> +	adv7511_power_off(adv7511);
> 
>  	i2c_set_clientdata(i2c, adv7511);
> 
> --
> Regards,
> 
> Laurent Pinchart