Message ID | 20221003050335.1007931-2-treapking@chromium.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/bridge: it6505: Power management fixes for it6505 bridge | expand |
Il 03/10/22 07:03, Pin-yen Lin ha scritto: > Use pm_runtime_(get|put)_sync to control the bridge power, and add > SET_SYSTEM_SLEEP_PM_OPS with pm_runtime_force_(suspend|resume) to it6505 > driver. Without SET_SYSTEM_SLEEP_PM_OPS, the bridge will be powered on > unnecessarily when no external display is connected. > > Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver") > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > --- > > Changes in v2: > - Handle the error from pm_runtime_get_sync in it6505_extcon_work > > drivers/gpu/drm/bridge/ite-it6505.c | 33 +++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c > index 2bb957cffd94..685d8e750b12 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -421,6 +421,7 @@ struct it6505 { > struct notifier_block event_nb; > struct extcon_dev *extcon; > struct work_struct extcon_wq; > + int extcon_state; > enum drm_connector_status connector_status; > enum link_train_status link_state; > struct work_struct link_works; > @@ -2685,31 +2686,42 @@ static void it6505_extcon_work(struct work_struct *work) > { > struct it6505 *it6505 = container_of(work, struct it6505, extcon_wq); > struct device *dev = &it6505->client->dev; > - int state = extcon_get_state(it6505->extcon, EXTCON_DISP_DP); > - unsigned int pwroffretry = 0; > + int state, ret; > > if (it6505->enable_drv_hold) > return; > > mutex_lock(&it6505->extcon_lock); > > + state = extcon_get_state(it6505->extcon, EXTCON_DISP_DP); > DRM_DEV_DEBUG_DRIVER(dev, "EXTCON_DISP_DP = 0x%02x", state); > + > + if (state == it6505->extcon_state) > + goto unlock; Even if it's unlikely for anything bad to happen, please add error handling, or we might end up with unbalanced pm_runtime calls. if (state == it6505->extcon_state || unlikely(state < 0)) goto unlock; it6505->extcon_state = state; if (state) { .... } else { .... } Regards, Angelo
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c index 2bb957cffd94..685d8e750b12 100644 --- a/drivers/gpu/drm/bridge/ite-it6505.c +++ b/drivers/gpu/drm/bridge/ite-it6505.c @@ -421,6 +421,7 @@ struct it6505 { struct notifier_block event_nb; struct extcon_dev *extcon; struct work_struct extcon_wq; + int extcon_state; enum drm_connector_status connector_status; enum link_train_status link_state; struct work_struct link_works; @@ -2685,31 +2686,42 @@ static void it6505_extcon_work(struct work_struct *work) { struct it6505 *it6505 = container_of(work, struct it6505, extcon_wq); struct device *dev = &it6505->client->dev; - int state = extcon_get_state(it6505->extcon, EXTCON_DISP_DP); - unsigned int pwroffretry = 0; + int state, ret; if (it6505->enable_drv_hold) return; mutex_lock(&it6505->extcon_lock); + state = extcon_get_state(it6505->extcon, EXTCON_DISP_DP); DRM_DEV_DEBUG_DRIVER(dev, "EXTCON_DISP_DP = 0x%02x", state); + + if (state == it6505->extcon_state) + goto unlock; + if (state > 0) { DRM_DEV_DEBUG_DRIVER(dev, "start to power on"); msleep(100); - it6505_poweron(it6505); + ret = pm_runtime_get_sync(dev); + + /* + * On system resume, extcon_work can be triggered before + * pm_runtime_force_resume re-enables runtime power management. + * Handling the error here to make sure the bridge is powered on. + */ + if (ret) + it6505_poweron(it6505); } else { DRM_DEV_DEBUG_DRIVER(dev, "start to power off"); - while (it6505_poweroff(it6505) && pwroffretry++ < 5) { - DRM_DEV_DEBUG_DRIVER(dev, "power off fail %d times", - pwroffretry); - } + pm_runtime_put_sync(dev); drm_helper_hpd_irq_event(it6505->bridge.dev); memset(it6505->dpcd, 0, sizeof(it6505->dpcd)); DRM_DEV_DEBUG_DRIVER(dev, "power off it6505 success!"); } + it6505->extcon_state = state; +unlock: mutex_unlock(&it6505->extcon_lock); } @@ -3032,8 +3044,10 @@ static __maybe_unused int it6505_bridge_suspend(struct device *dev) return it6505_poweroff(it6505); } -static SIMPLE_DEV_PM_OPS(it6505_bridge_pm_ops, it6505_bridge_suspend, - it6505_bridge_resume); +static const struct dev_pm_ops it6505_bridge_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) + SET_RUNTIME_PM_OPS(it6505_bridge_suspend, it6505_bridge_resume, NULL) +}; static int it6505_init_pdata(struct it6505 *it6505) { @@ -3315,6 +3329,7 @@ static int it6505_i2c_probe(struct i2c_client *client, DRM_DEV_DEBUG_DRIVER(dev, "it6505 device name: %s", dev_name(dev)); debugfs_init(it6505); + pm_runtime_enable(dev); it6505->bridge.funcs = &it6505_bridge_funcs; it6505->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
Use pm_runtime_(get|put)_sync to control the bridge power, and add SET_SYSTEM_SLEEP_PM_OPS with pm_runtime_force_(suspend|resume) to it6505 driver. Without SET_SYSTEM_SLEEP_PM_OPS, the bridge will be powered on unnecessarily when no external display is connected. Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver") Signed-off-by: Pin-yen Lin <treapking@chromium.org> --- Changes in v2: - Handle the error from pm_runtime_get_sync in it6505_extcon_work drivers/gpu/drm/bridge/ite-it6505.c | 33 +++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 9 deletions(-)