[v3,08/10] HID: i2c-hid: Support being a panel follower

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 <dianders@chromium.org>
---

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(-)

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 <dianders@chromium.org>
> ---
> 
> 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 <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,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

Hi,

On Wed, Jul 26, 2023 at 1:57 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> > @@ -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.

It's not too hard to change this if we're sure we want to. I could
change how the panel follower API works, though I'd rather keep it how
it is now for symmetry. Thus, if we want to do this I'd probably just
set a boolean at the beginning of i2c_hid_core_remove() to avoid the
suspend when the panel follower API calls us back.

That being said, are you sure you want me to do that?

1. My patch doesn't change the behavior of any existing hardware. It
will only do anything for hardware that indicates it needs the panel
follower logic. Presumably these people could confirm that the logic
is OK for them, though I'll also admit that it's likely not many of
them will test the remove() case.

2. Can you give more details about why you say that the function will
fail? The first thing that the remove() function will do is to
unfollow the panel and that can cause the suspend to happen. At the
time this code runs all the normal communications should work and so
there should be no problems calling into the suspend code.

3. You can correct me if I'm wrong, but I'd actually argue that
calling the suspend code during remove actually fixes issues and we
should probably do it for the non-panel-follower case as well. I think
there are at least two benefits. One benefit is that if the i2c-hid
device is on a power rail that can't turn off (either an always-on or
a shared power rail) that we'll at least get the device in a low power
state before we stop managing it with this driver. The second benefit
is that it implicitly disables the interrupt and that fixes a
potential crash at remove time(). The crash in the old code I'm
imagining is:

a) i2c_hid_core_remove() is called.

b) We try to power down the i2c hid device, which might not do
anything if the device is on an always-on rail.

c) We call hid_destroy_device(), which frees the hid device.

d) An interrupt comes in before the call to free_irq() and we try to
dispatch it to the already freed hid device and crash.


If you agree that my reasoning makes sense, I can add a separate patch
before this one to suspend during remove.



-Doug

On Jul 26 2023, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jul 26, 2023 at 1:57 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> >
> > > @@ -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.
> 
> It's not too hard to change this if we're sure we want to. I could
> change how the panel follower API works, though I'd rather keep it how
> it is now for symmetry. Thus, if we want to do this I'd probably just
> set a boolean at the beginning of i2c_hid_core_remove() to avoid the
> suspend when the panel follower API calls us back.

I was more thinking on a boolean. No need to overload the API.

> 
> That being said, are you sure you want me to do that?
> 
> 1. My patch doesn't change the behavior of any existing hardware. It
> will only do anything for hardware that indicates it needs the panel
> follower logic. Presumably these people could confirm that the logic
> is OK for them, though I'll also admit that it's likely not many of
> them will test the remove() case.

Isn't trogdor (patch 10/10) already supported? Though you should be the
one making tests, so it should be fine ;)

> 
> 2. Can you give more details about why you say that the function will
> fail? The first thing that the remove() function will do is to
> unfollow the panel and that can cause the suspend to happen. At the
> time this code runs all the normal communications should work and so
> there should be no problems calling into the suspend code.

Now that I think about it more, maybe I am too biased by USB where the
device remove would happened *after* the device has been physically
unplugged. And this doesn't apply of course in the I2C world.

> 
> 3. You can correct me if I'm wrong, but I'd actually argue that
> calling the suspend code during remove actually fixes issues and we
> should probably do it for the non-panel-follower case as well. I think
> there are at least two benefits. One benefit is that if the i2c-hid
> device is on a power rail that can't turn off (either an always-on or
> a shared power rail) that we'll at least get the device in a low power
> state before we stop managing it with this driver. The second benefit
> is that it implicitly disables the interrupt and that fixes a
> potential crash at remove time(). The crash in the old code I'm
> imagining is:
> 
> a) i2c_hid_core_remove() is called.
> 
> b) We try to power down the i2c hid device, which might not do
> anything if the device is on an always-on rail.
> 
> c) We call hid_destroy_device(), which frees the hid device.
> 
> d) An interrupt comes in before the call to free_irq() and we try to
> dispatch it to the already freed hid device and crash.
> 
> 
> If you agree that my reasoning makes sense, I can add a separate patch
> before this one to suspend during remove.

Yep, I agree with you :)

Adding a separate patch would be nice, yes. Thanks!

Cheers,
Benjamin