HID: i2c-hid: Unfollow the panel on shutdown

[PATCH v2] HID: i2c-hid: Unfollow the panel on shutdown

Posted by Justin Yeh 2 weeks, 3 days ago

i2c_hid_core_shutdown() sends SET_POWER(SLEEP) even for panel followers,
but nothing in device_shutdown() guarantees whether the panel, and hence
our own power, is still on by the time we get there.

Do what i2c_hid_core_remove() does and just unfollow the panel.
drm_panel_remove_follower() calls panel_unpreparing() only while the
panel is still prepared, which powers us down properly; if it is already
unprepared no transfer is attempted at all. On MT8196 the panel is
unprepared long before us, so this also drops the ~2s i2c bus timeout
from every reboot.

Fixes: 96a37bfd232a ("HID: i2c-hid: Support being a panel follower")
Suggested-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Justin Yeh <justin.yeh@mediatek.com>
---
Changes in v2:
- Unfollow the panel instead of just skipping SET_POWER, so we no longer
  depend on the panel having been unprepared before us (Doug).
- Link to v1: https://lore.kernel.org/all/20260904035009.4114363-1-justin.yeh@mediatek.com/
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 0ff07fdab442..c7e46004f513 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1355,7 +1355,16 @@ void i2c_hid_core_shutdown(struct i2c_client *client)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 
-	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+	/*
+	 * Like in i2c_hid_core_remove(): 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);
+	else
+		i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+
 	free_irq(client->irq, ihid);
 
 	i2c_hid_core_shutdown_tail(ihid);
-- 
2.45.2

Re: [PATCH v2] HID: i2c-hid: Unfollow the panel on shutdown

Posted by Jiri Kosina 2 weeks ago

On Tue, 8 Sep 2026, Justin Yeh wrote:

> i2c_hid_core_shutdown() sends SET_POWER(SLEEP) even for panel followers,
> but nothing in device_shutdown() guarantees whether the panel, and hence
> our own power, is still on by the time we get there.
> 
> Do what i2c_hid_core_remove() does and just unfollow the panel.
> drm_panel_remove_follower() calls panel_unpreparing() only while the
> panel is still prepared, which powers us down properly; if it is already
> unprepared no transfer is attempted at all. On MT8196 the panel is
> unprepared long before us, so this also drops the ~2s i2c bus timeout
> from every reboot.
> 
> Fixes: 96a37bfd232a ("HID: i2c-hid: Support being a panel follower")
> Suggested-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Justin Yeh <justin.yeh@mediatek.com>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] HID: i2c-hid: Unfollow the panel on shutdown

Posted by Doug Anderson 2 weeks, 2 days ago

Hi,

On Mon, Sep 7, 2026 at 6:38 PM Justin Yeh <justin.yeh@mediatek.com> wrote:
>
> i2c_hid_core_shutdown() sends SET_POWER(SLEEP) even for panel followers,
> but nothing in device_shutdown() guarantees whether the panel, and hence
> our own power, is still on by the time we get there.
>
> Do what i2c_hid_core_remove() does and just unfollow the panel.
> drm_panel_remove_follower() calls panel_unpreparing() only while the
> panel is still prepared, which powers us down properly; if it is already
> unprepared no transfer is attempted at all. On MT8196 the panel is
> unprepared long before us, so this also drops the ~2s i2c bus timeout
> from every reboot.
>
> Fixes: 96a37bfd232a ("HID: i2c-hid: Support being a panel follower")
> Suggested-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Justin Yeh <justin.yeh@mediatek.com>
> ---
> Changes in v2:
> - Unfollow the panel instead of just skipping SET_POWER, so we no longer
>   depend on the panel having been unprepared before us (Doug).
> - Link to v1: https://lore.kernel.org/all/20260904035009.4114363-1-justin.yeh@mediatek.com/
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 0ff07fdab442..c7e46004f513 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1355,7 +1355,16 @@ void i2c_hid_core_shutdown(struct i2c_client *client)
>  {
>         struct i2c_hid *ihid = i2c_get_clientdata(client);
>
> -       i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
> +       /*
> +        * Like in i2c_hid_core_remove(): 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);
> +       else
> +               i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);

This seems right to me. One note is for panel followers if the panel
_hasn't_ been powered off by now, the drm_panel_remove_follower() call
will cause "i2c_hid_core_suspend(ihid, true)" to be called. That does
quite a bit more than just "i2c_hid_set_power(ihid,
I2C_HID_PWR_SLEEP)". ...but I think that's fine. I would expect most
systems would be powering off the panel at shutdown time anyway.

Reviewed-by: Douglas Anderson <dianders@chromium.org>