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

Reply via email to