[PATCH] drm: adv7511: Refactor power management

[PATCH] drm: adv7511: Refactor power management

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
---
 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm: adv7511: Refactor power management

[Chris Kohn]

Hi Laurent,

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart+renesas@ideasonboard.com]
> Sent: Monday, March 02, 2015 5:20 AM
> To: dri-devel@lists.freedesktop.org
> Cc: Lars-Peter Clausen; Chris Kohn; Hyun Kwon
> Subject: [PATCH] drm: adv7511: Refactor power management
> 
> From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>

Works fine on my platform.

Tested-by: Christian Kohn <christian.kohn@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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: adv7511: Refactor power management

[Lars-Peter Clausen]

On 03/02/2015 02:19 PM, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>

Tested-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Lars-Peter Clausen <lars@metafoo.de>

Thanks.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel