From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2C609C433ED for ; Tue, 11 May 2021 04:34:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DFAA0615FF for ; Tue, 11 May 2021 04:34:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229968AbhEKEf0 (ORCPT ); Tue, 11 May 2021 00:35:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55772 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229548AbhEKEfZ (ORCPT ); Tue, 11 May 2021 00:35:25 -0400 Received: from mail-il1-x133.google.com (mail-il1-x133.google.com [IPv6:2607:f8b0:4864:20::133]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EFB54C061574 for ; Mon, 10 May 2021 21:34:19 -0700 (PDT) Received: by mail-il1-x133.google.com with SMTP id h6so16055461ila.7 for ; Mon, 10 May 2021 21:34:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=A/I5ZD1ncQP/PVS3uc11/1MXi9/3q76SHXYkN5zPqw0=; b=ghLy5Axa6iaQdEV9c7VqkEki0fGb6F1DYlIdkvpQwAkpQ8htzUaSeejBZ2FI5XdX2h g2HtcmjNpOKF+I9qbJLVx36iT0VgImevWoQsChSiNh3FuakovnQ6Vqpo6fNY0ctQtjKp wmxXQw1USIb5CYFlYQ5LccHjNgD7f7w089HK0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=A/I5ZD1ncQP/PVS3uc11/1MXi9/3q76SHXYkN5zPqw0=; b=bNSa5OYreM2atavBCw429CR5sCRG2rgDpSMCnhNLHM36G5j1tsCUqIWZF2NAzPVpTv avlWX/fWLcxgBzuMVaPKI6V6jE/7dZtJWuG8UWfToV/ouIqlEysVKv1aK2dAzSt0zAhY tNPQM72gJAHJEKjx7JI8yHIPY6RyYI2B5SRtdu00fDM5Po52DfxSe64Qf9jh3jeW7kKF w5d+YRZi4jVXt6ipSudJjQmP6y58zHemZVntNoFNEel4/vr5yKucr/1Vmndbx3WDDK8t 4jFAAknEK/3M94pmUzooXHZKZoXYRYDy4kqdrDxGliOrsry9TyK1xbS6BNzn1ACMG2bW DLXA== X-Gm-Message-State: AOAM533xfFHFq5x4S11kUZEovVHv9+1AsO0IUrldgDo6Mtb2bMgVIsED KSWZoazju2pOzbIuvTcZwo4Yx5pdxXwrkByzsK/sxk9Xk0w= X-Google-Smtp-Source: ABdhPJy58zkkqM52Ut8rSf8mSX+TIkQeLn3FLxsEmYgPzlkJGmOZ2hedPCwAshnCMjHVeAEaK7tpvInA6+dMLEuTfXk= X-Received: by 2002:a05:6e02:20ce:: with SMTP id 14mr24698028ilq.102.1620707659286; Mon, 10 May 2021 21:34:19 -0700 (PDT) MIME-Version: 1.0 References: <20210510053125.1595659-1-pihsun@chromium.org> In-Reply-To: <20210510053125.1595659-1-pihsun@chromium.org> From: Hsin-Yi Wang Date: Tue, 11 May 2021 12:33:53 +0800 Message-ID: Subject: Re: [PATCH v3 1/2] drm/bridge: anx7625: refactor power control to use runtime PM framework To: Pi-Hsun Shih Cc: Tzung-Bi Shih , Andrzej Hajda , Neil Armstrong , Robert Foss , Laurent Pinchart , Jonas Karlman , Jernej Skrabec , David Airlie , Daniel Vetter , Xin Ji , Sam Ravnborg , "open list:DRM DRIVERS" , open list Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 10, 2021 at 1:31 PM Pi-Hsun Shih wrote: > > The driver originally use an atomic_t for keep track of the power > status, which makes the driver more complicated than needed, and has > some race condition as it's possible to have the power on and power off > sequence going at the same time. > > This patch remove the usage of the atomic_t power_status, and use the > kernel runtime power management framework instead. > > Signed-off-by: Pi-Hsun Shih Tested-by: Hsin-Yi Wang Tested on a mt8183 juniper device. > --- > > Changes from v2: > * Add missing .pm field to anx7625_driver. > > --- > drivers/gpu/drm/bridge/analogix/anx7625.c | 149 ++++++++++------------ > drivers/gpu/drm/bridge/analogix/anx7625.h | 1 - > 2 files changed, 64 insertions(+), 86 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c > index 23283ba0c4f9..e1bf31eafe22 100644 > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > @@ -11,6 +11,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1005,33 +1006,6 @@ static void anx7625_power_on_init(struct anx7625_data *ctx) > } > } > > -static void anx7625_chip_control(struct anx7625_data *ctx, int state) > -{ > - struct device *dev = &ctx->client->dev; > - > - DRM_DEV_DEBUG_DRIVER(dev, "before set, power_state(%d).\n", > - atomic_read(&ctx->power_status)); > - > - if (!ctx->pdata.low_power_mode) > - return; > - > - if (state) { > - atomic_inc(&ctx->power_status); > - if (atomic_read(&ctx->power_status) == 1) > - anx7625_power_on_init(ctx); > - } else { > - if (atomic_read(&ctx->power_status)) { > - atomic_dec(&ctx->power_status); > - > - if (atomic_read(&ctx->power_status) == 0) > - anx7625_power_standby(ctx); > - } > - } > - > - DRM_DEV_DEBUG_DRIVER(dev, "after set, power_state(%d).\n", > - atomic_read(&ctx->power_status)); > -} > - > static void anx7625_init_gpio(struct anx7625_data *platform) > { > struct device *dev = &platform->client->dev; > @@ -1061,9 +1035,6 @@ static void anx7625_stop_dp_work(struct anx7625_data *ctx) > ctx->hpd_status = 0; > ctx->hpd_high_cnt = 0; > ctx->display_timing_valid = 0; > - > - if (ctx->pdata.low_power_mode == 0) > - anx7625_disable_pd_protocol(ctx); > } > > static void anx7625_start_dp_work(struct anx7625_data *ctx) > @@ -1105,49 +1076,26 @@ static void anx7625_hpd_polling(struct anx7625_data *ctx) > int ret, val; > struct device *dev = &ctx->client->dev; > > - if (atomic_read(&ctx->power_status) != 1) { > - DRM_DEV_DEBUG_DRIVER(dev, "No need to poling HPD status.\n"); > - return; > - } > - > ret = readx_poll_timeout(anx7625_read_hpd_status_p0, > ctx, val, > ((val & HPD_STATUS) || (val < 0)), > 5000, > 5000 * 100); > if (ret) { > - DRM_DEV_ERROR(dev, "HPD polling timeout!\n"); > - } else { > - DRM_DEV_DEBUG_DRIVER(dev, "HPD raise up.\n"); > - anx7625_reg_write(ctx, ctx->i2c.tcpc_client, > - INTR_ALERT_1, 0xFF); > - anx7625_reg_write(ctx, ctx->i2c.rx_p0_client, > - INTERFACE_CHANGE_INT, 0); > + DRM_DEV_ERROR(dev, "no hpd.\n"); > + return; > } > > - anx7625_start_dp_work(ctx); > -} > - > -static void anx7625_disconnect_check(struct anx7625_data *ctx) > -{ > - if (atomic_read(&ctx->power_status) == 0) > - anx7625_stop_dp_work(ctx); > -} > - > -static void anx7625_low_power_mode_check(struct anx7625_data *ctx, > - int state) > -{ > - struct device *dev = &ctx->client->dev; > + DRM_DEV_DEBUG_DRIVER(dev, "system status: 0x%x. HPD raise up.\n", val); > + anx7625_reg_write(ctx, ctx->i2c.tcpc_client, > + INTR_ALERT_1, 0xFF); > + anx7625_reg_write(ctx, ctx->i2c.rx_p0_client, > + INTERFACE_CHANGE_INT, 0); > > - DRM_DEV_DEBUG_DRIVER(dev, "low power mode check, state(%d).\n", state); > + anx7625_start_dp_work(ctx); > > - if (ctx->pdata.low_power_mode) { > - anx7625_chip_control(ctx, state); > - if (state) > - anx7625_hpd_polling(ctx); > - else > - anx7625_disconnect_check(ctx); > - } > + if (!ctx->pdata.panel_bridge && ctx->bridge_attached) > + drm_helper_hpd_irq_event(ctx->bridge.dev); > } > > static void anx7625_remove_edid(struct anx7625_data *ctx) > @@ -1180,9 +1128,6 @@ static int anx7625_hpd_change_detect(struct anx7625_data *ctx) > int intr_vector, status; > struct device *dev = &ctx->client->dev; > > - DRM_DEV_DEBUG_DRIVER(dev, "power_status=%d\n", > - (u32)atomic_read(&ctx->power_status)); > - > status = anx7625_reg_write(ctx, ctx->i2c.tcpc_client, > INTR_ALERT_1, 0xFF); > if (status < 0) { > @@ -1228,22 +1173,25 @@ static void anx7625_work_func(struct work_struct *work) > struct anx7625_data, work); > > mutex_lock(&ctx->lock); > + > + if (pm_runtime_suspended(&ctx->client->dev)) > + goto unlock; > + > event = anx7625_hpd_change_detect(ctx); > - mutex_unlock(&ctx->lock); > if (event < 0) > - return; > + goto unlock; > > if (ctx->bridge_attached) > drm_helper_hpd_irq_event(ctx->bridge.dev); > + > +unlock: > + mutex_unlock(&ctx->lock); > } > > static irqreturn_t anx7625_intr_hpd_isr(int irq, void *data) > { > struct anx7625_data *ctx = (struct anx7625_data *)data; > > - if (atomic_read(&ctx->power_status) != 1) > - return IRQ_NONE; > - > queue_work(ctx->workqueue, &ctx->work); > > return IRQ_HANDLED; > @@ -1305,9 +1253,9 @@ static struct edid *anx7625_get_edid(struct anx7625_data *ctx) > return (struct edid *)edid; > } > > - anx7625_low_power_mode_check(ctx, 1); > + pm_runtime_get_sync(dev); > edid_num = sp_tx_edid_read(ctx, p_edid->edid_raw_data); > - anx7625_low_power_mode_check(ctx, 0); > + pm_runtime_put(dev); > > if (edid_num < 1) { > DRM_DEV_ERROR(dev, "Fail to read EDID: %d\n", edid_num); > @@ -1611,10 +1559,7 @@ static void anx7625_bridge_enable(struct drm_bridge *bridge) > > DRM_DEV_DEBUG_DRIVER(dev, "drm enable\n"); > > - anx7625_low_power_mode_check(ctx, 1); > - > - if (WARN_ON(!atomic_read(&ctx->power_status))) > - return; > + pm_runtime_get_sync(dev); > > anx7625_dp_start(ctx); > } > @@ -1624,14 +1569,11 @@ static void anx7625_bridge_disable(struct drm_bridge *bridge) > struct anx7625_data *ctx = bridge_to_anx7625(bridge); > struct device *dev = &ctx->client->dev; > > - if (WARN_ON(!atomic_read(&ctx->power_status))) > - return; > - > DRM_DEV_DEBUG_DRIVER(dev, "drm disable\n"); > > anx7625_dp_stop(ctx); > > - anx7625_low_power_mode_check(ctx, 0); > + pm_runtime_put(dev); > } > > static enum drm_connector_status > @@ -1735,6 +1677,39 @@ static void anx7625_unregister_i2c_dummy_clients(struct anx7625_data *ctx) > i2c_unregister_device(ctx->i2c.tcpc_client); > } > > +static int __maybe_unused anx7625_runtime_pm_suspend(struct device *dev) > +{ > + struct anx7625_data *ctx = dev_get_drvdata(dev); > + > + mutex_lock(&ctx->lock); > + > + anx7625_stop_dp_work(ctx); > + anx7625_power_standby(ctx); > + > + mutex_unlock(&ctx->lock); > + > + return 0; > +} > + > +static int __maybe_unused anx7625_runtime_pm_resume(struct device *dev) > +{ > + struct anx7625_data *ctx = dev_get_drvdata(dev); > + > + mutex_lock(&ctx->lock); > + > + anx7625_power_on_init(ctx); > + anx7625_hpd_polling(ctx); > + > + mutex_unlock(&ctx->lock); > + > + return 0; > +} > + > +static const struct dev_pm_ops anx7625_pm_ops = { > + SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend, > + anx7625_runtime_pm_resume, NULL) > +}; > + > static int anx7625_i2c_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -1778,8 +1753,6 @@ static int anx7625_i2c_probe(struct i2c_client *client, > } > anx7625_init_gpio(platform); > > - atomic_set(&platform->power_status, 0); > - > mutex_init(&platform->lock); > > platform->pdata.intp_irq = client->irq; > @@ -1809,9 +1782,11 @@ static int anx7625_i2c_probe(struct i2c_client *client, > goto free_wq; > } > > - if (platform->pdata.low_power_mode == 0) { > + pm_runtime_enable(dev); > + > + if (!platform->pdata.low_power_mode) { > anx7625_disable_pd_protocol(platform); > - atomic_set(&platform->power_status, 1); > + pm_runtime_get_sync(dev); > } > > /* Add work function */ > @@ -1847,6 +1822,9 @@ static int anx7625_i2c_remove(struct i2c_client *client) > if (platform->pdata.intp_irq) > destroy_workqueue(platform->workqueue); > > + if (!platform->pdata.low_power_mode) > + pm_runtime_put_sync_suspend(&client->dev); > + > anx7625_unregister_i2c_dummy_clients(platform); > > kfree(platform); > @@ -1869,6 +1847,7 @@ static struct i2c_driver anx7625_driver = { > .driver = { > .name = "anx7625", > .of_match_table = anx_match_table, > + .pm = &anx7625_pm_ops, > }, > .probe = anx7625_i2c_probe, > .remove = anx7625_i2c_remove, > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h > index e4a086b3a3d7..034c3840028f 100644 > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h > @@ -369,7 +369,6 @@ struct anx7625_i2c_client { > > struct anx7625_data { > struct anx7625_platform_data pdata; > - atomic_t power_status; > int hpd_status; > int hpd_high_cnt; > /* Lock for work queue */ > > base-commit: e48661230cc35b3d0f4367eddfc19f86463ab917 > -- > 2.31.1.607.g51e8a6a459-goog >