Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower

From: Doug Anderson <dianders@chromium.org>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: dri-devel@lists.freedesktop.org,
	Chris Morgan <macroalpha82@gmail.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Sam Ravnborg <sam@ravnborg.org>,
	Frank Rowand <frowand.list@gmail.com>,
	linux-input@vger.kernel.org, hsinyi@google.com,
	devicetree@vger.kernel.org, Conor Dooley <conor+dt@kernel.org>,
	cros-qcom-dts-watchers@chromium.org,
	linux-arm-msm@vger.kernel.org,
	yangcong5@huaqin.corp-partner.google.com,
	Jiri Kosina <jikos@kernel.org>,
	Maxime Ripard <mripard@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	linux-kernel@vger.kernel.org,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Thomas Zimmermann <tzimmermann@suse.de>
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.