Received: by 2002:a05:6358:c692:b0:131:369:b2a3 with SMTP id fe18csp761915rwb; Wed, 26 Jul 2023 02:23:10 -0700 (PDT) X-Google-Smtp-Source: APBJJlHG+1iUJbgCJaUh0VHi5XE85R7nKLBaafAPpeNzZG62VwOgXGhlRAFd8v/PqRCqkHW8Q2Tq X-Received: by 2002:a17:906:53d9:b0:993:d536:3cb8 with SMTP id p25-20020a17090653d900b00993d5363cb8mr1227036ejo.2.1690363390066; Wed, 26 Jul 2023 02:23:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690363390; cv=none; d=google.com; s=arc-20160816; b=E1yRjTIQlUoGT0T0tcKS5HEdGH6+74FqtcsV0tFWvo8lJ9md6fTB/Ey6oXQgW0u7sj uX4KSVTpvZaM1ub8UdbUSn+N9SE64c0MgK23S5g7s2hGVnNFGnx6KWn38t6e26Rsx5c5 i+b7+kZUd2tzDP5TRltZbhz5ynZz5kU8aA43UmD1svtp/eyMMbgRUthM8GJ1gSRpeIZD XvzTxRnnYiFRNfpr6LF+syEkDWOm0iC3mJ5J6Zphk5mJL+NLf+K9rhEKpxBPz3klCFxH CbJ1K0ir+MB8kL0vGsy4F+osbXzYPtvg5L6p19M6qo2zzlCy+ew42Jcbvmtw7UN8yJye TnTQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=CL356SqE3Me/PJWJyaO8L6LB7jhiv/WeotjdOYfYqck=; fh=wWS6DSdMkHlqAxeEYx8tPy3P7SFRmFVc+ZX08jOG5j4=; b=YNzGU1Eb4rx8T0X/CFDLxENGHvUscJ5vDjhgic2StZygbJiHFKQ2M1B7fkjDdNOdGm lGYkhaSxZ6V8zKRhxZbgvVBw/htfhsOcb8i1bgyqsSkVOPdJoID4yUYUp1Xw4YySRQlu MpM2dpXUkDkXF24g+D2kHMvRCtSawoEQ9yYXX0EvJdv0fsPOz/PPY4ULkYgk2vXESLTA T17EeC7YdKkb6NnTwGRObtHLMkjf3kxv16XobnsMTBr74ImJGbQj348DbymBKWhrusHf DpRaKOgvLi5BIAxe7xiGTkpScLAsh8bm4F96vaOR33OCPCrmpowWPUP3oOnqwZot3IIb 5BvA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=fROJJIaB; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i8-20020a17090671c800b00988d6a7cdefsi9537595ejk.212.2023.07.26.02.22.44; Wed, 26 Jul 2023 02:23:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=fROJJIaB; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233243AbjGZJCo (ORCPT + 99 others); Wed, 26 Jul 2023 05:02:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57486 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232974AbjGZJCR (ORCPT ); Wed, 26 Jul 2023 05:02:17 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B233CE47; Wed, 26 Jul 2023 01:57:42 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 42C5C6191D; Wed, 26 Jul 2023 08:57:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1ADE5C433C8; Wed, 26 Jul 2023 08:57:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1690361861; bh=kjiQFIohojvIrF639adWnH/h/VJNwIVgZ5idr1q2NXU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fROJJIaB+DQoYOGdT7clpGBJe5sWA4dgcmlZ6PPjD9fSbVK79++e73RLoBLrm5GgL VnAXtQyoTTorp3Z4L1EILfqE75lUzu/8rWyScRJ2e6fbBbknQCvFvmFLEbCsPLqS9S 9CtKzfZcYOpmSunwgPsNbVqlf5bWA8u9/krqmW9KkA3ztsDz8r5LZnI1gutIXZOadu fuiPW/Zyg13o9K6+5twxyaQxNfF19ict/uV6aCffmXvLg5jiGYWXmvB8h8UtNzbU4E YA+NAOuYPkXJpGxqnyS0+/E/nW9V+w+z5Qstd6c9fixjvd4d2oNmuMQ/qX5rZfIKE3 ZpMvnkujKt7pg== Date: Wed, 26 Jul 2023 10:57:34 +0200 From: Benjamin Tissoires To: Douglas Anderson Cc: Jiri Kosina , Benjamin Tissoires , Bjorn Andersson , Konrad Dybcio , Rob Herring , Frank Rowand , Krzysztof Kozlowski , Conor Dooley , Neil Armstrong , Sam Ravnborg , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , devicetree@vger.kernel.org, cros-qcom-dts-watchers@chromium.org, linux-arm-msm@vger.kernel.org, yangcong5@huaqin.corp-partner.google.com, Dmitry Torokhov , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Chris Morgan , linux-input@vger.kernel.org, hsinyi@google.com Subject: Re: [PATCH v3 08/10] HID: i2c-hid: Support being a panel follower Message-ID: References: <20230725203545.2260506-1-dianders@chromium.org> <20230725133443.v3.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15@changeid> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230725133443.v3.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15@changeid> X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Jul 25 2023, Douglas Anderson wrote: > As talked about in the patch ("drm/panel: Add a way for other devices > to follow panel state"), we really want to keep the power states of a > touchscreen and the panel it's attached to in sync with each other. In > that spirit, add support to i2c-hid to be a panel follower. This will > let the i2c-hid driver get informed when the panel is powered on and > off. From there we can match the i2c-hid device's power state to that > of the panel. > > NOTE: this patch specifically _doesn't_ use pm_runtime to keep track > of / manage the power state of the i2c-hid device, even though my > first instinct said that would be the way to go. Specific problems > with using pm_runtime(): > * The initial power up couldn't happen in a runtime resume function > since it create sub-devices and, apparently, that's not good to do > in your resume function. > * Managing our power state with pm_runtime meant fighting to make the > right thing happen at system suspend to prevent the system from > trying to resume us only to suspend us again. While this might be > able to be solved, it added complexity. > Overall the code without pm_runtime() ended up being smaller and > easier to understand. > > Signed-off-by: Douglas Anderson > --- > > Changes in v3: > - Add "depends on DRM || !DRM" to Kconfig to avoid randconfig error. > - Split more of the panel follower code out of the core. > > Changes in v2: > - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static. > > drivers/hid/i2c-hid/Kconfig | 2 + > drivers/hid/i2c-hid/i2c-hid-core.c | 82 +++++++++++++++++++++++++++++- > 2 files changed, 82 insertions(+), 2 deletions(-) > > diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig > index 3be17109301a..2bdb55203104 100644 > --- a/drivers/hid/i2c-hid/Kconfig > +++ b/drivers/hid/i2c-hid/Kconfig > @@ -70,5 +70,7 @@ config I2C_HID_OF_GOODIX > > config I2C_HID_CORE > tristate > + # We need to call into panel code so if DRM=m, this can't be 'y' > + depends on DRM || !DRM > endif > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index fa8a1ca43d7f..fa6d1f624342 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -38,6 +38,8 @@ > #include > #include > > +#include > + > #include "../hid-ids.h" > #include "i2c-hid.h" > > @@ -107,6 +109,8 @@ struct i2c_hid { > struct mutex reset_lock; > > struct i2chid_ops *ops; > + struct drm_panel_follower panel_follower; > + bool is_panel_follower; > }; > > static const struct i2c_hid_quirks { > @@ -1058,6 +1062,59 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid) > return ret; > } > > +static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower) > +{ > + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower); > + struct hid_device *hid = ihid->hid; > + > + /* > + * hid->version is set on the first power up. If it's still zero then > + * this is the first power on so we should perform initial power up > + * steps. > + */ > + if (!hid->version) > + return i2c_hid_core_initial_power_up(ihid); > + > + return i2c_hid_core_resume(ihid); > +} > + > +static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower) > +{ > + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower); > + > + return i2c_hid_core_suspend(ihid); > +} > + > +static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = { > + .panel_prepared = i2c_hid_core_panel_prepared, > + .panel_unpreparing = i2c_hid_core_panel_unpreparing, > +}; > + > +static int i2c_hid_core_register_panel_follower(struct i2c_hid *ihid) > +{ > + struct device *dev = &ihid->client->dev; > + int ret; > + > + ihid->is_panel_follower = true; > + ihid->panel_follower.funcs = &i2c_hid_core_panel_follower_funcs; > + > + /* > + * If we're not in control of our own power up/power down then we can't > + * do the logic to manage wakeups. Give a warning if a user thought > + * that was possible then force the capability off. > + */ > + if (device_can_wakeup(dev)) { > + dev_warn(dev, "Can't wakeup if following panel\n"); > + device_set_wakeup_capable(dev, false); > + } > + > + ret = drm_panel_add_follower(dev, &ihid->panel_follower); > + if (ret) > + return ret; > + > + return 0; > +} > + > int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, > u16 hid_descriptor_address, u32 quirks) > { > @@ -1119,7 +1176,15 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops, > hid->bus = BUS_I2C; > hid->initial_quirks = quirks; > > - ret = i2c_hid_core_initial_power_up(ihid); > + /* > + * If we're a panel follower, we'll register and do our initial power > + * up when the panel turns on; otherwise we do it right away. > + */ > + if (drm_is_panel_follower(&client->dev)) > + ret = i2c_hid_core_register_panel_follower(ihid); > + else > + ret = i2c_hid_core_initial_power_up(ihid); nitpicks, but I'm not sure I'm a big fan of having "if (drm_is_panel_follower(&client->dev))" sprinkled everywhere in the generic probe/resume/suspend code. Would it be OK to define a `static int __do_i2c_hid_core_initial_power_up(struct i2c_hid *ihid)` that would do the actual powering up, and have i2c_hid_core_initial_power_up() doing the test if we are a panel follower? The i2c_hid_core_panel_* will need to be updated to use the `__do_` prefixed functions. > + > if (ret) > goto err_mem_free; > > @@ -1143,7 +1208,14 @@ void i2c_hid_core_remove(struct i2c_client *client) > struct i2c_hid *ihid = i2c_get_clientdata(client); > struct hid_device *hid; > > - i2c_hid_core_power_down(ihid); > + /* > + * If we're a follower, the act of unfollowing will cause us to be > + * powered down. Otherwise we need to manually do it. > + */ > + if (ihid->is_panel_follower) > + drm_panel_remove_follower(&ihid->panel_follower); That part is concerning, as we are now calling hid_drv->suspend() when removing the device. It might or not have an impact (I'm not sure of it), but we are effectively changing the path of commands sent to the device. hid-multitouch might call a feature in ->suspend, but the remove makes that the physical is actually disconnected, so the function will fail, and I'm not sure what is happening then. > + else > + i2c_hid_core_power_down(ihid); Same here, I *think* it would be best to have the `if (ihid->is_panel_follower)` test in i2c_hid_core_power_down() (and have a separate _do_i2c_hid_core_power_down()). > > hid = ihid->hid; > hid_destroy_device(hid); > @@ -1171,6 +1243,9 @@ static int i2c_hid_core_pm_suspend(struct device *dev) > struct i2c_client *client = to_i2c_client(dev); > struct i2c_hid *ihid = i2c_get_clientdata(client); > > + if (ihid->is_panel_follower) > + return 0; Not sure we need to split that one with _do_ prefix, it's already split :) > + > return i2c_hid_core_suspend(ihid); > } > > @@ -1179,6 +1254,9 @@ static int i2c_hid_core_pm_resume(struct device *dev) > struct i2c_client *client = to_i2c_client(dev); > struct i2c_hid *ihid = i2c_get_clientdata(client); > > + if (ihid->is_panel_follower) > + return 0; Same here, no need to split. > + > return i2c_hid_core_resume(ihid); > } > > -- > 2.41.0.487.g6d72f3e995-goog > Cheers, Benjamin