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=unavailable 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 123BBC43460 for ; Fri, 7 May 2021 12:15:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CE28C6144F for ; Fri, 7 May 2021 12:15:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236194AbhEGMQc (ORCPT ); Fri, 7 May 2021 08:16:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54990 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229637AbhEGMQb (ORCPT ); Fri, 7 May 2021 08:16:31 -0400 Received: from mail-io1-xd31.google.com (mail-io1-xd31.google.com [IPv6:2607:f8b0:4864:20::d31]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DA8A8C061574 for ; Fri, 7 May 2021 05:15:30 -0700 (PDT) Received: by mail-io1-xd31.google.com with SMTP id b10so7754410iot.4 for ; Fri, 07 May 2021 05:15:30 -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=r1YFZyFjDLwrVH+Ob+dxqW0DeyKhQmY7WFAaaDnzzFk=; b=bejFVwvYv45tz+n/Sm8svgpe76q3nacN1Kph1PygLvds0gYad90yUApikHa+MvnsVb mUCaqrtqmfqwNDi4oNgkjY67AFxg+6IL+u5FyzeB4jI/smy/j9VYzfyhP/gVUeYqau59 SGSNZKePuvYM/pW0TnKHuzu0xrC5LVO9Du6aU= 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=r1YFZyFjDLwrVH+Ob+dxqW0DeyKhQmY7WFAaaDnzzFk=; b=MX/0cOl2pl4LPnq+qb5qTj05kq/qivCfrn1x3XGlMnVR6K5E5agJQYx9fwapbhmxi5 5GnHMogtxCcEPHLsFIda8/GIsG7tnoa9LcjtcerqjYez7mZ1am5iXwmKHrZaQIGEYcct rNckbARlc2B/0hMBvqq+BN+OjE9g2STEZAl1S4UFWyyx9BC9RveoB0B07T/comrZICcZ uZAi9rY2muiZWktBEjUT2XtzOK6UHOvtigW70ZP0ipuHx8KpCfRJrxEQo97RqRHWo+sS dXj3zA1cOrroGHL2itxcSaqrBywUb+PYDpjHe9SnvaFfq5z40eSxUpLFQs9SoZI5UoIx ZATQ== X-Gm-Message-State: AOAM532XRKWdilPfivdMJF30GZQ9+kOKF0Eu/ofROWmHPDaCiVKJR4Ot 7flBi1r6wT00+5kv2W4fx67AalFid894ORf+M+sIcQ== X-Google-Smtp-Source: ABdhPJwOwL/q0WMh5FPyGz9VBIgi7sE+gus3NpRZsSKeRvAKXssUGQR7gZKaswMQjYLWzrIIEDbQ6ZKyf0hG/1RPP9I= X-Received: by 2002:a05:6638:3398:: with SMTP id h24mr199781jav.114.1620389730110; Fri, 07 May 2021 05:15:30 -0700 (PDT) MIME-Version: 1.0 References: <20210507064444.402829-1-pihsun@chromium.org> In-Reply-To: <20210507064444.402829-1-pihsun@chromium.org> From: Hsin-Yi Wang Date: Fri, 7 May 2021 20:15:04 +0800 Message-ID: Subject: Re: [PATCH v2 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 Fri, May 7, 2021 at 2:44 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 > --- > drivers/gpu/drm/bridge/analogix/anx7625.c | 148 +++++++++------------- > drivers/gpu/drm/bridge/analogix/anx7625.h | 1 - > 2 files changed, 63 insertions(+), 86 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c > index 23283ba0c4f9..f56f8cf1f3bd 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) > +}; > + .pm = &anx7625_pm_ops, is missing in static struct i2c_driver anx7625_driver{...} > 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); > 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 >