Hi, On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> 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@xxxxxxxxxxxx> > > --- > > > > 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.