From: Doug Anderson <dianders@chromium.org>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Jiri Kosina <jikos@kernel.org>,
Bjorn Andersson <andersson@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
Frank Rowand <frowand.list@gmail.com>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Neil Armstrong <neil.armstrong@linaro.org>,
Sam Ravnborg <sam@ravnborg.org>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <mripard@kernel.org>,
Thomas Zimmermann <tzimmermann@suse.de>,
dri-devel@lists.freedesktop.org,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
linux-input@vger.kernel.org, Daniel Vetter <daniel@ffwll.ch>,
linux-kernel@vger.kernel.org, hsinyi@google.com,
cros-qcom-dts-watchers@chromium.org, devicetree@vger.kernel.org,
yangcong5@huaqin.corp-partner.google.com,
linux-arm-msm@vger.kernel.org,
Chris Morgan <macroalpha82@gmail.com>
Subject: Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
Date: Thu, 8 Jun 2023 09:42:42 -0700 [thread overview]
Message-ID: <CAD=FV=W-fXpm4JCczrNgAS2M9u2VLd2jAkJvE9XJgQpvoE5rjA@mail.gmail.com> (raw)
In-Reply-To: <y3l4x3kv7jgog3miexati5wbveaynnryzqvj6sc4ul6625f2if@w7nqgojfavfw>
Hi,
On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
>
> On Jun 07 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.
>
> Generally speaking, I'm not that happy when we need to coordinate with
> other subsystems for bringing up resources...
Yeah, I'd agree that it's not amazingly elegant. Unfortunately, I
couldn't find any existing clean frameworks that would do what was
needed, which is (presumably) why this problem hasn't been solved
before. I could try to come up with a grand abstraction / new
framework, but that doesn't seem like a great choice either unless we
expect more users...
> Anyway, a remark inlined (at least):
>
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v2:
> > - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
> >
> > drivers/hid/i2c-hid/i2c-hid-core.c | 82 +++++++++++++++++++++++++++++-
> > 1 file changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> > index fa8a1ca43d7f..368db3ae612f 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > @@ -38,6 +38,8 @@
> > #include <linux/mutex.h>
> > #include <asm/unaligned.h>
> >
> > +#include <drm/drm_panel.h>
> > +
> > #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,34 @@ 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,
> > +};
>
> Can we make that above block at least behind a Kconfig?
>
> i2c-hid is often used for touchpads, and the notion of drm panel has
> nothing to do with them. So I'd be more confident if we could disable
> that code if not required.
Happy to put it behind a Kconfig. I'll plan on that for v3. I'll stub
the functions out if there is no Kconfig, but plan to still leave
structure members just to avoid uglifying the sources too much.
> Actually, I'd be even more happier if it were in a different compilation
> unit. Not necessary a different module, but at least a different file.
I suspect that it's not worth it, but I'll do this if you feel
strongly about it.
I guess the simplest way I can think of to move this to its own file
would be to put the whole private data structure (struct i2c_hid) in a
private header file and then add prototypes for i2c_hid_core_resume()
and i2c_hid_core_suspend() there. Then I could add something like
i2c_hid_core_handle_panel_follower() that would have all the
registration logic. I'd still need special cases in the core
suspend/resume/remove code unless I add a level of abstraction. While
the level of abstraction is more "pure", it also would make the code
harder to follow.
Unless I hear a strong opinion (or if this series changes
significantly), I'll plan to keep things in the same file and just use
a Kconfig.
next prev parent reply other threads:[~2023-06-08 16:43 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens Douglas Anderson
2023-06-09 15:53 ` Krzysztof Kozlowski
2023-06-07 21:49 ` [PATCH v2 02/10] drm/panel: Check for already prepared/enabled in drm_panel Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 03/10] drm/panel: Add a way for other devices to follow panel state Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers Douglas Anderson
2023-06-09 16:10 ` Rob Herring
2023-06-07 21:49 ` [PATCH v2 05/10] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 06/10] HID: i2c-hid: Rearrange probe() to power things up later Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 07/10] HID: i2c-hid: Make suspend and resume into helper functions Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower Douglas Anderson
2023-06-08 5:18 ` kernel test robot
2023-06-08 7:14 ` kernel test robot
2023-06-08 15:10 ` Doug Anderson
2023-06-08 15:36 ` Benjamin Tissoires
2023-06-08 16:42 ` Doug Anderson [this message]
2023-06-09 9:27 ` Benjamin Tissoires
2023-06-09 15:01 ` Doug Anderson
2023-06-26 22:49 ` Doug Anderson
2023-07-17 18:15 ` Doug Anderson
2023-07-25 20:41 ` Doug Anderson
2023-07-26 8:07 ` Benjamin Tissoires
2023-06-07 21:49 ` [PATCH v2 09/10] HID: i2c-hid: Do panel follower work on the system_wq Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 10/10] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels Douglas Anderson
2023-06-08 7:17 ` [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Maxime Ripard
2023-06-08 14:38 ` Doug Anderson
2023-06-12 16:03 ` Maxime Ripard
2023-06-12 21:13 ` Doug Anderson
2023-06-13 12:06 ` Maxime Ripard
2023-06-13 15:56 ` Doug Anderson
2023-06-21 16:31 ` Doug Anderson
2023-06-23 9:08 ` Maxime Ripard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=FV=W-fXpm4JCczrNgAS2M9u2VLd2jAkJvE9XJgQpvoE5rjA@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=andersson@kernel.org \
--cc=benjamin.tissoires@redhat.com \
--cc=conor+dt@kernel.org \
--cc=cros-qcom-dts-watchers@chromium.org \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=frowand.list@gmail.com \
--cc=hsinyi@google.com \
--cc=jikos@kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=macroalpha82@gmail.com \
--cc=mripard@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=robh+dt@kernel.org \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
--cc=yangcong5@huaqin.corp-partner.google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).