Message ID | 1425302380-24452-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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.
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);