* [PATCH RESEND v4 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-05-24 3:42 UTC (permalink / raw)
To: Jiri Kosina
Cc: Rob Herring, Matthias Brugger, Jeff LaBundy, Douglas Anderson,
Fei Shao, Benjamin Tissoires, Dmitry Torokhov, Matthias Kaehlcke,
Stephen Kitt, linux-input, linux-kernel
In-Reply-To: <20230524114233.RESEND.v4.2.I424e840ae6de3cdbd67394cf4efd24534f6434ba@changeid>
In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
reset line to true state of the regulator") introduced a change to tie
the reset line of the Goodix touchscreen to the state of the regulator
to fix a power leakage issue in suspend.
After some time, the change was deemed unnecessary and was reverted in
commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
the regulator") due to difficulties in managing regulator notifiers for
designs like Evoker, which provides a second power rail to touchscreen.
However, the revert caused a power regression on another Chromebook
device Steelix in the field, which has a dedicated always-on regulator
for touchscreen and was covered by the workaround in the first commit.
To address both cases, this patch adds the support for the new
"goodix,no-reset-during-suspend" property in the driver:
- When set to true, the driver does not assert the reset GPIO during
power-down.
Instead, the GPIO will be asserted during power-up to ensure the
touchscreen always has a clean start and consistent behavior after
resuming.
This is for designs with a dedicated always-on regulator.
- When set to false or unset, the driver uses the original control flow
and asserts GPIO and disables regulators normally.
This is for the two-regulator and shared-regulator designs.
Signed-off-by: Fei Shao <fshao@chromium.org>
Suggested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v4:
- Minor coding style improvement
Changes in v3:
- In power-down, only skip the GPIO but not the regulator calls if the
flag is set
Changes in v2:
- Drop the change to regulator_enable logic during power-up
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index 0060e3dcd775..db4639db9840 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
struct regulator *vdd;
struct regulator *vddio;
struct gpio_desc *reset_gpio;
+ bool no_reset_during_suspend;
const struct goodix_i2c_hid_timing_data *timings;
};
@@ -37,6 +38,14 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
container_of(ops, struct i2c_hid_of_goodix, ops);
int ret;
+ /*
+ * We assert reset GPIO here (instead of during power-down) to ensure
+ * the device will have a clean state after powering up, just like the
+ * normal scenarios will have.
+ */
+ if (ihid_goodix->no_reset_during_suspend)
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+
ret = regulator_enable(ihid_goodix->vdd);
if (ret)
return ret;
@@ -60,7 +69,9 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
struct i2c_hid_of_goodix *ihid_goodix =
container_of(ops, struct i2c_hid_of_goodix, ops);
- gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+ if (!ihid_goodix->no_reset_during_suspend)
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+
regulator_disable(ihid_goodix->vddio);
regulator_disable(ihid_goodix->vdd);
}
@@ -91,6 +102,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
if (IS_ERR(ihid_goodix->vddio))
return PTR_ERR(ihid_goodix->vddio);
+ ihid_goodix->no_reset_during_suspend =
+ of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
+
ihid_goodix->timings = device_get_match_data(&client->dev);
return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH RESEND v4 1/2] dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-05-24 3:42 UTC (permalink / raw)
To: Jiri Kosina
Cc: Rob Herring, Matthias Brugger, Jeff LaBundy, Douglas Anderson,
Fei Shao, Rob Herring, Conor Dooley, Dmitry Torokhov,
Krzysztof Kozlowski, devicetree, linux-input, linux-kernel
In-Reply-To: <20230524114233.RESEND.v4.2.I424e840ae6de3cdbd67394cf4efd24534f6434ba@changeid>
We observed that on Chromebook device Steelix, if Goodix GT7375P
touchscreen is powered in suspend (because, for example, it connects to
an always-on regulator) and with the reset GPIO asserted, it will
introduce about 14mW power leakage.
To address that, we add this property to skip reset during suspend.
If it's set, the driver will stop asserting the reset GPIO during
power-down. Refer to the comments in the driver for details.
Signed-off-by: Fei Shao <fshao@chromium.org>
Suggested-by: Jeff LaBundy <jeff@labundy.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v4:
- Rebase on top of next-20230523
- Collect the review tags
Changes in v2:
- Use a more accurate property name and with "goodix," prefix
.../devicetree/bindings/input/goodix,gt7375p.yaml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
index ce18d7dadae2..1edad1da1196 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -43,6 +43,15 @@ properties:
itself as long as it allows the main board to make signals compatible
with what the touchscreen is expecting for its IO rails.
+ goodix,no-reset-during-suspend:
+ description:
+ Set this to true to enforce the driver to not assert the reset GPIO
+ during suspend.
+ Due to potential touchscreen hardware flaw, back-powering could happen in
+ suspend if the power supply is on and with active-low reset GPIO asserted.
+ This property is used to avoid the back-powering issue.
+ type: boolean
+
required:
- compatible
- reg
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH RESEND v4 0/2] Fix Goodix touchscreen power leakage for MT8186 boards
From: Fei Shao @ 2023-05-24 3:42 UTC (permalink / raw)
To: Jiri Kosina
Cc: Rob Herring, Matthias Brugger, Jeff LaBundy, Douglas Anderson,
Fei Shao, Benjamin Tissoires, Conor Dooley, Dmitry Torokhov,
Krzysztof Kozlowski, Matthias Kaehlcke, Stephen Kitt, devicetree,
linux-input, linux-kernel
These changes are based on the series in [1], which modified the
i2c-hid-of-goodix driver and removed the workaround for a power leakage
issue, so the issue revisits on Mediatek MT8186 boards (Steelix).
The root cause is that the touchscreen can be powered in different ways
depending on the hardware designs, and it's not as easy to come up with
a solution that is both simple and elegant for all the known designs.
To address the issue, I ended up adding a new boolean property for the
driver so that we can control the power up/down sequence depending on
that.
Adding a new property might not be the cleanest approach for this, but
at least the intention would be easy enough to understand, and it
introduces relatively small change to the code and fully preserves the
original control flow.
[1] https://lore.kernel.org/all/20230207024816.525938-1-dianders@chromium.org/
Changes in v4:
- Rebase on top of next-20230523
- Collect the review tags
- Minor coding style improvement
Changes in v3:
- In power-down, only skip the GPIO but not the regulator calls if the
flag is set
Changes in v2:
- Use a more accurate property name and with "goodix," prefix
- Drop the change to regulator_enable logic during power-up
Fei Shao (2):
dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend"
property
HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend"
property
.../bindings/input/goodix,gt7375p.yaml | 9 +++++++++
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply
* Re: [PATCH v4] Add rumble support to latest xbox controllers
From: Siarhei Vishniakou @ 2023-05-24 1:43 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel; +Cc: Bastien Nocera
In-Reply-To: <20230425163844.3711710-1-svv@google.com>
Friendly ping on this CL, thanks!
^ permalink raw reply
* Re: [PATCH 0/9] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Doug Anderson @ 2023-05-23 23:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
dri-devel, linux-input, hsinyi, devicetree, yangcong5,
linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers
In-Reply-To: <ZG0yjuNvhnircAxA@google.com>
Hi,
On Tue, May 23, 2023 at 2:39 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Doug,
>
> On Tue, May 23, 2023 at 12:27:54PM -0700, Douglas Anderson wrote:
> >
> > The big motivation for this patch series is mostly described in the patch
> > ("drm/panel: Add a way for other devices to follow panel state"), but to
> > quickly summarize here: for touchscreens that are connected to a panel we
> > need the ability to power sequence the two device together. This is not a
> > new need, but so far we've managed to get by through a combination of
> > inefficiency, added costs, or perhaps just a little bit of brokenness.
> > It's time to do better. This patch series allows us to do better.
>
> This seems to grow a new way of building relationship between panels and
> associated devices. Can we make device_link_*() work for us?
If you know of a way to make it work, that'd be great. ...but I don't
_think_ it would be that easy. I haven't spent much time with the
device_link APIs, though, so please correct me if I'm wrong.
I guess my main issue with device_link would be that that ordering
feels backward. Specifically, the device we're acting on (the one
we're turning off and on) is the panel. We typically turn the panel
off and on at times (during a modeset, when the lid is closed, or just
if the system is idle). When this happens we'd like to remove power
from both the panel and the touchscreen. Userspace is just not setup
to power off touchscreens in tandem with the panel and sometimes (like
in the case of a modeset) it might not even know it needs to. With
device_link I believe that the "child" device is in charge of powering
the parent. I think that would mean that to use device_link we'd need
to make the panel be the child of the touchscreen. Then, I guess we'd
tell the touchscreen not to power itself on if it had children and
just wait for a child to power it on? It feels really awkward partly
because the panel doesn't feel like it should be a child of the
touchscreen and partially because it seems weird that the touchscreen
would somehow need to know not to request power for itself when it has
a child.
...if we're willing to accept the backwardness as described above and
we can find a hack to keep the touchscreen from powering itself up,
then I think things could be made to work OK-ish. I can try to
investigate that route if it doesn't feel too ugly. ...oh, except for
another problem. The initial power up of the i2c-hid device would also
be a problem here. I guess with device_link we'd need to put all the
power up work into "runtime resume". The problem is that upon initial
power up we create "HID" sub-devices and (as far as I could tell) you
can't do that during a runtime resume. :( We could put a special case
to power the touchscreen up before the panel at probe time, but that
could have other consequences?
If we don't go with the backwardness then I think we're a bit stuck
with some of the original problems. Specifically it means that unless
userspace knows to turn off the touchscreen that the touchscreen would
force the panel to never be power cycled. There's at least one panel
(the one on sc7180-trogdor-homestar) where that's known to be a
problem. It also means that we're back to drawing extra power if the
touchscreen is left "on" but the panel power is turned off.
Let me know if I'm misunderstanding.
-Doug
^ permalink raw reply
* Re: [PATCH] [resend] Input: gpio-keys - Use input_report_key()
From: Dmitry Torokhov @ 2023-05-23 21:58 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linux-input, linux-kernel
In-Reply-To: <185f3320e39000159d4bd843fd3119b94c30d607.1684854795.git.geert+renesas@glider.be>
On Tue, May 23, 2023 at 05:13:55PM +0200, Geert Uytterhoeven wrote:
> Use the input_report_key() helper instead of open-coding the same
> operation.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3] Input: tests: add test to cover all input_grab_device() function
From: Dmitry Torokhov @ 2023-05-23 21:58 UTC (permalink / raw)
To: Dana Elfassy; +Cc: eballetb, javierm, linux-input, linux-kernel, Dana Elfassy
In-Reply-To: <20230522215514.722564-1-dangel101@gmail.com>
On Tue, May 23, 2023 at 12:55:14AM +0300, Dana Elfassy wrote:
> Currently input_grab_device() isn't covered by any tests
> Thus, adding a test to cover the cases:
> 1. The device is grabbed successfully
> 2. Trying to grab a device that is already grabbed by another input
> handle
>
> Signed-off-by: Dana Elfassy <dangel101@gmail.com>
> Tested-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] Input: xpad: Delete a Razer DeathAdder mouse VID/PID entry from the gamepad list
From: Dmitry Torokhov @ 2023-05-23 21:49 UTC (permalink / raw)
To: Ismael Ferreras Morezuelas; +Cc: linux-input, linux-kernel, Cameron Gutman
In-Reply-To: <5c12dbdb-5774-fc68-5c58-ca596383663e@gmail.com>
On Wed, Jan 05, 2022 at 01:28:09AM +0100, Ismael Ferreras Morezuelas wrote:
> While doing my research to improve the xpad device names I noticed
> that the 1532:0037 VID/PID seems to be used by the DeathAdder 2013,
> so that Razer Sabertooth instance looked wrong and very suspect to
> me. I didn't see any mention in the official drivers, either.
>
> After doing more research, it turns out that the xpad list
> is used by many other projects (like Steam) as-is [1], this
> issue was reported [2] and Valve/Sam Lantinga fixed it [3]:
>
> [1]: https://github.com/libsdl-org/SDL/blob/dcc5eef0e2395854b254ea2873a4899edab347c6/src/joystick/controller_type.h#L251
> [2]: https://steamcommunity.com/app/353380/discussions/0/1743392486228754770/
> [3]: https://hg.libsdl.org/SDL/rev/29809f6f0271
>
> (With multiple Internet users reporting similar issues, not linked here)
>
> After not being able to find the correct VID/PID combination anywhere
> on the Internet and not receiving any reply from Razer support I did
> some additional detective work, it seems like it presents itself as
> "Razer Sabertooth Gaming Controller (XBOX360)", code 1689:FE00.
>
> Leaving us with this:
> * Razer Sabertooth (1689:fe00)
> * Razer Sabertooth Elite (24c6:5d04)
> * Razer DeathAdder 2013 (1532:0037) [note: not a gamepad]
>
> So, to sum things up; remove this conflicting/duplicate entry:
>
> { 0x1532, 0x0037, "Razer Sabertooth", 0, XTYPE_XBOX360 },
>
> As the real/correct one is already present there, even if
> the Internet as a whole insists on presenting it as the
> Razer Sabertooth Elite, which (by all accounts) is not:
>
> { 0x1689, 0xfe00, "Razer Sabertooth", 0, XTYPE_XBOX360 },
>
> Actual change in SDL2 referencing this kernel issue:
> https://github.com/libsdl-org/SDL/commit/e5e54169754ca5d3e86339d968b20126d9da0a15
>
> For more information of the device, take a look here:
> https://github.com/xboxdrv/xboxdrv/pull/59
>
> You can see a lsusb dump here: https://github.com/xboxdrv/xboxdrv/files/76581/Qa6dBcrv.txt
>
> Cc: stable@vger.kernel.org
> Cc: Cameron Gutman <aicommander@gmail.com>
> Fixes: f554f619b70 ("Input: xpad - sync device IDs with xboxdrv")
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
> Reviewed-by: Cameron Gutman <aicommander@gmail.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 0/9] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Dmitry Torokhov @ 2023-05-23 21:39 UTC (permalink / raw)
To: Douglas Anderson
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
dri-devel, linux-input, hsinyi, devicetree, yangcong5,
linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
Hi Doug,
On Tue, May 23, 2023 at 12:27:54PM -0700, Douglas Anderson wrote:
>
> The big motivation for this patch series is mostly described in the patch
> ("drm/panel: Add a way for other devices to follow panel state"), but to
> quickly summarize here: for touchscreens that are connected to a panel we
> need the ability to power sequence the two device together. This is not a
> new need, but so far we've managed to get by through a combination of
> inefficiency, added costs, or perhaps just a little bit of brokenness.
> It's time to do better. This patch series allows us to do better.
This seems to grow a new way of building relationship between panels and
associated devices. Can we make device_link_*() work for us?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: ensure timely release of driver-allocated resources
From: Dmitry Torokhov @ 2023-05-23 21:05 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Benjamin Tissoires, Stephen Boyd, linux-input, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.2305231554250.29760@cbobk.fhfr.pm>
On Tue, May 23, 2023 at 03:57:03PM +0200, Jiri Kosina wrote:
> On Fri, 5 May 2023, Dmitry Torokhov wrote:
>
> > More and more drivers rely on devres to manage their resources, however
> > if bus' probe() and release() methods are not trivial and control some
> > of resources as well (for example enable or disable clocks, or attach
> > device to a power domain), we need to make sure that driver-allocated
> > resources are released immediately after driver's remove() method
> > returns, and not postponed until driver core gets around to releasing
> > resources.
> >
> > In case of HID we should not try to close the report and release
> > associated memory until after all devres callbacks are executed. To fix
> > that we open a new devres group before calling driver's probe() and
> > explicitly release it when we return from driver's remove().
> >
> > This is similar to what we did for I2C bus in commit 5b5475826c52 ("i2c:
> > ensure timely release of driver-allocated resources"). It is tempting to
> > try and move this into driver core, but actually doing so is challenging,
> > we need to split bus' remove() method into pre- and post-remove methods,
> > which would make the logic even less clear.
> >
> > Reported-by: Stephen Boyd <swboyd@chromium.org>
> > Link: https://lore.kernel.org/r/20230505232417.1377393-1-swboyd@chromium.org
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/hid/hid-core.c | 55 ++++++++++++++++++++++++++++--------------
> > include/linux/hid.h | 1 +
> > 2 files changed, 38 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index c4ac9081194c..02a43bba9091 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -2602,35 +2602,29 @@ static bool hid_device_check_match(struct hid_device *hdev,
> > return !hid_ignore_special_drivers;
> > }
> >
> > -static int hid_device_probe(struct device *dev)
> > +static int __hid_device_probe(struct hid_device *hdev)
> > {
> > - struct hid_driver *hdrv = to_hid_driver(dev->driver);
> > - struct hid_device *hdev = to_hid_device(dev);
> > + struct hid_driver *hdrv = to_hid_driver(hdev->dev.driver);
> > const struct hid_device_id *id;
> > int ret;
> >
> > - if (down_interruptible(&hdev->driver_input_lock)) {
> > - ret = -EINTR;
> > - goto end;
> > - }
> > hdev->io_started = false;
> > -
> > clear_bit(ffs(HID_STAT_REPROBED), &hdev->status);
> >
> > - if (hdev->driver) {
> > - ret = 0;
> > - goto unlock;
> > - }
> > + if (hdev->driver)
> > + return 0;
> >
> > - if (!hid_device_check_match(hdev, hdrv, &id)) {
> > - ret = -ENODEV;
> > - goto unlock;
> > - }
>
> Dmitry, which tree is this patch against, please? The code above looks
> different in current tree (and hasn't been touched for a while).
My bad, I had some patches in my tree that I forgot about. I sent out
a v2.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v2 2/2] HID: ensure timely release of driver-allocated resources
From: Dmitry Torokhov @ 2023-05-23 21:04 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel, Stephen Boyd
In-Reply-To: <20230523-hid-devres-v2-0-5220211a428c@gmail.com>
More and more drivers rely on devres to manage their resources, however
if bus' probe() and release() methods are not trivial and control some
of resources as well (for example enable or disable clocks, or attach
device to a power domain), we need to make sure that driver-allocated
resources are released immediately after driver's remove() method
returns, and not postponed until driver core gets around to releasing
resources.
In case of HID we should not try to close the report and release
associated memory until after all devres callbacks are executed. To fix
that we open a new devres group before calling driver's probe() and
explicitly release it when we return from driver's remove().
This is similar to what we did for I2C bus in commit 5b5475826c52 ("i2c:
ensure timely release of driver-allocated resources"). It is tempting to
try and move this into driver core, but actually doing so is challenging,
we need to split bus' remove() method into pre- and post-remove methods,
which would make the logic even less clear.
Reported-by: Stephen Boyd <swboyd@chromium.org>
Link: https://lore.kernel.org/r/20230505232417.1377393-1-swboyd@chromium.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/hid/hid-core.c | 17 +++++++++++++++++
include/linux/hid.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 1f0bb2784bfc..4b7062dcefec 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2614,6 +2614,10 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv)
if (!hid_check_device_match(hdev, hdrv, &id))
return -ENODEV;
+ hdev->devres_group_id = devres_open_group(&hdev->dev, NULL, GFP_KERNEL);
+ if (!hdev->devres_group_id)
+ return -ENOMEM;
+
/* reset the quirks that has been previously set */
hdev->quirks = hid_lookup_quirk(hdev);
hdev->driver = hdrv;
@@ -2626,7 +2630,16 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv)
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
}
+ /*
+ * Note that we are not closing the devres group opened above so
+ * even resources that were attached to the device after probe is
+ * run are released when hid_device_remove() is executed. This is
+ * needed as some drivers would allocate additional resources,
+ * for example when updating firmware.
+ */
+
if (ret) {
+ devres_release_group(&hdev->dev, hdev->devres_group_id);
hid_close_report(hdev);
hdev->driver = NULL;
}
@@ -2669,6 +2682,10 @@ static void hid_device_remove(struct device *dev)
hdrv->remove(hdev);
else /* default remove */
hid_hw_stop(hdev);
+
+ /* Release all devres resources allocated by the driver */
+ devres_release_group(&hdev->dev, hdev->devres_group_id);
+
hid_close_report(hdev);
hdev->driver = NULL;
}
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 4e4c4fe36911..39e21e3815ad 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -597,6 +597,7 @@ struct hid_device { /* device report descriptor */
struct semaphore driver_input_lock; /* protects the current driver */
struct device dev; /* device */
struct hid_driver *driver;
+ void *devres_group_id; /* ID of probe devres group */
const struct hid_ll_driver *ll_driver;
struct mutex ll_open_lock;
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH v2 1/2] HID: split apart hid_device_probe to make logic more apparent
From: Dmitry Torokhov @ 2023-05-23 21:04 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel, Stephen Boyd
In-Reply-To: <20230523-hid-devres-v2-0-5220211a428c@gmail.com>
hid_device_probe() has a complex flow and locks and unlocks a mutex.
Move the most of the logic into __hid_device_probe() and
hid_check_device_match() and leave the locking in hid_device_probe()
which makes the code more clear.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/hid/hid-core.c | 101 ++++++++++++++++++++++++++-----------------------
1 file changed, 54 insertions(+), 47 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 22623eb4f72f..1f0bb2784bfc 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2587,64 +2587,71 @@ bool hid_compare_device_paths(struct hid_device *hdev_a,
}
EXPORT_SYMBOL_GPL(hid_compare_device_paths);
+static bool hid_check_device_match(struct hid_device *hdev,
+ struct hid_driver *hdrv,
+ const struct hid_device_id **id)
+{
+ *id = hid_match_device(hdev, hdrv);
+ if (!*id)
+ return -ENODEV;
+
+ if (hdrv->match)
+ return hdrv->match(hdev, hid_ignore_special_drivers);
+
+ /*
+ * hid-generic implements .match(), so we must be dealing with a
+ * different HID driver here, and can simply check if
+ * hid_ignore_special_drivers is set or not.
+ */
+ return !hid_ignore_special_drivers;
+}
+
+static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv)
+{
+ const struct hid_device_id *id;
+ int ret;
+
+ if (!hid_check_device_match(hdev, hdrv, &id))
+ return -ENODEV;
+
+ /* reset the quirks that has been previously set */
+ hdev->quirks = hid_lookup_quirk(hdev);
+ hdev->driver = hdrv;
+
+ if (hdrv->probe) {
+ ret = hdrv->probe(hdev, id);
+ } else { /* default probe */
+ ret = hid_open_report(hdev);
+ if (!ret)
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ }
+
+ if (ret) {
+ hid_close_report(hdev);
+ hdev->driver = NULL;
+ }
+
+ return ret;
+}
+
static int hid_device_probe(struct device *dev)
{
- struct hid_driver *hdrv = to_hid_driver(dev->driver);
struct hid_device *hdev = to_hid_device(dev);
- const struct hid_device_id *id;
+ struct hid_driver *hdrv = to_hid_driver(dev->driver);
int ret = 0;
- if (down_interruptible(&hdev->driver_input_lock)) {
- ret = -EINTR;
- goto end;
- }
- hdev->io_started = false;
+ if (down_interruptible(&hdev->driver_input_lock))
+ return -EINTR;
+ hdev->io_started = false;
clear_bit(ffs(HID_STAT_REPROBED), &hdev->status);
- if (!hdev->driver) {
- id = hid_match_device(hdev, hdrv);
- if (id == NULL) {
- ret = -ENODEV;
- goto unlock;
- }
+ if (!hdev->driver)
+ ret = __hid_device_probe(hdev, hdrv);
- if (hdrv->match) {
- if (!hdrv->match(hdev, hid_ignore_special_drivers)) {
- ret = -ENODEV;
- goto unlock;
- }
- } else {
- /*
- * hid-generic implements .match(), so if
- * hid_ignore_special_drivers is set, we can safely
- * return.
- */
- if (hid_ignore_special_drivers) {
- ret = -ENODEV;
- goto unlock;
- }
- }
-
- /* reset the quirks that has been previously set */
- hdev->quirks = hid_lookup_quirk(hdev);
- hdev->driver = hdrv;
- if (hdrv->probe) {
- ret = hdrv->probe(hdev, id);
- } else { /* default probe */
- ret = hid_open_report(hdev);
- if (!ret)
- ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
- }
- if (ret) {
- hid_close_report(hdev);
- hdev->driver = NULL;
- }
- }
-unlock:
if (!hdev->io_started)
up(&hdev->driver_input_lock);
-end:
+
return ret;
}
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH v2 0/2] HID: ensure timely release of driver resources
From: Dmitry Torokhov @ 2023-05-23 21:04 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel, Stephen Boyd
These 2 patches address an issue in HID subsystem where cleanup code in
the bus code runs before releasing devres-managed resources by the
driver core.
This is a V2 of the patch, which was split in 2 parts - generic cleanup
and the actualy change addressing the issue.
Link: https://lore.kernel.org/r/20230505232417.1377393-1-swboyd@chromium.org
Link: https://lore.kernel.org/r/ZFWarGkRAfPOmI6E@google.com
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Dmitry Torokhov (2):
HID: split apart hid_device_probe to make logic more apparent
HID: ensure timely release of driver-allocated resources
drivers/hid/hid-core.c | 118 +++++++++++++++++++++++++++++--------------------
include/linux/hid.h | 1 +
2 files changed, 72 insertions(+), 47 deletions(-)
---
base-commit: 9f925874281f7e8855855d6210d6e29d844e4307
change-id: 20230523-hid-devres-b0edd05de354
Best regards,
--
Dmitry
^ permalink raw reply
* [PATCH 9/9] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels
From: Douglas Anderson @ 2023-05-23 19:28 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
Let's provide the proper link from the touchscreen to the panel on
trogdor devices where the touchscreen support it. This allows the OS
to power sequence the touchscreen more properly.
For the most part, this is just expected to marginally improve power
consumption while the screen is off. However, in at least one trogdor
model (wormdingler) it's suspected that this will fix some behavorial
corner cases when the panel power cycles (like for a modeset) without
the touchscreen power cycling.
NOTE: some trogdor variants use touchscreens that don't (yet) support
linking the touchscreen and the panel. Those variants are left alone.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 1 +
arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi | 1 +
arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi | 1 +
arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi | 1 +
arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi | 1 +
arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi | 1 +
6 files changed, 6 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
index 8b8ea8af165d..b4f328d3e1f6 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
@@ -104,6 +104,7 @@ ap_ts: touchscreen@5d {
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+ panel = <&panel>;
reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
vdd-supply = <&pp3300_ts>;
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
index b3ba23a88a0b..88aeb415bd5b 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
@@ -116,6 +116,7 @@ ap_ts: touchscreen@14 {
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+ panel = <&panel>;
reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
vdd-supply = <&pp3300_touch>;
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
index 269007d73162..c65f18ea3e5c 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
@@ -43,6 +43,7 @@ ap_ts: touchscreen@10 {
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+ panel = <&panel>;
post-power-on-delay-ms = <20>;
hid-descr-addr = <0x0001>;
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
index 6c5287bd27d6..d2aafd1ea672 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
@@ -102,6 +102,7 @@ ap_ts: touchscreen@10 {
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+ panel = <&panel>;
post-power-on-delay-ms = <20>;
hid-descr-addr = <0x0001>;
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi
index 8e7b42f843d4..0785873d1345 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi
@@ -99,6 +99,7 @@ ap_ts: touchscreen@10 {
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+ panel = <&panel>;
post-power-on-delay-ms = <20>;
hid-descr-addr = <0x0001>;
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
index 262d6691abd9..f70f5b42c845 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
@@ -154,6 +154,7 @@ ap_ts: touchscreen@1 {
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_EDGE_FALLING>;
+ panel = <&panel>;
post-power-on-delay-ms = <70>;
hid-descr-addr = <0x0001>;
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH 6/9] HID: i2c-hid: Make suspend and resume into helper functions
From: Douglas Anderson @ 2023-05-23 19:28 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
In a future patch we'd like to be able to call the current i2c-hid
suspend and resume functions from times other than system
suspend. Move the functions higher up in the file and have them take a
"struct i2c_hid" to make this simpler. We'll then add tiny wrappers of
the functions for use with system suspend.
This change is expected to have no functional effect.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 98 +++++++++++++++++-------------
1 file changed, 56 insertions(+), 42 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index fb5ebf3ca739..34c0d98b4976 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -941,6 +941,57 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid)
ihid->ops->shutdown_tail(ihid->ops);
}
+static int i2c_hid_core_suspend(struct i2c_hid *ihid)
+{
+ struct i2c_client *client = ihid->client;
+ struct hid_device *hid = ihid->hid;
+ int ret;
+
+ ret = hid_driver_suspend(hid, PMSG_SUSPEND);
+ if (ret < 0)
+ return ret;
+
+ /* Save some power */
+ i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+
+ disable_irq(client->irq);
+
+ if (!device_may_wakeup(&client->dev))
+ i2c_hid_core_power_down(ihid);
+
+ return 0;
+}
+
+static int i2c_hid_core_resume(struct i2c_hid *ihid)
+{
+ struct i2c_client *client = ihid->client;
+ struct hid_device *hid = ihid->hid;
+ int ret;
+
+ if (!device_may_wakeup(&client->dev))
+ i2c_hid_core_power_up(ihid);
+
+ enable_irq(client->irq);
+
+ /* Instead of resetting device, simply powers the device on. This
+ * solves "incomplete reports" on Raydium devices 2386:3118 and
+ * 2386:4B33 and fixes various SIS touchscreens no longer sending
+ * data after a suspend/resume.
+ *
+ * However some ALPS touchpads generate IRQ storm without reset, so
+ * let's still reset them here.
+ */
+ if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME)
+ ret = i2c_hid_hwreset(ihid);
+ else
+ ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
+
+ if (ret)
+ return ret;
+
+ return hid_driver_reset_resume(hid);
+}
+
/**
* i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device.
* @ihid: The ihid object created during probe.
@@ -1115,61 +1166,24 @@ void i2c_hid_core_shutdown(struct i2c_client *client)
}
EXPORT_SYMBOL_GPL(i2c_hid_core_shutdown);
-static int i2c_hid_core_suspend(struct device *dev)
+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);
- struct hid_device *hid = ihid->hid;
- int ret;
-
- ret = hid_driver_suspend(hid, PMSG_SUSPEND);
- if (ret < 0)
- return ret;
- /* Save some power */
- i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
-
- disable_irq(client->irq);
-
- if (!device_may_wakeup(&client->dev))
- i2c_hid_core_power_down(ihid);
-
- return 0;
+ return i2c_hid_core_suspend(ihid);
}
-static int i2c_hid_core_resume(struct device *dev)
+static int i2c_hid_core_pm_resume(struct device *dev)
{
- int ret;
struct i2c_client *client = to_i2c_client(dev);
struct i2c_hid *ihid = i2c_get_clientdata(client);
- struct hid_device *hid = ihid->hid;
- if (!device_may_wakeup(&client->dev))
- i2c_hid_core_power_up(ihid);
-
- enable_irq(client->irq);
-
- /* Instead of resetting device, simply powers the device on. This
- * solves "incomplete reports" on Raydium devices 2386:3118 and
- * 2386:4B33 and fixes various SIS touchscreens no longer sending
- * data after a suspend/resume.
- *
- * However some ALPS touchpads generate IRQ storm without reset, so
- * let's still reset them here.
- */
- if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME)
- ret = i2c_hid_hwreset(ihid);
- else
- ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
-
- if (ret)
- return ret;
-
- return hid_driver_reset_resume(hid);
+ return i2c_hid_core_resume(ihid);
}
const struct dev_pm_ops i2c_hid_core_pm = {
- SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
+ SYSTEM_SLEEP_PM_OPS(i2c_hid_core_pm_suspend, i2c_hid_core_pm_resume)
};
EXPORT_SYMBOL_GPL(i2c_hid_core_pm);
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH 8/9] HID: i2c-hid: Do panel follower work on the system_wq
From: Douglas Anderson @ 2023-05-23 19:28 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
Turning on an i2c-hid device can be a slow process. This is why
i2c-hid devices use PROBE_PREFER_ASYNCHRONOUS. Unfortunately, when
we're a panel follower the i2c-hid power up sequence now blocks the
power on of the panel. Let's fix that by scheduling the work on the
system_wq.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 42 +++++++++++++++++++++++++++---
1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index f1bb89377e8d..800f0dc6f6cf 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -110,7 +110,9 @@ struct i2c_hid {
struct i2chid_ops *ops;
struct drm_panel_follower panel_follower;
+ struct work_struct panel_follower_prepare_work;
bool is_panel_follower;
+ bool prepare_work_finished;
};
static const struct i2c_hid_quirks {
@@ -1062,10 +1064,12 @@ int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
return ret;
}
-int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
+void ihid_core_panel_prepare_work(struct work_struct *work)
{
- struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
+ struct i2c_hid *ihid = container_of(work, struct i2c_hid,
+ panel_follower_prepare_work);
struct hid_device *hid = ihid->hid;
+ int ret;
/*
* hid->version is set on the first power up. If it's still zero then
@@ -1073,15 +1077,44 @@ int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
* steps.
*/
if (!hid->version)
- return i2c_hid_core_initial_power_up(ihid);
+ ret = i2c_hid_core_initial_power_up(ihid);
+ else
+ ret = i2c_hid_core_resume(ihid);
- return i2c_hid_core_resume(ihid);
+ if (ret)
+ dev_warn(&ihid->client->dev, "Power on failed: %d\n", ret);
+ else
+ WRITE_ONCE(ihid->prepare_work_finished, true);
+
+ /* Match with i2c_hid_core_panel_unpreparing() */
+ smp_wmb();
+}
+
+int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
+{
+ struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
+
+ /*
+ * Powering on a touchscreen can be a slow process. Queue the work to
+ * the system workqueue so we don't block the panel's power up.
+ */
+ WRITE_ONCE(ihid->prepare_work_finished, false);
+ schedule_work(&ihid->panel_follower_prepare_work);
+
+ return 0;
}
int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower)
{
struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
+ cancel_work_sync(&ihid->panel_follower_prepare_work);
+
+ /* Match with ihid_core_panel_prepare_work() */
+ smp_rmb();
+ if (!READ_ONCE(ihid->prepare_work_finished))
+ return 0;
+
return i2c_hid_core_suspend(ihid);
}
@@ -1124,6 +1157,7 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
init_waitqueue_head(&ihid->wait);
mutex_init(&ihid->reset_lock);
+ INIT_WORK(&ihid->panel_follower_prepare_work, ihid_core_panel_prepare_work);
/* we need to allocate the command buffer without knowing the maximum
* size of the reports. Let's use HID_MIN_BUFFER_SIZE, then we do the
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH 7/9] HID: i2c-hid: Support being a panel follower
From: Douglas Anderson @ 2023-05-23 19:28 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
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>
---
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 34c0d98b4976..f1bb89377e8d 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 @@ int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
return ret;
}
+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);
+}
+
+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,
+};
+
int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
u16 hid_descriptor_address, u32 quirks)
{
@@ -1119,6 +1151,41 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
hid->bus = BUS_I2C;
hid->initial_quirks = quirks;
+ /*
+ * See if we're following a panel. If drm_panel_add_follower()
+ * returns no error then we are.
+ */
+ ihid->panel_follower.funcs = &i2c_hid_core_panel_follower_funcs;
+ ret = drm_panel_add_follower(&client->dev, &ihid->panel_follower);
+ if (!ret) {
+ /* We're a follower. That means we'll power things up later. */
+ ihid->is_panel_follower = true;
+
+ /*
+ * 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(&client->dev)) {
+ dev_warn(&client->dev, "Can't wakeup if following panel\n");
+ device_set_wakeup_capable(&client->dev, false);
+ }
+
+ return 0;
+ }
+
+ /*
+ * -ENODEV means that we're not following a panel, so any other error
+ * is a real problem (like -EPROBE_DEFER, -ENOMEM, ...).
+ */
+ if (ret != -ENODEV)
+ goto err_mem_free;
+
+ /*
+ * We're not following a panel. That's fine and means that we
+ * can power up right away.
+ */
ret = i2c_hid_core_initial_power_up(ihid);
if (ret)
goto err_mem_free;
@@ -1143,7 +1210,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);
+ else
+ i2c_hid_core_power_down(ihid);
hid = ihid->hid;
hid_destroy_device(hid);
@@ -1171,6 +1245,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;
+
return i2c_hid_core_suspend(ihid);
}
@@ -1179,6 +1256,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;
+
return i2c_hid_core_resume(ihid);
}
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH 5/9] HID: i2c-hid: Rearrange probe() to power things up later
From: Douglas Anderson @ 2023-05-23 19:27 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
In a future patch, we want to change i2c-hid not to necessarily power
up the touchscreen during probe. In preparation for that, rearrange
the probe function so that we put as much stuff _before_ powering up
the device as possible.
This change is expected to have no functional effect.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 124 ++++++++++++++++++-----------
1 file changed, 77 insertions(+), 47 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 19d985c20a5c..fb5ebf3ca739 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -855,7 +855,8 @@ static int i2c_hid_init_irq(struct i2c_client *client)
irqflags = IRQF_TRIGGER_LOW;
ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
- irqflags | IRQF_ONESHOT, client->name, ihid);
+ irqflags | IRQF_ONESHOT | IRQF_NO_AUTOEN,
+ client->name, ihid);
if (ret < 0) {
dev_warn(&client->dev,
"Could not register for %s interrupt, irq = %d,"
@@ -940,6 +941,72 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid)
ihid->ops->shutdown_tail(ihid->ops);
}
+/**
+ * i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device.
+ * @ihid: The ihid object created during probe.
+ *
+ * This function is called at probe time.
+ *
+ * The initial power on is where we do some basic validation that the device
+ * exists, where we fetch the HID descriptor, and where we create the actual
+ * HID devices.
+ *
+ * Return: 0 or error code.
+ */
+int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
+{
+ struct i2c_client *client = ihid->client;
+ struct hid_device *hid = ihid->hid;
+ int ret;
+
+ ret = i2c_hid_core_power_up(ihid);
+ if (ret)
+ return ret;
+
+ /* Make sure there is something at this address */
+ ret = i2c_smbus_read_byte(client);
+ if (ret < 0) {
+ i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
+ ret = -ENXIO;
+ goto err;
+ }
+
+ ret = i2c_hid_fetch_hid_descriptor(ihid);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "Failed to fetch the HID Descriptor\n");
+ goto err;
+ }
+
+ enable_irq(client->irq);
+
+ hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
+ hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
+ hid->product = le16_to_cpu(ihid->hdesc.wProductID);
+
+ hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor,
+ hid->product);
+
+ snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
+ client->name, (u16)hid->vendor, (u16)hid->product);
+ strscpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
+
+ ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
+
+ ret = hid_add_device(hid);
+ if (ret) {
+ if (ret != -ENODEV)
+ hid_err(client, "can't add hid device: %d\n", ret);
+ goto err;
+ }
+
+ return 0;
+
+err:
+ i2c_hid_core_power_down(ihid);
+ return ret;
+}
+
int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
u16 hid_descriptor_address, u32 quirks)
{
@@ -966,16 +1033,10 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
if (!ihid)
return -ENOMEM;
- ihid->ops = ops;
-
- ret = i2c_hid_core_power_up(ihid);
- if (ret)
- return ret;
-
i2c_set_clientdata(client, ihid);
+ ihid->ops = ops;
ihid->client = client;
-
ihid->wHIDDescRegister = cpu_to_le16(hid_descriptor_address);
init_waitqueue_head(&ihid->wait);
@@ -986,28 +1047,12 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
* real computation later. */
ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
if (ret < 0)
- goto err_powered;
-
+ return ret;
device_enable_async_suspend(&client->dev);
- /* Make sure there is something at this address */
- ret = i2c_smbus_read_byte(client);
- if (ret < 0) {
- i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
- ret = -ENXIO;
- goto err_powered;
- }
-
- ret = i2c_hid_fetch_hid_descriptor(ihid);
- if (ret < 0) {
- dev_err(&client->dev,
- "Failed to fetch the HID Descriptor\n");
- goto err_powered;
- }
-
ret = i2c_hid_init_irq(client);
if (ret < 0)
- goto err_powered;
+ goto err_buffers_allocated;
hid = hid_allocate_device();
if (IS_ERR(hid)) {
@@ -1021,26 +1066,11 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
hid->ll_driver = &i2c_hid_ll_driver;
hid->dev.parent = &client->dev;
hid->bus = BUS_I2C;
- hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
- hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
- hid->product = le16_to_cpu(ihid->hdesc.wProductID);
-
hid->initial_quirks = quirks;
- hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor,
- hid->product);
-
- snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
- client->name, (u16)hid->vendor, (u16)hid->product);
- strscpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
-
- ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
- ret = hid_add_device(hid);
- if (ret) {
- if (ret != -ENODEV)
- hid_err(client, "can't add hid device: %d\n", ret);
+ ret = i2c_hid_core_initial_power_up(ihid);
+ if (ret)
goto err_mem_free;
- }
return 0;
@@ -1050,9 +1080,9 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
err_irq:
free_irq(client->irq, ihid);
-err_powered:
- i2c_hid_core_power_down(ihid);
+err_buffers_allocated:
i2c_hid_free_buffers(ihid);
+
return ret;
}
EXPORT_SYMBOL_GPL(i2c_hid_core_probe);
@@ -1062,6 +1092,8 @@ 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);
+
hid = ihid->hid;
hid_destroy_device(hid);
@@ -1069,8 +1101,6 @@ void i2c_hid_core_remove(struct i2c_client *client)
if (ihid->bufsize)
i2c_hid_free_buffers(ihid);
-
- i2c_hid_core_power_down(ihid);
}
EXPORT_SYMBOL_GPL(i2c_hid_core_remove);
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH 4/9] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS()
From: Douglas Anderson @ 2023-05-23 19:27 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
The SYSTEM_SLEEP_PM_OPS() allows us to get rid of '#ifdef
CONFIG_PM_SLEEP', as talked about in commit 1a3c7bb08826 ("PM: core:
Add new *_PM_OPS macros, deprecate old ones").
This change is expected to have no functional effect.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index efbba0465eef..19d985c20a5c 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1085,7 +1085,6 @@ void i2c_hid_core_shutdown(struct i2c_client *client)
}
EXPORT_SYMBOL_GPL(i2c_hid_core_shutdown);
-#ifdef CONFIG_PM_SLEEP
static int i2c_hid_core_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -1138,10 +1137,9 @@ static int i2c_hid_core_resume(struct device *dev)
return hid_driver_reset_resume(hid);
}
-#endif
const struct dev_pm_ops i2c_hid_core_pm = {
- SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
+ SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
};
EXPORT_SYMBOL_GPL(i2c_hid_core_pm);
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH 3/9] drm/panel: Add a way for other devices to follow panel state
From: Douglas Anderson @ 2023-05-23 19:27 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
These days, it's fairly common to see panels that have touchscreens
attached to them. The panel and the touchscreen can somewhat be
thought of as totally separate devices and, historically, this is how
Linux has treated them. However, treating them as separate isn't
necessarily the best way to model the two devices, it was just that
there was no better way. Specifically, there is little practical
reason to have the touchscreen powered on when the panel is turned
off, but if we model the devices separately we have no way to keep the
two devices' power states in sync with each other.
The issue described above makes it sound as if the problem here is
just about efficiency. We're wasting power keeping the touchscreen
powered up when the screen is off. While that's true, the problem can
go deeper. Specifically, hardware designers see that there's no reason
to have the touchscreen on while the screen is off and then build
hardware assuming that software would never turn the touchscreen on
while the screen is off.
In the very simplest case of hardware designs like this, the
touchscreen and the panel share some power rails. In most cases, this
turns out not to be terrible and is, again, just a little less
efficient. Specifically if we tell Linux that the touchscreen and the
panel are using the same rails then Linux will keep the rails on when
_either_ device is turned on. That ends to work OK-ish, but now if you
turn the panel off not only will the touchscreen remain powered, but
the power rails for the panel itself won't be switched off, burning
extra power.
The above two inefficiencies are _extra_ minor when you consider the
fact that laptops rarely spend much time with the screen off. The main
use case would be when an external screen (and presumably a power
supply) is attached.
Unfortunately, it gets worse from here. On sc7180-trogdor-homestar,
for instance, the display's TCON (timing controller) sometimes crashes
if you didn't power cycle it whenever you stopp and restart the video
stream (like during a modeset). The touchscreen keeping the power
rails on caused real problems. One proposal in the homestar timeframe
was to move the touchscreen to an always-on rail, dedicating the main
power rail to the panel. That caused _different_ problems as talked
about in commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the
reset line to the regulator"). The end result of all of this was to
add an extra regulator to the board, increasing cost.
Recently, Cong Yang posted a patch [1] where things are even worse.
The panel and touch controller on that system seem even more
intimately tied together and really can't be thought of separately.
To address this issue, let's start allowing devices to register
themselves as "panel followers". These devices will get called after a
panel has been powered on and before a panel is powered off. This
makes the panel the primary device in charge of the power state, which
matches how userspace uses it.
The panel follower API should be fairly straightforward to use. The
current code assumes that panel followers are using device tree and
have a "panel" property pointing to the panel to follow. More
flexibility and non-DT implementations could be added as needed.
Right now, panel followers can follow the prepare/unprepare functions.
There could be arguments made that, instead, they should follow
enable/disable. I've chosen prepare/unprepare for now since those
functions are guaranteed to power up/power down the panel and it seems
better to start the process earlier.
[1] 20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/gpu/drm/drm_panel.c | 149 +++++++++++++++++++++++++++++++++++-
include/drm/drm_panel.h | 75 ++++++++++++++++++
2 files changed, 220 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 4e1c4e42575b..c4d9db435f15 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -58,6 +58,8 @@ void drm_panel_init(struct drm_panel *panel, struct device *dev,
const struct drm_panel_funcs *funcs, int connector_type)
{
INIT_LIST_HEAD(&panel->list);
+ INIT_LIST_HEAD(&panel->followers);
+ mutex_init(&panel->follower_lock);
panel->dev = dev;
panel->funcs = funcs;
panel->connector_type = connector_type;
@@ -105,6 +107,7 @@ EXPORT_SYMBOL(drm_panel_remove);
*/
int drm_panel_prepare(struct drm_panel *panel)
{
+ struct drm_panel_follower *follower;
int ret;
if (!panel)
@@ -115,14 +118,27 @@ int drm_panel_prepare(struct drm_panel *panel)
return 0;
}
+ mutex_lock(&panel->follower_lock);
+
if (panel->funcs && panel->funcs->prepare) {
ret = panel->funcs->prepare(panel);
if (ret < 0)
- return ret;
+ goto exit;
}
panel->prepared = true;
- return 0;
+ list_for_each_entry(follower, &panel->followers, list) {
+ ret = follower->funcs->panel_prepared(follower);
+ if (ret < 0)
+ dev_info(panel->dev, "%ps failed: %d\n",
+ follower->funcs->panel_prepared, ret);
+ }
+
+ ret = 0;
+exit:
+ mutex_unlock(&panel->follower_lock);
+
+ return ret;
}
EXPORT_SYMBOL(drm_panel_prepare);
@@ -139,6 +155,7 @@ EXPORT_SYMBOL(drm_panel_prepare);
*/
int drm_panel_unprepare(struct drm_panel *panel)
{
+ struct drm_panel_follower *follower;
int ret;
if (!panel)
@@ -149,14 +166,27 @@ int drm_panel_unprepare(struct drm_panel *panel)
return 0;
}
+ mutex_lock(&panel->follower_lock);
+
+ list_for_each_entry(follower, &panel->followers, list) {
+ ret = follower->funcs->panel_unpreparing(follower);
+ if (ret < 0)
+ dev_info(panel->dev, "%ps failed: %d\n",
+ follower->funcs->panel_unpreparing, ret);
+ }
+
if (panel->funcs && panel->funcs->unprepare) {
ret = panel->funcs->unprepare(panel);
if (ret < 0)
- return ret;
+ goto exit;
}
panel->prepared = false;
- return 0;
+ ret = 0;
+exit:
+ mutex_unlock(&panel->follower_lock);
+
+ return ret;
}
EXPORT_SYMBOL(drm_panel_unprepare);
@@ -342,6 +372,117 @@ int of_drm_get_panel_orientation(const struct device_node *np,
EXPORT_SYMBOL(of_drm_get_panel_orientation);
#endif
+/**
+ * drm_panel_add_follower() - Register something to follow panel enable state.
+ * @follower_dev: The 'struct device' for the follower.
+ * @follower: The panel follower descriptor for the follower.
+ *
+ * A panel follower is called right after preparing the panel and right before
+ * unpreparing the panel. It's primary intention is to power on an associated
+ * touchscreen, though it could be used for any similar devices. Multiple
+ * devices are allowed the follow the same panel.
+ *
+ * If a follower is added to a panel that's already been turned on, the
+ * follower's prepare callback is called right away.
+ *
+ * At the moment panels can only be followed on device tree enabled systems.
+ * The "panel" property of the follower points to the panel to be followed.
+ *
+ * Return: 0 or an error code.
+ */
+int drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ struct device_node *panel_np;
+ struct drm_panel *panel;
+ int ret;
+
+ panel_np = of_parse_phandle(follower_dev->of_node, "panel", 0);
+ if (!panel_np)
+ return -ENODEV;
+
+ panel = of_drm_find_panel(panel_np);
+ of_node_put(panel_np);
+ if (IS_ERR(panel))
+ return PTR_ERR(panel);
+
+ get_device(panel->dev);
+ follower->panel = panel;
+
+ mutex_lock(&panel->follower_lock);
+
+ list_add_tail(&follower->list, &panel->followers);
+ if (panel->prepared) {
+ ret = follower->funcs->panel_prepared(follower);
+ if (ret < 0)
+ dev_info(panel->dev, "%ps failed: %d\n",
+ follower->funcs->panel_prepared, ret);
+ }
+
+ mutex_unlock(&panel->follower_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_panel_add_follower);
+
+/**
+ * drm_panel_remove_follower() - Reverse drm_panel_add_follower().
+ * @follower: The panel follower descriptor for the follower.
+ *
+ * Undo drm_panel_add_follower(). This includes calling the follower's disable
+ * function if we're removed from a panel that's currently enabled.
+ *
+ * Return: 0 or an error code.
+ */
+void drm_panel_remove_follower(struct drm_panel_follower *follower)
+{
+ struct drm_panel *panel = follower->panel;
+ int ret;
+
+ mutex_lock(&panel->follower_lock);
+
+ if (panel->prepared) {
+ ret = follower->funcs->panel_unpreparing(follower);
+ if (ret < 0)
+ dev_info(panel->dev, "%ps failed: %d\n",
+ follower->funcs->panel_unpreparing, ret);
+ }
+ list_del_init(&follower->list);
+
+ mutex_unlock(&panel->follower_lock);
+
+ put_device(panel->dev);
+}
+EXPORT_SYMBOL(drm_panel_remove_follower);
+
+static void drm_panel_remove_follower_void(void *follower)
+{
+ drm_panel_remove_follower(follower);
+}
+
+/**
+ * devm_drm_panel_add_follower() - devm version of drm_panel_add_follower()
+ * @follower_dev: The 'struct device' for the follower.
+ * @follower: The panel follower descriptor for the follower.
+ *
+ * Handles calling drm_panel_remove_follower() using devm on the follower_dev.
+ *
+ * Return: 0 or an error code.
+ */
+int devm_drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ int ret;
+
+ ret = drm_panel_add_follower(follower_dev, follower);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(follower_dev,
+ drm_panel_remove_follower_void, follower);
+}
+EXPORT_SYMBOL(devm_drm_panel_add_follower);
+
#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
/**
* drm_panel_of_backlight - use backlight device node for backlight
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index c6cf75909389..e0a4d2f6f7fb 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -27,12 +27,14 @@
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/list.h>
+#include <linux/mutex.h>
struct backlight_device;
struct dentry;
struct device_node;
struct drm_connector;
struct drm_device;
+struct drm_panel_follower;
struct drm_panel;
struct display_timing;
@@ -144,6 +146,45 @@ struct drm_panel_funcs {
void (*debugfs_init)(struct drm_panel *panel, struct dentry *root);
};
+struct drm_panel_follower_funcs {
+ /**
+ * @panel_prepared:
+ *
+ * Called after the panel has been powered on.
+ */
+ int (*panel_prepared)(struct drm_panel_follower *follower);
+
+ /**
+ * @panel_unpreparing:
+ *
+ * Called before the panel is powered off.
+ */
+ int (*panel_unpreparing)(struct drm_panel_follower *follower);
+};
+
+struct drm_panel_follower {
+ /**
+ * @funcs:
+ *
+ * Dependent device callbacks; should be initted by the caller.
+ */
+ const struct drm_panel_follower_funcs *funcs;
+
+ /**
+ * @list
+ *
+ * Used for linking into panel's list; set by drm_panel_add_follower().
+ */
+ struct list_head list;
+
+ /**
+ * @panel
+ *
+ * The panel we're dependent on; set by drm_panel_add_follower().
+ */
+ struct drm_panel *panel;
+};
+
/**
* struct drm_panel - DRM panel object
*/
@@ -189,6 +230,20 @@ struct drm_panel {
*/
struct list_head list;
+ /**
+ * @followers:
+ *
+ * A list of struct drm_panel_follower dependent on this panel.
+ */
+ struct list_head followers;
+
+ /**
+ * @followers_lock:
+ *
+ * Lock for followers list.
+ */
+ struct mutex follower_lock;
+
/**
* @prepare_prev_first:
*
@@ -246,6 +301,26 @@ static inline int of_drm_get_panel_orientation(const struct device_node *np,
}
#endif
+#if defined(CONFIG_DRM_PANEL)
+int drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower);
+void drm_panel_remove_follower(struct drm_panel_follower *follower);
+int devm_drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower);
+#else
+static inline int drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ return -ENODEV;
+}
+static inline void drm_panel_remove_follower(struct drm_panel_follower *follower) { }
+static inline int devm_drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ return -ENODEV;
+}
+#endif
+
#if IS_ENABLED(CONFIG_DRM_PANEL) && (IS_BUILTIN(CONFIG_BACKLIGHT_CLASS_DEVICE) || \
(IS_MODULE(CONFIG_DRM) && IS_MODULE(CONFIG_BACKLIGHT_CLASS_DEVICE)))
int drm_panel_of_backlight(struct drm_panel *panel);
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH 2/9] drm/panel: Check for already prepared/enabled in drm_panel
From: Douglas Anderson @ 2023-05-23 19:27 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
In a whole pile of panel drivers, we have code to make the
prepare/unprepare/enable/disable callbacks behave as no-ops if they've
already been called. It's silly to have this code duplicated
everywhere. Add it to the core instead so that we can eventually
delete it from all the drivers. Note: to get some idea of the
duplicated code, try:
git grep 'if.*>prepared' -- drivers/gpu/drm/panel
git grep 'if.*>enabled' -- drivers/gpu/drm/panel
NOTE: arguably, the right thing to do here is actually to skip this
patch and simply remove all the extra checks from the individual
drivers. Perhaps the checks were needed at some point in time in the
past but maybe they no longer are? Certainly as we continue
transitioning over to "panel_bridge" then we expect there to be much
less variety in how these calls are made. When we're called as part of
the bridge chain, things should be pretty simple. In fact, there was
some discussion in the past about these checks [1], including a
discussion about whether the checks were needed and whether the calls
ought to be refcounted. At the time, I decided not to mess with it
because it felt too risky.
Looking closer at it now, I'm fairly certain that nothing in the
existing codebase is expecting these calls to be refcounted. The only
real question is whether someone is already doing something to ensure
prepare()/unprepare() match and enabled()/disable() match. I would say
that, even if there is something else ensuring that things match,
there's enough complexity that adding an extra bool and an extra
double-check here is a good idea. Let's add a drm_warn() to let people
know that it's considered a minor error to take advantage of
drm_panel's double-checking but we'll still make things work fine.
[1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/gpu/drm/drm_panel.c | 49 ++++++++++++++++++++++++++++++++-----
include/drm/drm_panel.h | 14 +++++++++++
2 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index f634371c717a..4e1c4e42575b 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -105,11 +105,22 @@ EXPORT_SYMBOL(drm_panel_remove);
*/
int drm_panel_prepare(struct drm_panel *panel)
{
+ int ret;
+
if (!panel)
return -EINVAL;
- if (panel->funcs && panel->funcs->prepare)
- return panel->funcs->prepare(panel);
+ if (panel->prepared) {
+ dev_warn(panel->dev, "Skipping prepare of already prepared panel\n");
+ return 0;
+ }
+
+ if (panel->funcs && panel->funcs->prepare) {
+ ret = panel->funcs->prepare(panel);
+ if (ret < 0)
+ return ret;
+ }
+ panel->prepared = true;
return 0;
}
@@ -128,11 +139,22 @@ EXPORT_SYMBOL(drm_panel_prepare);
*/
int drm_panel_unprepare(struct drm_panel *panel)
{
+ int ret;
+
if (!panel)
return -EINVAL;
- if (panel->funcs && panel->funcs->unprepare)
- return panel->funcs->unprepare(panel);
+ if (!panel->prepared) {
+ dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
+ return 0;
+ }
+
+ if (panel->funcs && panel->funcs->unprepare) {
+ ret = panel->funcs->unprepare(panel);
+ if (ret < 0)
+ return ret;
+ }
+ panel->prepared = false;
return 0;
}
@@ -155,11 +177,17 @@ int drm_panel_enable(struct drm_panel *panel)
if (!panel)
return -EINVAL;
+ if (panel->enabled) {
+ dev_warn(panel->dev, "Skipping enable of already enabled panel\n");
+ return 0;
+ }
+
if (panel->funcs && panel->funcs->enable) {
ret = panel->funcs->enable(panel);
if (ret < 0)
return ret;
}
+ panel->enabled = true;
ret = backlight_enable(panel->backlight);
if (ret < 0)
@@ -187,13 +215,22 @@ int drm_panel_disable(struct drm_panel *panel)
if (!panel)
return -EINVAL;
+ if (!panel->enabled) {
+ dev_warn(panel->dev, "Skipping disable of already disabled panel\n");
+ return 0;
+ }
+
ret = backlight_disable(panel->backlight);
if (ret < 0)
DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n",
ret);
- if (panel->funcs && panel->funcs->disable)
- return panel->funcs->disable(panel);
+ if (panel->funcs && panel->funcs->disable) {
+ ret = panel->funcs->disable(panel);
+ if (ret < 0)
+ return ret;
+ }
+ panel->enabled = false;
return 0;
}
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 432fab2347eb..c6cf75909389 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -198,6 +198,20 @@ struct drm_panel {
* the panel is powered up.
*/
bool prepare_prev_first;
+
+ /**
+ * @prepared:
+ *
+ * If true then the panel has been prepared.
+ */
+ bool prepared;
+
+ /**
+ * @enabled:
+ *
+ * If true then the panel has been enabled.
+ */
+ bool enabled;
};
void drm_panel_init(struct drm_panel *panel, struct device *dev,
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH 1/9] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed panels
From: Douglas Anderson @ 2023-05-23 19:27 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org>
As talked about in the patch ("drm/panel: Add a way for other devices
to follow panel state"), touchscreens that are connected to panels are
generally expected to be power sequenced together with the panel
they're attached to. Today, nothing provides information allowing you
to find out that a touchscreen is connected to a panel. Let's add a
phandle for this.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 6 ++++++
Documentation/devicetree/bindings/input/goodix,gt7375p.yaml | 6 ++++++
Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++
3 files changed, 18 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index 05e6f2df604c..d55b03bd3ec4 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -24,6 +24,12 @@ properties:
interrupts:
maxItems: 1
+ panel:
+ description: If this is a touchscreen, the panel it's connected to. This
+ indicates that the panel and touchscreen are expected to be power
+ sequenced together.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
reset-gpios:
description: Reset GPIO; not all touchscreens using eKTH6915 hook this up.
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
index ce18d7dadae2..a5cd8dafd450 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -30,6 +30,12 @@ properties:
interrupts:
maxItems: 1
+ panel:
+ description: If this is a touchscreen, the panel it's connected to. This
+ indicates that the panel and touchscreen are expected to be power
+ sequenced together.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
reset-gpios:
true
diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
index 7156b08f7645..c7ea6c148838 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
@@ -44,6 +44,12 @@ properties:
description: HID descriptor address
$ref: /schemas/types.yaml#/definitions/uint32
+ panel:
+ description: If this is a touchscreen, the panel it's connected to. This
+ indicates that the panel and touchscreen are expected to be power
+ sequenced together.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
post-power-on-delay-ms:
description: Time required by the device after enabling its regulators
or powering it on, before it is ready for communication.
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply related
* [PATCH 0/9] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Douglas Anderson @ 2023-05-23 19:27 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann
Cc: dri-devel, linux-input, Dmitry Torokhov, hsinyi, devicetree,
yangcong5, linux-kernel, Daniel Vetter, linux-arm-msm,
cros-qcom-dts-watchers, Douglas Anderson
The big motivation for this patch series is mostly described in the patch
("drm/panel: Add a way for other devices to follow panel state"), but to
quickly summarize here: for touchscreens that are connected to a panel we
need the ability to power sequence the two device together. This is not a
new need, but so far we've managed to get by through a combination of
inefficiency, added costs, or perhaps just a little bit of brokenness.
It's time to do better. This patch series allows us to do better.
Assuming that people think this patch series looks OK, we'll have to
figure out the right way to land it. The panel patches and i2c-hid
patches will go through very different trees and so either we'll need
an Ack from one side or the other or someone to create a tag for the
other tree to pull in. This will _probably_ require the true drm-misc
maintainers to get involved, not a lowly committer. ;-)
Douglas Anderson (9):
dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed
panels
drm/panel: Check for already prepared/enabled in drm_panel
drm/panel: Add a way for other devices to follow panel state
HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS()
HID: i2c-hid: Rearrange probe() to power things up later
HID: i2c-hid: Make suspend and resume into helper functions
HID: i2c-hid: Support being a panel follower
HID: i2c-hid: Do panel follower work on the system_wq
arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels
.../bindings/input/elan,ekth6915.yaml | 6 +
.../bindings/input/goodix,gt7375p.yaml | 6 +
.../bindings/input/hid-over-i2c.yaml | 6 +
.../boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 1 +
.../dts/qcom/sc7180-trogdor-homestar.dtsi | 1 +
.../boot/dts/qcom/sc7180-trogdor-lazor.dtsi | 1 +
.../boot/dts/qcom/sc7180-trogdor-pompom.dtsi | 1 +
.../qcom/sc7180-trogdor-quackingstick.dtsi | 1 +
.../dts/qcom/sc7180-trogdor-wormdingler.dtsi | 1 +
drivers/gpu/drm/drm_panel.c | 194 +++++++++-
drivers/hid/i2c-hid/i2c-hid-core.c | 330 +++++++++++++-----
include/drm/drm_panel.h | 89 +++++
12 files changed, 542 insertions(+), 95 deletions(-)
--
2.40.1.698.g37aff9b760-goog
^ permalink raw reply
* amd_sfh driver causes kernel oops during boot
From: Haochen Tong @ 2023-05-23 17:27 UTC (permalink / raw)
To: stable; +Cc: regressions, linux-input, Basavaraj Natikar
Hi,
Since kernel 6.3.0 (and also 6.4rc3), on a ThinkPad Z13 system with Arch
Linux, I've noticed that the amd_sfh driver spews a lot of stack traces
during boot. Sometimes it is an oops:
BUG: unable to handle page fault for address: 000000000001000f
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP NOPTI
CPU: 8 PID: 457 Comm: (udev-worker) Not tainted 6.3.3-arch1-1 #1
fa7b7e0107004b3021a57a74b951e0a25e7e8584
Hardware name: LENOVO 21D2CTO1WW/21D2CTO1WW, BIOS N3GET47W (1.27 )
12/08/2022
RIP: 0010:amd_sfh_get_report+0x1e/0x110 [amd_sfh]
Code: 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f 00 0f 1f 44 00 00 41
57 41 56 41 55 41 54 55 53 48 8b 87 60 1d 00 00 48 8b 68 08 <8b> 45 10
85 c0 0f 84 a9 00 00 00 49 89 fc 41 89 f7 41 89 d6 31 db
RSP: 0018:ffffb164426f3a20 EFLAGS: 00010246
RAX: ffff9b0ae6b7bd00 RBX: ffff9b0ac0f46000 RCX: 0000000000000000
RDX: 0000000000000002 RSI: 0000000000000002 RDI: ffff9b0ac0f46000
RBP: 000000000000ffff R08: ffffb164426f3ab8 R09: ffffb164426f3ab8
R10: 000000000020031b R11: ffff9b0ace40ac00 R12: ffff9b0ace40ac00
R13: 0000000000000002 R14: 0000000000000002 R15: ffff9b0acd213010
FS: 00007fe9ceb82200(0000) GS:ffff9b1122000000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000001000f CR3: 000000010940c000 CR4: 0000000000750ee0
PKRU: 55555554
Call Trace:
<TASK>
amdtp_hid_request+0x36/0x50 [amd_sfh
2e3095779aada9fdb1764f08ca578ccb14e41fe4]
sensor_hub_get_feature+0xad/0x170 [hid_sensor_hub
d6157999c9d260a1bfa6f27d4a0dc2c3e2c5654e]
hid_sensor_parse_common_attributes+0x217/0x310 [hid_sensor_iio_common
07a7935272aa9c7a28193b574580b3e953a64ec4]
hid_gyro_3d_probe+0x7f/0x2e0 [hid_sensor_gyro_3d
9f2eb51294a1f0c0315b365f335617cbaef01eab]
platform_probe+0x44/0xa0
really_probe+0x19e/0x3e0
? __pfx___driver_attach+0x10/0x10
__driver_probe_device+0x78/0x160
driver_probe_device+0x1f/0x90
__driver_attach+0xd2/0x1c0
bus_for_each_dev+0x88/0xd0
bus_add_driver+0x116/0x220
driver_register+0x59/0x100
? __pfx_init_module+0x10/0x10 [hid_sensor_gyro_3d
9f2eb51294a1f0c0315b365f335617cbaef01eab]
do_one_initcall+0x5d/0x240
do_init_module+0x4a/0x200
__do_sys_init_module+0x17f/0x1b0
do_syscall_64+0x60/0x90
? ksys_read+0x6f/0xf0
? syscall_exit_to_user_mode+0x1b/0x40
? do_syscall_64+0x6c/0x90
? exc_page_fault+0x7c/0x180
entry_SYSCALL_64_after_hwframe+0x72/0xdc
RIP: 0033:0x7fe9ce721f9e
Code: 48 8b 0d bd ed 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84
00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d 8a ed 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffd280dd828 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
RAX: ffffffffffffffda RBX: 000055b72a37f630 RCX: 00007fe9ce721f9e
RDX: 00007fe9cec7a343 RSI: 00000000000077f8 RDI: 000055b72a56c7f0
RBP: 00007fe9cec7a343 R08: 00000000000077f8 R09: 0000000000000000
R10: 000000000001a0a1 R11: 0000000000000246 R12: 0000000000020000
R13: 000055b72a363b90 R14: 000055b72a37f630 R15: 000055b72a36a070
</TASK>
Modules linked in: hid_sensor_accel_3d(+) hid_sensor_gyro_3d(+) qrtr
hid_sensor_trigger snd_sof industrialio_triggered_buffer ath11k_pci(+)
kfifo_buf snd_sof_utils hid_sensor_iio_common joydev ath11k industrialio
snd_soc_core mousedev qmi_helpers snd_compress hid_sensor_hub
snd_hda_scodec_cs35l41_spi ac97_bus snd_hda_codec_realtek(+)
snd_pcm_dmaengine intel_rapl_msr snd_hda_codec_hdmi
snd_hda_codec_generic intel_rapl_common mac80211 snd_pci_ps btusb
snd_rpl_pci_acp6x btrtl snd_hda_intel edac_mce_amd uvcvideo btbcm
snd_acp_pci snd_intel_dspcfg snd_pci_acp6x videobuf2_vmalloc
snd_intel_sdw_acpi libarc4 uvc btintel snd_usb_audio(+) snd_pci_acp5x
videobuf2_memops btmtk snd_hda_codec kvm_amd videobuf2_v4l2
snd_hda_scodec_cs35l41_i2c snd_usbmidi_lib snd_hda_scodec_cs35l41
snd_rn_pci_acp3x ucsi_acpi bluetooth videodev snd_hda_core typec_ucsi
snd_acp_config snd_hda_cs_dsp_ctls wacom(+) hid_multitouch cfg80211
snd_rawmidi sp5100_tco kvm snd_seq_device cs_dsp videobuf2_common typec
ecdh_generic snd_soc_acpi
think_lmi snd_hwdep snd_pcm irqbypass crc16 snd_soc_cs35l41_lib mhi
thunderbolt firmware_attributes_class snd_pci_acp3x amd_sfh(+) k10temp
psmouse roles rapl i2c_piix4 mc snd_timer wmi_bmof
serial_multi_instantiate i2c_hid_acpi acpi_tad i2c_hid amd_pmf amd_pmc
mac_hid sch_fq tcp_bbr dm_multipath i2c_dev crypto_user fuse loop zram
ip_tables x_tables xfs libcrc32c crc32c_generic dm_crypt cbc
encrypted_keys trusted asn1_encoder tee usbhid dm_mod amdgpu
i2c_algo_bit serio_raw thinkpad_acpi drm_ttm_helper atkbd libps2
crct10dif_pclmul vivaldi_fmap crc32_pclmul ledtrig_audio crc32c_intel
polyval_clmulni ttm polyval_generic drm_buddy nvme gf128mul
platform_profile gpu_sched ghash_clmulni_intel sha512_ssse3 snd
aesni_intel soundcore drm_display_helper crypto_simd rfkill nvme_core
xhci_pci cryptd cec ccp xhci_pci_renesas i8042 video nvme_common serio wmi
CR2: 000000000001000f
---[ end trace 0000000000000000 ]---
RIP: 0010:amd_sfh_get_report+0x1e/0x110 [amd_sfh]
Code: 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f 00 0f 1f 44 00 00 41
57 41 56 41 55 41 54 55 53 48 8b 87 60 1d 00 00 48 8b 68 08 <8b> 45 10
85 c0 0f 84 a9 00 00 00 49 89 fc 41 89 f7 41 89 d6 31 db
RSP: 0018:ffffb164426f3a20 EFLAGS: 00010246
RAX: ffff9b0ae6b7bd00 RBX: ffff9b0ac0f46000 RCX: 0000000000000000
RDX: 0000000000000002 RSI: 0000000000000002 RDI: ffff9b0ac0f46000
RBP: 000000000000ffff R08: ffffb164426f3ab8 R09: ffffb164426f3ab8
R10: 000000000020031b R11: ffff9b0ace40ac00 R12: ffff9b0ace40ac00
R13: 0000000000000002 R14: 0000000000000002 R15: ffff9b0acd213010
FS: 00007fe9ceb82200(0000) GS:ffff9b1122000000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000001000f CR3: 000000010940c000 CR4: 0000000000750ee0
PKRU: 55555554
Sometimes it is a list corruption in the same function with a similar stack:
------------[ cut here ]------------
list_add corruption. next is NULL.
WARNING: CPU: 5 PID: 433 at lib/list_debug.c:25 __list_add_valid+0x57/0xa0
...
CPU: 5 PID: 433 Comm: (udev-worker) Not tainted 6.4.0-rc3-1-mainline #1
b60166e85cb97a6631db26f9dcda0196ed7a0c93
Hardware name: LENOVO 21D2CTO1WW/21D2CTO1WW, BIOS N3GET47W (1.27 )
12/08/2022
RIP: 0010:__list_add_valid+0x57/0xa0
Code: 01 00 00 00 c3 cc cc cc cc 48 c7 c7 58 91 e6 9a e8 1e b9 a8 ff 0f
0b 31 c0 c3 cc cc cc cc 48 c7 c7 80 91 e6 9a e8 09 b9 a8 ff <0f> 0b eb
e9 48 89 c1 48 c7 c7 a8 91 e6 9a e8 f6 b8 a8 ff 0f 0b eb
RSP: 0018:ffffad9dc0c7bb10 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff92d5a8099448 RCX: 0000000000000027
RDX: ffff92dbe1f61688 RSI: 0000000000000001 RDI: ffff92dbe1f61680
RBP: ffff92d59ea93508 R08: 0000000000000000 R09: ffffad9dc0c7b9a0
R10: 0000000000000003 R11: ffffffff9b6ca808 R12: 0000000000000000
R13: ffff92d5a8099440 R14: ffff92d59ea93760 R15: 0000000000000002
FS: 00007fbaf0262200(0000) GS:ffff92dbe1f40000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005651de666000 CR3: 000000011cfee000 CR4: 0000000000750ee0
PKRU: 55555554
Call Trace:
<TASK>
amd_sfh_get_report+0xba/0x110 [amd_sfh
78bf82e66cdb2ccf24cbe871a0835ef4eedddb17]
amdtp_hid_request+0x36/0x50 [amd_sfh
78bf82e66cdb2ccf24cbe871a0835ef4eedddb17]
sensor_hub_get_feature+0xad/0x170 [hid_sensor_hub
30e53e2c49ea1702e2482c0b3860e22265679e39]
hid_sensor_parse_common_attributes+0x217/0x310 [hid_sensor_iio_common
ed7fba7a4d4147d48156e6a4b2a034ad3fc94350]
hid_gyro_3d_probe+0x7f/0x2e0 [hid_sensor_gyro_3d
10978a2cdfc8979f2a7366fcd005e0ea826088eb]
platform_probe+0x44/0xa0
really_probe+0x19e/0x3e0
? __pfx___driver_attach+0x10/0x10
__driver_probe_device+0x78/0x160
driver_probe_device+0x1f/0x90
__driver_attach+0xd2/0x1c0
bus_for_each_dev+0x88/0xd0
bus_add_driver+0x116/0x220
driver_register+0x59/0x100
? __pfx_hid_gyro_3d_platform_driver_init+0x10/0x10 [hid_sensor_gyro_3d
10978a2cdfc8979f2a7366fcd005e0ea826088eb]
do_one_initcall+0x5d/0x240
do_init_module+0x60/0x240
__do_sys_init_module+0x17f/0x1b0
do_syscall_64+0x60/0x90
? exc_page_fault+0x7f/0x180
entry_SYSCALL_64_after_hwframe+0x72/0xdc
RIP: 0033:0x7fbaf06c0f9e
Code: 48 8b 0d bd ed 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84
00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01
f0 ff ff 73 01 c3 48 8b 0d 8a ed 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffc5ce88528 EFLAGS: 00000246 ORIG_RAX: 00000000000000af
RAX: ffffffffffffffda RBX: 00005651de36dff0 RCX: 00007fbaf06c0f9e
RDX: 00007fbaf0ba9343 RSI: 00000000000079f0 RDI: 00005651de646fe0
RBP: 00007fbaf0ba9343 R08: 00000000000079f0 R09: 0000000000000000
R10: 0000000000019fb1 R11: 0000000000000246 R12: 0000000000020000
R13: 00005651de45fb10 R14: 00005651de36dff0 R15: 00005651de44d5f0
</TASK>
---[ end trace 0000000000000000 ]---
This occurs during almost every boot. When it happens there is usually a
(udev-worker) process lingering forever, which is unkillable and even
prevents shutdown.
Looking at past journals it never happened before 6.3 so I believe it is
a regression.
Relevant device:
63:00.7 Signal processing controller [1180]: Advanced Micro Devices,
Inc. [AMD] Sensor Fusion Hub [1022:15e4]
Subsystem: Lenovo Sensor Fusion Hub [17aa:22f1]
Kernel driver in use: pcie_mp2_amd
Kernel modules: amd_sfh
I would appreciate it if someone could take a look at this.
Best regards,
Haochen Tong
^ permalink raw reply
* [PATCH] hid-wiimote-debug.c: Fix error checking for debugfs_create_file
From: Osama Muhammad @ 2023-05-23 17:12 UTC (permalink / raw)
To: david.rheinsberg, benjamin.tissoires
Cc: linux-input, linux-kernel, Osama Muhammad
This patch fixes the error checking in hid-wiimote-debug.c in
debugfs_create_file. The correct way to check if an error occurred
is 'IS_ERR' inline function.
Signed-off-by: Osama Muhammad <osmtendev@gmail.com>
---
drivers/hid/hid-wiimote-debug.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-wiimote-debug.c b/drivers/hid/hid-wiimote-debug.c
index a99dcca2e099..eddd981fee1a 100644
--- a/drivers/hid/hid-wiimote-debug.c
+++ b/drivers/hid/hid-wiimote-debug.c
@@ -183,12 +183,12 @@ int wiidebug_init(struct wiimote_data *wdata)
dbg->eeprom = debugfs_create_file("eeprom", S_IRUSR,
dbg->wdata->hdev->debug_dir, dbg, &wiidebug_eeprom_fops);
- if (!dbg->eeprom)
+ if (IS_ERR(dbg->eeprom))
goto err;
dbg->drm = debugfs_create_file("drm", S_IRUSR,
dbg->wdata->hdev->debug_dir, dbg, &wiidebug_drm_fops);
- if (!dbg->drm)
+ if (IS_ERR(dbg->drm))
goto err_drm;
spin_lock_irqsave(&wdata->state.lock, flags);
--
2.34.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox