Message-ID: <CAO-hwJ+3M1iYgaAFEtf-63U20ccGfdiRoi3197YoZmyvMYsGzQ@mail.gmail.com>
Date:   Fri, 9 Jun 2023 11:27:27 +0200
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        dri-devel@...ts.freedesktop.org,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-input@...r.kernel.org, Daniel Vetter <daniel@...ll.ch>,
        linux-kernel@...r.kernel.org, hsinyi@...gle.com,
        cros-qcom-dts-watchers@...omium.org, devicetree@...r.kernel.org,
        yangcong5@...qin.corp-partner.google.com,
        linux-arm-msm@...r.kernel.org,
        Chris Morgan <macroalpha82@...il.com>
Subject: Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower

On Thu, Jun 8, 2023 at 6:43 PM Doug Anderson <dianders@...omium.org> wrote:
>
> Hi,
>
> On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires
> <benjamin.tissoires@...hat.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@...omium.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.
>

Right, a separate file might not be the best then :(

Do you envision this to be used on the ACPI side of i2c-hid? Because
if this is OF only, then maybe it would be interesting to put it there
(in i2c-hid-of.c), instead of having it in the core. IIRC i2c-hid-of
also has ways to set up/down regulators, so maybe it'll be better
there?

Cheers,
Benjamin

Powered by blists - more mailing lists