* Re: [PATCH v3 02/10] drm/panel: Check for already prepared/enabled in drm_panel
From: Maxime Ripard @ 2023-07-26 12:41 UTC (permalink / raw)
To: Douglas Anderson
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, cros-qcom-dts-watchers, Chris Morgan,
linux-input, hsinyi, linux-arm-msm, dri-devel, linux-kernel,
Dmitry Torokhov, devicetree, Daniel Vetter, yangcong5
In-Reply-To: <20230725133443.v3.2.I59b417d4c29151cc2eff053369ec4822b606f375@changeid>
[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]
Hi,
On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> 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.
Yeah, I'd agree here too. I've never found evidence that it was actually
needed and it really looks like cargo cult to me.
And if it was needed, then I'm not sure we need refcounting either. We
don't have refcounting for atomic_enable / disable, we have a sound API
design that makes sure we don't fall into that trap :)
> 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.
I'm ok with this, if we follow-up in a couple of releases and remove it
and all the calls.
Could you add a TODO item so that we can keep a track of it? A follow-up
is fine if you don't send a new version of that series.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3 08/10] HID: i2c-hid: Support being a panel follower
From: Benjamin Tissoires @ 2023-07-26 8:57 UTC (permalink / raw)
To: Douglas Anderson
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, devicetree, cros-qcom-dts-watchers,
linux-arm-msm, yangcong5, Dmitry Torokhov, linux-kernel,
dri-devel, Chris Morgan, linux-input, hsinyi
In-Reply-To: <20230725133443.v3.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15@changeid>
On Jul 25 2023, Douglas Anderson wrote:
> As talked about in the patch ("drm/panel: Add a way for other devices
> to follow panel state"), we really want to keep the power states of a
> touchscreen and the panel it's attached to in sync with each other. In
> that spirit, add support to i2c-hid to be a panel follower. This will
> let the i2c-hid driver get informed when the panel is powered on and
> off. From there we can match the i2c-hid device's power state to that
> of the panel.
>
> NOTE: this patch specifically _doesn't_ use pm_runtime to keep track
> of / manage the power state of the i2c-hid device, even though my
> first instinct said that would be the way to go. Specific problems
> with using pm_runtime():
> * The initial power up couldn't happen in a runtime resume function
> since it create sub-devices and, apparently, that's not good to do
> in your resume function.
> * Managing our power state with pm_runtime meant fighting to make the
> right thing happen at system suspend to prevent the system from
> trying to resume us only to suspend us again. While this might be
> able to be solved, it added complexity.
> Overall the code without pm_runtime() ended up being smaller and
> easier to understand.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v3:
> - Add "depends on DRM || !DRM" to Kconfig to avoid randconfig error.
> - Split more of the panel follower code out of the core.
>
> Changes in v2:
> - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
>
> drivers/hid/i2c-hid/Kconfig | 2 +
> drivers/hid/i2c-hid/i2c-hid-core.c | 82 +++++++++++++++++++++++++++++-
> 2 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
> index 3be17109301a..2bdb55203104 100644
> --- a/drivers/hid/i2c-hid/Kconfig
> +++ b/drivers/hid/i2c-hid/Kconfig
> @@ -70,5 +70,7 @@ config I2C_HID_OF_GOODIX
>
> config I2C_HID_CORE
> tristate
> + # We need to call into panel code so if DRM=m, this can't be 'y'
> + depends on DRM || !DRM
> endif
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index fa8a1ca43d7f..fa6d1f624342 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -38,6 +38,8 @@
> #include <linux/mutex.h>
> #include <asm/unaligned.h>
>
> +#include <drm/drm_panel.h>
> +
> #include "../hid-ids.h"
> #include "i2c-hid.h"
>
> @@ -107,6 +109,8 @@ struct i2c_hid {
> struct mutex reset_lock;
>
> struct i2chid_ops *ops;
> + struct drm_panel_follower panel_follower;
> + bool is_panel_follower;
> };
>
> static const struct i2c_hid_quirks {
> @@ -1058,6 +1062,59 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
> return ret;
> }
>
> +static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
> +{
> + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
> + struct hid_device *hid = ihid->hid;
> +
> + /*
> + * hid->version is set on the first power up. If it's still zero then
> + * this is the first power on so we should perform initial power up
> + * steps.
> + */
> + if (!hid->version)
> + return i2c_hid_core_initial_power_up(ihid);
> +
> + return i2c_hid_core_resume(ihid);
> +}
> +
> +static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower)
> +{
> + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
> +
> + return i2c_hid_core_suspend(ihid);
> +}
> +
> +static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = {
> + .panel_prepared = i2c_hid_core_panel_prepared,
> + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> +};
> +
> +static int i2c_hid_core_register_panel_follower(struct i2c_hid *ihid)
> +{
> + struct device *dev = &ihid->client->dev;
> + int ret;
> +
> + ihid->is_panel_follower = true;
> + ihid->panel_follower.funcs = &i2c_hid_core_panel_follower_funcs;
> +
> + /*
> + * If we're not in control of our own power up/power down then we can't
> + * do the logic to manage wakeups. Give a warning if a user thought
> + * that was possible then force the capability off.
> + */
> + if (device_can_wakeup(dev)) {
> + dev_warn(dev, "Can't wakeup if following panel\n");
> + device_set_wakeup_capable(dev, false);
> + }
> +
> + ret = drm_panel_add_follower(dev, &ihid->panel_follower);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> u16 hid_descriptor_address, u32 quirks)
> {
> @@ -1119,7 +1176,15 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> hid->bus = BUS_I2C;
> hid->initial_quirks = quirks;
>
> - ret = i2c_hid_core_initial_power_up(ihid);
> + /*
> + * If we're a panel follower, we'll register and do our initial power
> + * up when the panel turns on; otherwise we do it right away.
> + */
> + if (drm_is_panel_follower(&client->dev))
> + ret = i2c_hid_core_register_panel_follower(ihid);
> + else
> + ret = i2c_hid_core_initial_power_up(ihid);
nitpicks, but I'm not sure I'm a big fan of having
"if (drm_is_panel_follower(&client->dev))" sprinkled everywhere in the
generic probe/resume/suspend code.
Would it be OK to define a `static int __do_i2c_hid_core_initial_power_up(struct i2c_hid *ihid)`
that would do the actual powering up, and have
i2c_hid_core_initial_power_up() doing the test if we are a panel
follower?
The i2c_hid_core_panel_* will need to be updated to use the `__do_`
prefixed functions.
> +
> if (ret)
> goto err_mem_free;
>
> @@ -1143,7 +1208,14 @@ void i2c_hid_core_remove(struct i2c_client *client)
> struct i2c_hid *ihid = i2c_get_clientdata(client);
> struct hid_device *hid;
>
> - i2c_hid_core_power_down(ihid);
> + /*
> + * If we're a follower, the act of unfollowing will cause us to be
> + * powered down. Otherwise we need to manually do it.
> + */
> + if (ihid->is_panel_follower)
> + drm_panel_remove_follower(&ihid->panel_follower);
That part is concerning, as we are now calling hid_drv->suspend() when removing
the device. It might or not have an impact (I'm not sure of it), but we
are effectively changing the path of commands sent to the device.
hid-multitouch might call a feature in ->suspend, but the remove makes
that the physical is actually disconnected, so the function will fail,
and I'm not sure what is happening then.
> + else
> + i2c_hid_core_power_down(ihid);
Same here, I *think* it would be best to have the `if (ihid->is_panel_follower)`
test in i2c_hid_core_power_down() (and have a separate
_do_i2c_hid_core_power_down()).
>
> hid = ihid->hid;
> hid_destroy_device(hid);
> @@ -1171,6 +1243,9 @@ static int i2c_hid_core_pm_suspend(struct device *dev)
> struct i2c_client *client = to_i2c_client(dev);
> struct i2c_hid *ihid = i2c_get_clientdata(client);
>
> + if (ihid->is_panel_follower)
> + return 0;
Not sure we need to split that one with _do_ prefix, it's already split
:)
> +
> return i2c_hid_core_suspend(ihid);
> }
>
> @@ -1179,6 +1254,9 @@ static int i2c_hid_core_pm_resume(struct device *dev)
> struct i2c_client *client = to_i2c_client(dev);
> struct i2c_hid *ihid = i2c_get_clientdata(client);
>
> + if (ihid->is_panel_follower)
> + return 0;
Same here, no need to split.
> +
> return i2c_hid_core_resume(ihid);
> }
>
> --
> 2.41.0.487.g6d72f3e995-goog
>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
From: Benjamin Tissoires @ 2023-07-26 8:07 UTC (permalink / raw)
To: Doug Anderson
Cc: Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
In-Reply-To: <CAD=FV=WmpH6cB0oZOxbL+TUxjLRf3PM+kKvuYRAZSiEhS4o37A@mail.gmail.com>
On Jul 25 2023, Doug Anderson wrote:
> Hi,
>
> On Mon, Jul 17, 2023 at 11:15 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Benjamin,
> >
> > On Mon, Jun 26, 2023 at 3:49 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Benjamin,
> > >
> > > On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > >
> > > > > +static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = {
> > > > > + .panel_prepared = i2c_hid_core_panel_prepared,
> > > > > + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> > > > > +};
> > > >
> > > > Can we make that above block at least behind a Kconfig?
> > > >
> > > > i2c-hid is often used for touchpads, and the notion of drm panel has
> > > > nothing to do with them. So I'd be more confident if we could disable
> > > > that code if not required.
> > >
> > > Now that other concerns are addressed, I started trying to write up a
> > > v3 and I found myself writing this as the description of the Kconfig
> > > entry:
> > >
> > > --
> > > config I2C_HID_SUPPORT_PANEL_FOLLOWER
> > > bool "Support i2c-hid devices that must be power sequenced with a panel"
> > >
> > > Say Y here if you want support for i2c-hid devices that need to
> > > coordinate power sequencing with a panel. This is typically important
> > > when you have a panel and a touchscreen that share power rails or
> > > reset GPIOs. If you say N here then the kernel will not try to honor
> > > any shared power sequencing for your hardware. In the best case,
> > > ignoring power sequencing when it's needed will draw extra power. In
> > > the worst case this will prevent your hardware from functioning or
> > > could even damage your hardware.
> > >
> > > If unsure, say Y.
> > >
> > > --
> > >
> > > I can certainly go that way, but I just wanted to truly make sure
> > > that's what we want. Specifically:
> > >
> > > 1. If we put the panel follower code behind a Kconfig then we actually
> > > have no idea if a touchscreen was intended to be a panel follower.
> > > Specifically the panel follower API is the one that detects the
> > > connection between the panel and the i2c-hid device, so without being
> > > able to call the panel follower API we have no idea that an i2c-hid
> > > device was supposed to be a panel follower.
> > >
> > > 2. It is conceivable that power sequencing a device incorrectly could
> > > truly cause hardware damage.
> > >
> > > Together, those points mean that if you turn off the Kconfig entry and
> > > then try to boot on a device that needed that Kconfig setting that you
> > > might damage hardware. I can code it up that way if you want, but it
> > > worries me...
> > >
> > >
> > > Alternatives that I can think of:
> > >
> > > a) I could change the panel follower API so that panel followers are
> > > in charge of detecting the panel that they follow. Today, that looks
> > > like:
> > >
> > > panel_np = of_parse_phandle(dev->of_node, "panel", 0);
> > > if (panel_np)
> > > /* It's a panel follower */
> > > of_node_put(panel_np);
> > >
> > > ...so we could put that code in each touchscreen driver and then fail
> > > to probe i2c-hid if we detect that we're supposed to be a panel
> > > follower but the Kconfig is turned off. The above doesn't seem
> > > massively ideal since it duplicates code. Also, one reason why I put
> > > that code in drm_panel_add_follower() is that I think this concept
> > > will eventually be needed even for non-DT cases. I don't know how to
> > > write the non-DT code right now, though...
> > >
> > >
> > > b) I could open-code detect the panel follower case but leave the
> > > actual linking to the panel follower API. AKA add to i2c-hid:
> > >
> > > if (of_property_read_bool(dev->of_node, "panel"))
> > > /* It's a panel follower */
> > >
> > > ...that's a smaller bit of code, but feels like an abstraction
> > > violation. It also would need to be updated if/when we added support
> > > for non-DT panel followers.
> > >
> > >
> > > c) I could add a "static inline" implementation of b) to "drm_panel.h".
> > >
> > > That sounds great and I started doing it. ...but then realized that it
> > > means adding to drm_panel.h:
> > >
> > > #include <linux/device.h>
> > > #include <linux/of.h>
> > >
> > > ...because otherwise of_property_read_bool() isn't defined and "struct
> > > device" can't be dereferenced. That might be OK, but it looks as if
> > > folks have been working hard to avoid things like this in header
> > > files. Presumably it would get uglier if/when we added the non-DT
> > > case, as well. That being said, I can give it a shot...
> > >
> > > --
> > >
> > > At this point, I'm hoping for some advice. How important is it for you
> > > to have a Kconfig for "I2C_HID_SUPPORT_PANEL_FOLLOWER"?
> > >
> > > NOTE: even if I don't add the Kconfig, I could at least create a
> > > function for registering the panel follower that would get most of the
> > > panel follower logic out of the probe function. Would that be enough?
> >
> > I'd love to send a new version of this patch series, but I'm still
> > stuck with the above issue. I'm hoping you might have a minute to
> > provide your thoughts. If I don't hear anything, I'll try a v3 where I
> > don't have the Kconfig for "I2C_HID_SUPPORT_PANEL_FOLLOWER" but just
> > try to pull a little more of the code out of the probe function.
>
> To provide breadcrumbs, I posted the v3 which pulls a bit more code
> out of the probe function but is otherwise largely unchanged. The
> cover letter for v3 can be found at:
Apologies for the delay. Given that you received feedbacks from other
folks I wanted things to settle down a little bit before returning to
this discussion. Sorry.
>
> https://lore.kernel.org/r/20230725203545.2260506-1-dianders@chromium.org/
I like the 8th patch of this series much more. If there is a risk of
damaging the device, then we should not have the Kconfig to disable it.
I have some comments on that particular patch (v3 8/10), but I; ll reply
inline.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH v4] Input: psmouse - add delay when deactivating for SMBus mode
From: Dmitry Torokhov @ 2023-07-26 5:13 UTC (permalink / raw)
To: Jeffery Miller
Cc: Lyude Paul, Jonathan Denose, jdenose, Andrew Duggan,
Andrew Duggan, loic.poulain, benjamin.tissoires, linux-input,
linux-kernel
In-Reply-To: <20230726025256.81174-1-jefferymiller@google.com>
On Wed, Jul 26, 2023 at 02:52:48AM +0000, Jeffery Miller wrote:
> There is a period of time between the psmouse deactivate and the
> ability to communicate with the SMBus companion. Insert a
> sleep after the deactivate to account for the delay and ensure
> the SMBus companion is responsive.
>
> Attempting to read from the SMBus companion too quickly was causing
> the touchpad on machines with an i801_smbus companion to stop working
> after a sleep/resume cycle.
>
> On resume the rmi4_smbus would fail with errors reading the SMBus version
> number:
> ```
> [5454] i2c_i801:i801_check_post:414: i801_smbus 0000:00:1f.3: No response
> smbus_result: i2c-0 a=02c f=0000 c=fd BYTE_DATA rd res=-6
> rmi4_smbus 0-002c: failed to get SMBus version number!
> ...
> rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6.
> rmi4_f01 rmi4-00.fn01: Resume failed with code -6.
> rmi4_physical rmi4-00: Failed to suspend functions: -6
> rmi4_smbus 0-002c: Failed to resume device: -6
> ```
> In this case the rmi_smb_get_version fails with -ENXIO if it happens too
> soon after the preceding serio_resume -> psmouse_deactivate call.
>
> On boot this issue could cause the touchpad to stay in the limited PS/2
> mode. This only reproduced in 1 in 10 boots on the Lenovo T440p.
> Failures in the log on boot would show up as:
> ```
> psmouse serio1: synaptics: Trying to set up SMBus access
> [122] i2c_i801:i801_check_post:437: i801_smbus 0000:00:1f.3: No response
> psmouse serio1: synaptics: SMbus companion is not ready yet
> ```
>
> Experimentation on the Lenovo T440p showed that a delay of 7-12ms on
> resume allowed the companion to respond.
>
> The 30ms delay in this patch was chosen based on the linux-input message:
> Link: https://lore.kernel.org/all/BYAPR03MB47572F2C65E52ED673238D41B2439@BYAPR03MB4757.namprd03.prod.outlook.com/
>
> Signed-off-by: Jeffery Miller <jefferymiller@google.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH v4] Input: psmouse - add delay when deactivating for SMBus mode
From: Jeffery Miller @ 2023-07-26 2:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Lyude Paul, Jonathan Denose, jdenose, Andrew Duggan,
Andrew Duggan, loic.poulain, benjamin.tissoires, Jeffery Miller,
linux-input, linux-kernel
There is a period of time between the psmouse deactivate and the
ability to communicate with the SMBus companion. Insert a
sleep after the deactivate to account for the delay and ensure
the SMBus companion is responsive.
Attempting to read from the SMBus companion too quickly was causing
the touchpad on machines with an i801_smbus companion to stop working
after a sleep/resume cycle.
On resume the rmi4_smbus would fail with errors reading the SMBus version
number:
```
[5454] i2c_i801:i801_check_post:414: i801_smbus 0000:00:1f.3: No response
smbus_result: i2c-0 a=02c f=0000 c=fd BYTE_DATA rd res=-6
rmi4_smbus 0-002c: failed to get SMBus version number!
...
rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6.
rmi4_f01 rmi4-00.fn01: Resume failed with code -6.
rmi4_physical rmi4-00: Failed to suspend functions: -6
rmi4_smbus 0-002c: Failed to resume device: -6
```
In this case the rmi_smb_get_version fails with -ENXIO if it happens too
soon after the preceding serio_resume -> psmouse_deactivate call.
On boot this issue could cause the touchpad to stay in the limited PS/2
mode. This only reproduced in 1 in 10 boots on the Lenovo T440p.
Failures in the log on boot would show up as:
```
psmouse serio1: synaptics: Trying to set up SMBus access
[122] i2c_i801:i801_check_post:437: i801_smbus 0000:00:1f.3: No response
psmouse serio1: synaptics: SMbus companion is not ready yet
```
Experimentation on the Lenovo T440p showed that a delay of 7-12ms on
resume allowed the companion to respond.
The 30ms delay in this patch was chosen based on the linux-input message:
Link: https://lore.kernel.org/all/BYAPR03MB47572F2C65E52ED673238D41B2439@BYAPR03MB4757.namprd03.prod.outlook.com/
Signed-off-by: Jeffery Miller <jefferymiller@google.com>
---
Early boot dmesg include:
```
rmi4_smbus 0-002c: registering SMbus-connected sensor
rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: Synaptics, product: TM2722-001, fw id: 0
```
The resume order looks correct. The `psmouse serio1` resume returns
before the rmi_smb_resume is called showing the patch from
https://lore.kernel.org/all/89456fcd-a113-4c82-4b10-a9bcaefac68f@google.com/
is applied and working for that ordering.
I attempted to try to rule out some interaction between the concurrent
input resume calls for other i8042 devices.
Adding a 7ms delay after psmouse_deactivate which is called in the
preceding psmouse serio1 serio_resume function also allows
this version call to succeed.
If the rmi_smb_probe device_disable_async_suspend patch is applied
it will also avoid this issue on resume. However the time between
the psmouse_deactivate call for serio_resume and rmi_smb_resume
was over 60ms on my test machine. This would naturally be long
enough to avoid this particular delay.
Changes in v4:
- Moved the delay out of rmi4_smbus to psmouse_smbus based on review
comments. This also fixes an issue at boot time.
Changes in v3:
- Tagged mail message Link to resolve checkpatch warning.
Changes in v2:
- Changed to a single retry of 30ms based on previous feedback.
drivers/input/mouse/psmouse-smbus.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/input/mouse/psmouse-smbus.c b/drivers/input/mouse/psmouse-smbus.c
index 2a2459b1b4f2..7b13de979908 100644
--- a/drivers/input/mouse/psmouse-smbus.c
+++ b/drivers/input/mouse/psmouse-smbus.c
@@ -5,6 +5,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/delay.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/libps2.h>
@@ -118,13 +119,18 @@ static psmouse_ret_t psmouse_smbus_process_byte(struct psmouse *psmouse)
return PSMOUSE_FULL_PACKET;
}
-static int psmouse_smbus_reconnect(struct psmouse *psmouse)
+static void psmouse_activate_smbus_mode(struct psmouse_smbus_dev *smbdev)
{
- struct psmouse_smbus_dev *smbdev = psmouse->private;
-
- if (smbdev->need_deactivate)
- psmouse_deactivate(psmouse);
+ if (smbdev->need_deactivate) {
+ psmouse_deactivate(smbdev->psmouse);
+ /* Give the device time to switch into SMBus mode */
+ msleep(30);
+ }
+}
+static int psmouse_smbus_reconnect(struct psmouse *psmouse)
+{
+ psmouse_activate_smbus_mode(psmouse->private);
return 0;
}
@@ -257,8 +263,7 @@ int psmouse_smbus_init(struct psmouse *psmouse,
}
}
- if (need_deactivate)
- psmouse_deactivate(psmouse);
+ psmouse_activate_smbus_mode(smbdev);
psmouse->private = smbdev;
psmouse->protocol_handler = psmouse_smbus_process_byte;
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH Wacom EKR Battery Fixes 1/3] HID: wacom: remove the battery when the EKR is off
From: Aaron Armstrong Skomra @ 2023-07-25 22:20 UTC (permalink / raw)
To: linux-input, linux-kernel, benjamin.tissoires, jikos
Cc: pinglinux, skomra, Joshua, jason.gerecke, stable,
Aaron Armstrong Skomra
From: Aaron Armstrong Skomra <aaron.skomra@wacom.com>
Currently the EKR battery remains even after we stop getting information
from the device. This can lead to a stale battery persisting indefinitely
in userspace.
The remote sends a heartbeat every 10 seconds. Delete the battery if we
miss two heartbeats (after 21 seconds). Restore the battery once we see
a heartbeat again.
Signed-off-by: Aaron Skomra <skomra@gmail.com>
Signed-off-by: Aaron Armstrong Skomra <aaron.skomra@wacom.com>
Reviewed-by: Jason Gerecke <jason.gerecke@wacom.com>
Fixes: 9f1015d45f62 ("HID: wacom: EKR: attach the power_supply on first connection")
CC: stable@vger.kernel.org
---
drivers/hid/wacom.h | 1 +
drivers/hid/wacom_sys.c | 25 +++++++++++++++++++++----
drivers/hid/wacom_wac.c | 1 +
drivers/hid/wacom_wac.h | 1 +
4 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h
index 4da50e19808e..166a76c9bcad 100644
--- a/drivers/hid/wacom.h
+++ b/drivers/hid/wacom.h
@@ -150,6 +150,7 @@ struct wacom_remote {
struct input_dev *input;
bool registered;
struct wacom_battery battery;
+ ktime_t active_time;
} remotes[WACOM_MAX_REMOTES];
};
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 76e5353aca0c..eb833455abd5 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2523,6 +2523,18 @@ static void wacom_wireless_work(struct work_struct *work)
return;
}
+static void wacom_remote_destroy_battery(struct wacom *wacom, int index)
+{
+ struct wacom_remote *remote = wacom->remote;
+
+ if (remote->remotes[index].battery.battery) {
+ devres_release_group(&wacom->hdev->dev,
+ &remote->remotes[index].battery.bat_desc);
+ remote->remotes[index].battery.battery = NULL;
+ remote->remotes[index].active_time = 0;
+ }
+}
+
static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
{
struct wacom_remote *remote = wacom->remote;
@@ -2537,9 +2549,7 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
remote->remotes[i].registered = false;
spin_unlock_irqrestore(&remote->remote_lock, flags);
- if (remote->remotes[i].battery.battery)
- devres_release_group(&wacom->hdev->dev,
- &remote->remotes[i].battery.bat_desc);
+ wacom_remote_destroy_battery(wacom, i);
if (remote->remotes[i].group.name)
devres_release_group(&wacom->hdev->dev,
@@ -2547,7 +2557,6 @@ static void wacom_remote_destroy_one(struct wacom *wacom, unsigned int index)
remote->remotes[i].serial = 0;
remote->remotes[i].group.name = NULL;
- remote->remotes[i].battery.battery = NULL;
wacom->led.groups[i].select = WACOM_STATUS_UNKNOWN;
}
}
@@ -2632,6 +2641,9 @@ static int wacom_remote_attach_battery(struct wacom *wacom, int index)
if (remote->remotes[index].battery.battery)
return 0;
+ if (!remote->remotes[index].active_time)
+ return 0;
+
if (wacom->led.groups[index].select == WACOM_STATUS_UNKNOWN)
return 0;
@@ -2647,6 +2659,7 @@ static void wacom_remote_work(struct work_struct *work)
{
struct wacom *wacom = container_of(work, struct wacom, remote_work);
struct wacom_remote *remote = wacom->remote;
+ ktime_t kt = ktime_get();
struct wacom_remote_data data;
unsigned long flags;
unsigned int count;
@@ -2673,6 +2686,10 @@ static void wacom_remote_work(struct work_struct *work)
serial = data.remote[i].serial;
if (data.remote[i].connected) {
+ if (kt - remote->remotes[i].active_time > WACOM_REMOTE_BATTERY_TIMEOUT
+ && remote->remotes[i].active_time != 0)
+ wacom_remote_destroy_battery(wacom, i);
+
if (remote->remotes[i].serial == serial) {
wacom_remote_attach_battery(wacom, i);
continue;
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 174bf03908d7..6c056f8844e7 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1134,6 +1134,7 @@ static int wacom_remote_irq(struct wacom_wac *wacom_wac, size_t len)
if (index < 0 || !remote->remotes[index].registered)
goto out;
+ remote->remotes[i].active_time = ktime_get();
input = remote->remotes[index].input;
input_report_key(input, BTN_0, (data[9] & 0x01));
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index ee21bb260f22..2e7cc5e7a0cb 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -13,6 +13,7 @@
#define WACOM_NAME_MAX 64
#define WACOM_MAX_REMOTES 5
#define WACOM_STATUS_UNKNOWN 255
+#define WACOM_REMOTE_BATTERY_TIMEOUT 21000000000ll
/* packet length for individual models */
#define WACOM_PKGLEN_BBFUN 9
--
2.34.1
^ permalink raw reply related
* [PATCH Wacom EKR Battery Fixes 2/3] HID: wacom: remove unnecessary 'connected' variable from EKR
From: Aaron Armstrong Skomra @ 2023-07-25 22:20 UTC (permalink / raw)
To: linux-input, linux-kernel, benjamin.tissoires, jikos
Cc: pinglinux, skomra, Joshua, jason.gerecke, Aaron Armstrong Skomra
From: Aaron Armstrong Skomra <aaron.skomra@wacom.com>
The 'connected' variable was poorly named, and this has led to some
confusion. We can get the same information by checking if a serial number
exists in the specified EKR slot.
Signed-off-by: Aaron Skomra <skomra@gmail.com>
Signed-off-by: Aaron Armstrong Skomra <aaron.skomra@wacom.com>
Reviewed-by: Jason Gerecke <jason.gerecke@wacom.com>
---
drivers/hid/wacom_sys.c | 2 +-
drivers/hid/wacom_wac.c | 2 --
drivers/hid/wacom_wac.h | 1 -
3 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index eb833455abd5..16862f97c46c 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2684,7 +2684,7 @@ static void wacom_remote_work(struct work_struct *work)
for (i = 0; i < WACOM_MAX_REMOTES; i++) {
serial = data.remote[i].serial;
- if (data.remote[i].connected) {
+ if (serial) {
if (kt - remote->remotes[i].active_time > WACOM_REMOTE_BATTERY_TIMEOUT
&& remote->remotes[i].active_time != 0)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 6c056f8844e7..c922c1c4c78f 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1209,10 +1209,8 @@ static void wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
for (i = 0; i < WACOM_MAX_REMOTES; i++) {
int j = i * 6;
int serial = (data[j+6] << 16) + (data[j+5] << 8) + data[j+4];
- bool connected = data[j+2];
remote_data.remote[i].serial = serial;
- remote_data.remote[i].connected = connected;
}
spin_lock_irqsave(&remote->remote_lock, flags);
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 2e7cc5e7a0cb..a10b19669947 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -331,7 +331,6 @@ struct hid_data {
struct wacom_remote_data {
struct {
u32 serial;
- bool connected;
} remote[WACOM_MAX_REMOTES];
};
--
2.34.1
^ permalink raw reply related
* [PATCH Wacom EKR Battery Fixes 0/3]
From: Aaron Armstrong Skomra @ 2023-07-25 22:19 UTC (permalink / raw)
To: linux-input, linux-kernel, benjamin.tissoires, jikos
Cc: pinglinux, skomra, Joshua, jason.gerecke, stable
Currently the ExpressKey Remote battery will persist indefinitely in
/sys/class/power_supply. Remove the battery when we stop getting
reports from the device.
Also remove some unneccessary code and make an EKR struct name more
descriptive.
Only the bugfix in the first patch will be sent to stable.
Aaron Armstrong Skomra (3):
HID: wacom: remove the battery when the EKR is off
HID: wacom: remove unnecessary 'connected' variable from EKR
HID: wacom: struct name cleanup
drivers/hid/wacom.h | 1 +
drivers/hid/wacom_sys.c | 44 +++++++++++++++++++++++++++++------------
drivers/hid/wacom_wac.c | 7 +++----
drivers/hid/wacom_wac.h | 4 ++--
4 files changed, 37 insertions(+), 19 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH Wacom EKR Battery Fixes 3/3] HID: wacom: struct name cleanup
From: Aaron Armstrong Skomra @ 2023-07-25 22:21 UTC (permalink / raw)
To: linux-input, linux-kernel, benjamin.tissoires, jikos
Cc: pinglinux, skomra, Joshua, jason.gerecke, Aaron Armstrong Skomra
From: Aaron Armstrong Skomra <aaron.skomra@wacom.com>
Help differentiate the two remote related "serial" struct variables by
renaming "wacom_remote_data" to "wacom_remote_work_data".
Signed-off-by: Aaron Skomra <skomra@gmail.com>
Signed-off-by: Aaron Armstrong Skomra <aaron.skomra@wacom.com>
Reviewed-by: Jason Gerecke <jason.gerecke@wacom.com>
---
drivers/hid/wacom_sys.c | 19 ++++++++++---------
drivers/hid/wacom_wac.c | 4 ++--
drivers/hid/wacom_wac.h | 2 +-
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 16862f97c46c..3f704b8072e8 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1997,7 +1997,7 @@ static int wacom_initialize_remotes(struct wacom *wacom)
spin_lock_init(&remote->remote_lock);
error = kfifo_alloc(&remote->remote_fifo,
- 5 * sizeof(struct wacom_remote_data),
+ 5 * sizeof(struct wacom_remote_work_data),
GFP_KERNEL);
if (error) {
hid_err(wacom->hdev, "failed allocating remote_fifo\n");
@@ -2660,17 +2660,18 @@ static void wacom_remote_work(struct work_struct *work)
struct wacom *wacom = container_of(work, struct wacom, remote_work);
struct wacom_remote *remote = wacom->remote;
ktime_t kt = ktime_get();
- struct wacom_remote_data data;
+ struct wacom_remote_work_data remote_work_data;
unsigned long flags;
unsigned int count;
- u32 serial;
+ u32 work_serial;
int i;
spin_lock_irqsave(&remote->remote_lock, flags);
- count = kfifo_out(&remote->remote_fifo, &data, sizeof(data));
+ count = kfifo_out(&remote->remote_fifo, &remote_work_data,
+ sizeof(remote_work_data));
- if (count != sizeof(data)) {
+ if (count != sizeof(remote_work_data)) {
hid_err(wacom->hdev,
"workitem triggered without status available\n");
spin_unlock_irqrestore(&remote->remote_lock, flags);
@@ -2683,14 +2684,14 @@ static void wacom_remote_work(struct work_struct *work)
spin_unlock_irqrestore(&remote->remote_lock, flags);
for (i = 0; i < WACOM_MAX_REMOTES; i++) {
- serial = data.remote[i].serial;
- if (serial) {
+ work_serial = remote_work_data.remote[i].serial;
+ if (work_serial) {
if (kt - remote->remotes[i].active_time > WACOM_REMOTE_BATTERY_TIMEOUT
&& remote->remotes[i].active_time != 0)
wacom_remote_destroy_battery(wacom, i);
- if (remote->remotes[i].serial == serial) {
+ if (remote->remotes[i].serial == work_serial) {
wacom_remote_attach_battery(wacom, i);
continue;
}
@@ -2698,7 +2699,7 @@ static void wacom_remote_work(struct work_struct *work)
if (remote->remotes[i].serial)
wacom_remote_destroy_one(wacom, i);
- wacom_remote_create_one(wacom, serial, i);
+ wacom_remote_create_one(wacom, work_serial, i);
} else if (remote->remotes[i].serial) {
wacom_remote_destroy_one(wacom, i);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index c922c1c4c78f..471db78dbbf0 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1197,14 +1197,14 @@ static void wacom_remote_status_irq(struct wacom_wac *wacom_wac, size_t len)
struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
unsigned char *data = wacom_wac->data;
struct wacom_remote *remote = wacom->remote;
- struct wacom_remote_data remote_data;
+ struct wacom_remote_work_data remote_data;
unsigned long flags;
int i, ret;
if (data[0] != WACOM_REPORT_DEVICE_LIST)
return;
- memset(&remote_data, 0, sizeof(struct wacom_remote_data));
+ memset(&remote_data, 0, sizeof(struct wacom_remote_work_data));
for (i = 0; i < WACOM_MAX_REMOTES; i++) {
int j = i * 6;
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index a10b19669947..57e185f18d53 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -328,7 +328,7 @@ struct hid_data {
ktime_t time_delayed;
};
-struct wacom_remote_data {
+struct wacom_remote_work_data {
struct {
u32 serial;
} remote[WACOM_MAX_REMOTES];
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
From: Doug Anderson @ 2023-07-25 20:41 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Frank Rowand, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan
In-Reply-To: <CAD=FV=UUdc5xi-HoOye-a1Oki3brcX3V1+=zuxQKLAcrd3iTSA@mail.gmail.com>
Hi,
On Mon, Jul 17, 2023 at 11:15 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Benjamin,
>
> On Mon, Jun 26, 2023 at 3:49 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Benjamin,
> >
> > On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > > +static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = {
> > > > + .panel_prepared = i2c_hid_core_panel_prepared,
> > > > + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> > > > +};
> > >
> > > Can we make that above block at least behind a Kconfig?
> > >
> > > i2c-hid is often used for touchpads, and the notion of drm panel has
> > > nothing to do with them. So I'd be more confident if we could disable
> > > that code if not required.
> >
> > Now that other concerns are addressed, I started trying to write up a
> > v3 and I found myself writing this as the description of the Kconfig
> > entry:
> >
> > --
> > config I2C_HID_SUPPORT_PANEL_FOLLOWER
> > bool "Support i2c-hid devices that must be power sequenced with a panel"
> >
> > Say Y here if you want support for i2c-hid devices that need to
> > coordinate power sequencing with a panel. This is typically important
> > when you have a panel and a touchscreen that share power rails or
> > reset GPIOs. If you say N here then the kernel will not try to honor
> > any shared power sequencing for your hardware. In the best case,
> > ignoring power sequencing when it's needed will draw extra power. In
> > the worst case this will prevent your hardware from functioning or
> > could even damage your hardware.
> >
> > If unsure, say Y.
> >
> > --
> >
> > I can certainly go that way, but I just wanted to truly make sure
> > that's what we want. Specifically:
> >
> > 1. If we put the panel follower code behind a Kconfig then we actually
> > have no idea if a touchscreen was intended to be a panel follower.
> > Specifically the panel follower API is the one that detects the
> > connection between the panel and the i2c-hid device, so without being
> > able to call the panel follower API we have no idea that an i2c-hid
> > device was supposed to be a panel follower.
> >
> > 2. It is conceivable that power sequencing a device incorrectly could
> > truly cause hardware damage.
> >
> > Together, those points mean that if you turn off the Kconfig entry and
> > then try to boot on a device that needed that Kconfig setting that you
> > might damage hardware. I can code it up that way if you want, but it
> > worries me...
> >
> >
> > Alternatives that I can think of:
> >
> > a) I could change the panel follower API so that panel followers are
> > in charge of detecting the panel that they follow. Today, that looks
> > like:
> >
> > panel_np = of_parse_phandle(dev->of_node, "panel", 0);
> > if (panel_np)
> > /* It's a panel follower */
> > of_node_put(panel_np);
> >
> > ...so we could put that code in each touchscreen driver and then fail
> > to probe i2c-hid if we detect that we're supposed to be a panel
> > follower but the Kconfig is turned off. The above doesn't seem
> > massively ideal since it duplicates code. Also, one reason why I put
> > that code in drm_panel_add_follower() is that I think this concept
> > will eventually be needed even for non-DT cases. I don't know how to
> > write the non-DT code right now, though...
> >
> >
> > b) I could open-code detect the panel follower case but leave the
> > actual linking to the panel follower API. AKA add to i2c-hid:
> >
> > if (of_property_read_bool(dev->of_node, "panel"))
> > /* It's a panel follower */
> >
> > ...that's a smaller bit of code, but feels like an abstraction
> > violation. It also would need to be updated if/when we added support
> > for non-DT panel followers.
> >
> >
> > c) I could add a "static inline" implementation of b) to "drm_panel.h".
> >
> > That sounds great and I started doing it. ...but then realized that it
> > means adding to drm_panel.h:
> >
> > #include <linux/device.h>
> > #include <linux/of.h>
> >
> > ...because otherwise of_property_read_bool() isn't defined and "struct
> > device" can't be dereferenced. That might be OK, but it looks as if
> > folks have been working hard to avoid things like this in header
> > files. Presumably it would get uglier if/when we added the non-DT
> > case, as well. That being said, I can give it a shot...
> >
> > --
> >
> > At this point, I'm hoping for some advice. How important is it for you
> > to have a Kconfig for "I2C_HID_SUPPORT_PANEL_FOLLOWER"?
> >
> > NOTE: even if I don't add the Kconfig, I could at least create a
> > function for registering the panel follower that would get most of the
> > panel follower logic out of the probe function. Would that be enough?
>
> I'd love to send a new version of this patch series, but I'm still
> stuck with the above issue. I'm hoping you might have a minute to
> provide your thoughts. If I don't hear anything, I'll try a v3 where I
> don't have the Kconfig for "I2C_HID_SUPPORT_PANEL_FOLLOWER" but just
> try to pull a little more of the code out of the probe function.
To provide breadcrumbs, I posted the v3 which pulls a bit more code
out of the probe function but is otherwise largely unchanged. The
cover letter for v3 can be found at:
https://lore.kernel.org/r/20230725203545.2260506-1-dianders@chromium.org/
^ permalink raw reply
* [PATCH v3 09/10] HID: i2c-hid: Do panel follower work on the system_wq
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson
In-Reply-To: <20230725203545.2260506-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>
---
(no changes since v2)
Changes in v2:
- ihid_core_panel_prepare_work() is now static.
- Improve documentation for smp_wmb().
drivers/hid/i2c-hid/i2c-hid-core.c | 50 +++++++++++++++++++++++++++---
1 file changed, 46 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index fa6d1f624342..6940e74d8acb 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 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
return ret;
}
-static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
+static 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,52 @@ static 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);
+
+ /*
+ * The work APIs provide a number of memory ordering guarantees
+ * including one that says that memory writes before schedule_work()
+ * are always visible to the work function, but they don't appear to
+ * guarantee that a write that happened in the work is visible after
+ * cancel_work_sync(). We'll add a write memory barrier here to match
+ * with i2c_hid_core_panel_unpreparing() to ensure that our write to
+ * prepare_work_finished is visible there.
+ */
+ smp_wmb();
+}
+
+static 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;
}
static 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);
}
@@ -1149,6 +1190,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.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 10/10] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson
In-Reply-To: <20230725203545.2260506-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>
---
(no changes since v1)
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 62ab6427dd65..e5d6a7898f8c 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi
@@ -69,6 +69,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 2efa8a4bcda6..0e2b4d06b490 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
@@ -123,6 +123,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.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 08/10] HID: i2c-hid: Support being a panel follower
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson
In-Reply-To: <20230725203545.2260506-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>
---
Changes in v3:
- Add "depends on DRM || !DRM" to Kconfig to avoid randconfig error.
- Split more of the panel follower code out of the core.
Changes in v2:
- i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
drivers/hid/i2c-hid/Kconfig | 2 +
drivers/hid/i2c-hid/i2c-hid-core.c | 82 +++++++++++++++++++++++++++++-
2 files changed, 82 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 3be17109301a..2bdb55203104 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -70,5 +70,7 @@ config I2C_HID_OF_GOODIX
config I2C_HID_CORE
tristate
+ # We need to call into panel code so if DRM=m, this can't be 'y'
+ depends on DRM || !DRM
endif
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index fa8a1ca43d7f..fa6d1f624342 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -38,6 +38,8 @@
#include <linux/mutex.h>
#include <asm/unaligned.h>
+#include <drm/drm_panel.h>
+
#include "../hid-ids.h"
#include "i2c-hid.h"
@@ -107,6 +109,8 @@ struct i2c_hid {
struct mutex reset_lock;
struct i2chid_ops *ops;
+ struct drm_panel_follower panel_follower;
+ bool is_panel_follower;
};
static const struct i2c_hid_quirks {
@@ -1058,6 +1062,59 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
return ret;
}
+static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
+{
+ struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
+ struct hid_device *hid = ihid->hid;
+
+ /*
+ * hid->version is set on the first power up. If it's still zero then
+ * this is the first power on so we should perform initial power up
+ * steps.
+ */
+ if (!hid->version)
+ return i2c_hid_core_initial_power_up(ihid);
+
+ return i2c_hid_core_resume(ihid);
+}
+
+static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower)
+{
+ struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
+
+ return i2c_hid_core_suspend(ihid);
+}
+
+static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = {
+ .panel_prepared = i2c_hid_core_panel_prepared,
+ .panel_unpreparing = i2c_hid_core_panel_unpreparing,
+};
+
+static int i2c_hid_core_register_panel_follower(struct i2c_hid *ihid)
+{
+ struct device *dev = &ihid->client->dev;
+ int ret;
+
+ ihid->is_panel_follower = true;
+ ihid->panel_follower.funcs = &i2c_hid_core_panel_follower_funcs;
+
+ /*
+ * If we're not in control of our own power up/power down then we can't
+ * do the logic to manage wakeups. Give a warning if a user thought
+ * that was possible then force the capability off.
+ */
+ if (device_can_wakeup(dev)) {
+ dev_warn(dev, "Can't wakeup if following panel\n");
+ device_set_wakeup_capable(dev, false);
+ }
+
+ ret = drm_panel_add_follower(dev, &ihid->panel_follower);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
u16 hid_descriptor_address, u32 quirks)
{
@@ -1119,7 +1176,15 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
hid->bus = BUS_I2C;
hid->initial_quirks = quirks;
- ret = i2c_hid_core_initial_power_up(ihid);
+ /*
+ * If we're a panel follower, we'll register and do our initial power
+ * up when the panel turns on; otherwise we do it right away.
+ */
+ if (drm_is_panel_follower(&client->dev))
+ ret = i2c_hid_core_register_panel_follower(ihid);
+ else
+ ret = i2c_hid_core_initial_power_up(ihid);
+
if (ret)
goto err_mem_free;
@@ -1143,7 +1208,14 @@ void i2c_hid_core_remove(struct i2c_client *client)
struct i2c_hid *ihid = i2c_get_clientdata(client);
struct hid_device *hid;
- i2c_hid_core_power_down(ihid);
+ /*
+ * If we're a follower, the act of unfollowing will cause us to be
+ * powered down. Otherwise we need to manually do it.
+ */
+ if (ihid->is_panel_follower)
+ drm_panel_remove_follower(&ihid->panel_follower);
+ else
+ i2c_hid_core_power_down(ihid);
hid = ihid->hid;
hid_destroy_device(hid);
@@ -1171,6 +1243,9 @@ static int i2c_hid_core_pm_suspend(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct i2c_hid *ihid = i2c_get_clientdata(client);
+ if (ihid->is_panel_follower)
+ return 0;
+
return i2c_hid_core_suspend(ihid);
}
@@ -1179,6 +1254,9 @@ static int i2c_hid_core_pm_resume(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct i2c_hid *ihid = i2c_get_clientdata(client);
+ if (ihid->is_panel_follower)
+ return 0;
+
return i2c_hid_core_resume(ihid);
}
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 07/10] HID: i2c-hid: Make suspend and resume into helper functions
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson
In-Reply-To: <20230725203545.2260506-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>
---
(no changes since v1)
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 d29e6421ecba..fa8a1ca43d7f 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.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 06/10] HID: i2c-hid: Rearrange probe() to power things up later
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson
In-Reply-To: <20230725203545.2260506-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>
---
(no changes since v2)
Changes in v2:
- i2c_hid_core_initial_power_up() is now static.
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..d29e6421ecba 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.
+ */
+static 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.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 05/10] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS()
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson
In-Reply-To: <20230725203545.2260506-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>
---
(no changes since v1)
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.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 04/10] of: property: fw_devlink: Add a devlink for panel followers
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson,
Rob Herring
In-Reply-To: <20230725203545.2260506-1-dianders@chromium.org>
Inform fw_devlink of the fact that a panel follower (like a
touchscreen) is effectively a consumer of the panel from the purposes
of fw_devlink.
NOTE: this patch isn't required for correctness but instead optimizes
probe order / helps avoid deferrals.
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Since this is so small, I'd presume it's OK for it to go through a DRM
tree with the proper Ack. That being said, this patch is just an
optimization and thus it could land completely separately from the
rest and they could all meet up in mainline.
(no changes since v2)
Changes in v2:
- ("Add a devlink for panel followers") new for v2.
drivers/of/property.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index ddc75cd50825..cf8dacf3e3b8 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1266,6 +1266,7 @@ DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
DEFINE_SIMPLE_PROP(leds, "leds", NULL)
DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
+DEFINE_SIMPLE_PROP(panel, "panel", NULL)
DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
@@ -1354,6 +1355,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
{ .parse_prop = parse_resets, },
{ .parse_prop = parse_leds, },
{ .parse_prop = parse_backlight, },
+ { .parse_prop = parse_panel, },
{ .parse_prop = parse_gpio_compat, },
{ .parse_prop = parse_interrupts, },
{ .parse_prop = parse_regulators, },
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 03/10] drm/panel: Add a way for other devices to follow panel state
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson
In-Reply-To: <20230725203545.2260506-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 don't power cycle it whenever you stop and restart the video
stream (like during a modeset). The touchscreen keeping the power
rails on causes 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.
A bit of explaining about why this is a roll-your-own API instead of
using something more standard:
1. In standard APIs in Linux, parent devices are automatically powered
on when a child needs power. Applying that here, it would mean that
we'd force the panel on any time someone was listening to the
touchscreen. That, unfortunately, would have broken homestar's need
(if we hadn't changed the hardware, as per above) where the panel
absolutely needs to be able to power cycle itself. While one could
argue that homestar is broken hardware and we shouldn't have the
API do backflips for it, _officially_ the eDP timing guidelines
agree with homestar's needs and the panel power sequencing diagrams
show power going off. It's nice to be able to support this.
2. We could, conceibably, try to add a new flag to device_link causing
the parent to be in charge of power. Then we could at least use
normal pm_runtime APIs. This sounds great, except that we run into
problems with initial probe. As talked about in the later patch
("HID: i2c-hid: Support being a panel follower") the initial power
on of a panel follower might need to do things (like add
sub-devices) that aren't allowed in a runtime_resume function.
The above complexities explain why this API isn't using common
functions. That being said, this patch is very small and
self-contained, so if someone was later able to adapt it to using more
common APIs while solving the above issues then that could happen in
the future.
[1] https://lore.kernel.org/r/20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v3:
- Add is_panel_follower() as a convenience for clients.
Changes in v2:
- Add even more text to the commit message.
- A few comment cleanups.
drivers/gpu/drm/drm_panel.c | 173 +++++++++++++++++++++++++++++++++++-
include/drm/drm_panel.h | 80 +++++++++++++++++
2 files changed, 249 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 4e1c4e42575b..e814020bbcd3 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,141 @@ int of_drm_get_panel_orientation(const struct device_node *np,
EXPORT_SYMBOL(of_drm_get_panel_orientation);
#endif
+/**
+ * drm_is_panel_follower() - Check if the device is a panel follower
+ * @dev: The 'struct device' to check
+ *
+ * This checks to see if a device needs to be power sequenced together with
+ * a panel using the panel follower API.
+ * 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: true if we should be power sequenced with a panel; false otherwise.
+ */
+bool drm_is_panel_follower(struct device *dev)
+{
+ /*
+ * The "panel" property is actually a phandle, but for simplicity we
+ * don't bother trying to parse it here. We just need to know if the
+ * property is there.
+ */
+ return of_property_read_bool(dev->of_node, "panel");
+}
+EXPORT_SYMBOL(drm_is_panel_follower);
+
+/**
+ * drm_panel_add_follower() - Register something to follow panel 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. Note that -ENODEV means that we detected that
+ * follower_dev is not actually following a panel. The caller may
+ * choose to ignore this return value if following a panel is optional.
+ */
+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
+ * unprepare function if we're removed from a panel that's currently prepared.
+ *
+ * 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..5ac67eeb0860 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,31 @@ static inline int of_drm_get_panel_orientation(const struct device_node *np,
}
#endif
+#if defined(CONFIG_DRM_PANEL)
+bool drm_is_panel_follower(struct device *dev);
+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 bool drm_is_panel_follower(struct device *dev)
+{
+ return false;
+}
+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.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 02/10] drm/panel: Check for already prepared/enabled in drm_panel
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson
In-Reply-To: <20230725203545.2260506-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
Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This has Neil's Ack and I could commit it to drm-misc-next, but for
now I'm holding off to see where this series ends up. If the series
ends up looking good we'll have to coordinate landing the various bits
between the drm and the hid trees and the second drm patch in my
series depends on this one.
If my series implodes I'll land this one on its own. In any case, once
this lands somewhere I'll take an AI to cleanup the panels.
(no changes since v1)
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.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, Douglas Anderson,
Krzysztof Kozlowski
In-Reply-To: <20230725203545.2260506-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.
The proerty is added to the generic touchscreen bindings and then
enabled in the bindings for the i2c-hid backed devices. This can and
should be added for other touchscreens in the future, but for now
let's start small.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
(no changes since v2)
Changes in v2:
- Move the description to the generic touchscreen.yaml.
- Update the desc to make it clearer it's only for integrated devices.
Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 5 +++++
.../devicetree/bindings/input/goodix,gt7375p.yaml | 5 +++++
Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 2 ++
.../devicetree/bindings/input/touchscreen/touchscreen.yaml | 7 +++++++
4 files changed, 19 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index 05e6f2df604c..3e2d216c6432 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -13,6 +13,9 @@ description:
Supports the Elan eKTH6915 touchscreen controller.
This touchscreen controller uses the i2c-hid protocol with a reset GPIO.
+allOf:
+ - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+
properties:
compatible:
items:
@@ -24,6 +27,8 @@ properties:
interrupts:
maxItems: 1
+ panel: true
+
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 1edad1da1196..358cb8275bf1 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -14,6 +14,9 @@ description:
This touchscreen uses the i2c-hid protocol but has some non-standard
power sequencing required.
+allOf:
+ - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+
properties:
compatible:
oneOf:
@@ -30,6 +33,8 @@ properties:
interrupts:
maxItems: 1
+ panel: true
+
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..138caad96a29 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
@@ -44,6 +44,8 @@ properties:
description: HID descriptor address
$ref: /schemas/types.yaml#/definitions/uint32
+ panel: true
+
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.
diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
index 895592da9626..431c13335c40 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
@@ -10,6 +10,13 @@ maintainers:
- Dmitry Torokhov <dmitry.torokhov@gmail.com>
properties:
+ panel:
+ description: If this touchscreen is integrally connected to a panel, this
+ is a reference to that panel. The presence of this reference indicates
+ that the touchscreen should be power sequenced together with the panel
+ and that they may share power and/or reset signals.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
touchscreen-min-x:
description: minimum x coordinate reported
$ref: /schemas/types.yaml#/definitions/uint32
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v3 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Douglas Anderson @ 2023-07-25 20:34 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: cros-qcom-dts-watchers, Chris Morgan, linux-input, hsinyi,
linux-arm-msm, dri-devel, linux-kernel, Dmitry Torokhov,
devicetree, Daniel Vetter, yangcong5, 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. ;-)
Version 3 of this patch was a long time coming after v2. Maxime and I
had a very long discussion trying to figure out if there was a beter
way and in the end we didn't find one so he was OK with the series in
general [1]. After that got resolved, I tried to resolve Benjamin's
feedback but got stuck [2]. Presumably Benjamin is busy at the moment,
so I've done my best to try to resolve things. The end result is a v3
that's not that different from v2 but that has a tiny bit more code
split out.
Version 2 of this patch series didn't change too much. At a high level:
* I added all the forgotten "static" to functions.
* I've hopefully made the bindings better.
* I've integrated into fw_devlink.
* I cleaned up a few descriptions / comments.
This still needs someone to say that the idea looks OK or to suggest
an alternative that solves the problems. ;-)
[1] https://lore.kernel.org/r/gkwymmfkdy2p2evz22wmbwgw42ii4wnvmvu64m3bghmj2jhv7x@4mbstjxnagxd
[2] https://lore.kernel.org/r/CAD=FV=VbdeomBGbWhppY+5TOSwt64GWBHga68OXFwsnO4gg4UA@mail.gmail.com
Changes in v3:
- Add is_panel_follower() as a convenience for clients.
- Add "depends on DRM || !DRM" to Kconfig to avoid randconfig error.
- Split more of the panel follower code out of the core.
Changes in v2:
- Move the description to the generic touchscreen.yaml.
- Update the desc to make it clearer it's only for integrated devices.
- Add even more text to the commit message.
- A few comment cleanups.
- ("Add a devlink for panel followers") new for v2.
- i2c_hid_core_initial_power_up() is now static.
- i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
- ihid_core_panel_prepare_work() is now static.
- Improve documentation for smp_wmb().
Douglas Anderson (10):
dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed
touchscreens
drm/panel: Check for already prepared/enabled in drm_panel
drm/panel: Add a way for other devices to follow panel state
of: property: fw_devlink: Add a devlink for panel followers
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 | 5 +
.../bindings/input/goodix,gt7375p.yaml | 5 +
.../bindings/input/hid-over-i2c.yaml | 2 +
.../input/touchscreen/touchscreen.yaml | 7 +
.../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 | 218 ++++++++++-
drivers/hid/i2c-hid/Kconfig | 2 +
drivers/hid/i2c-hid/i2c-hid-core.c | 338 +++++++++++++-----
drivers/of/property.c | 2 +
include/drm/drm_panel.h | 94 +++++
15 files changed, 583 insertions(+), 96 deletions(-)
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply
* Re: [PATCH 5/5] Input: tca6416-keypad - switch to using input core's polling features
From: Dmitry Torokhov @ 2023-07-25 19:50 UTC (permalink / raw)
To: Silvan Jegen; +Cc: linux-input, Yangtao Li, linux-kernel
In-Reply-To: <2KN6Z0RZYGDKU.2GGSSC8MYU6GX@homearch.localdomain>
On Tue, Jul 25, 2023 at 09:43:12PM +0200, Silvan Jegen wrote:
> Hi
>
> Just one question below.
>
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > Instead of rolling custom polling implementation use input core
> > facilities.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/keyboard/tca6416-keypad.c | 46 ++++++++++---------------
> > 1 file changed, 19 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/tca6416-keypad.c b/drivers/input/keyboard/tca6416-keypad.c
> > index ff665319791e..ebc8b9561266 100644
> > --- a/drivers/input/keyboard/tca6416-keypad.c
> > +++ b/drivers/input/keyboard/tca6416-keypad.c
> > @@ -24,6 +24,8 @@
> > #define TCA6416_INVERT 2
> > #define TCA6416_DIRECTION 3
> >
> > +#define TCA6416_POLL_INTERVAL 100 /* msec */
> > +
> > static const struct i2c_device_id tca6416_id[] = {
> > { "tca6416-keys", 16, },
> > { "tca6408-keys", 8, },
> > @@ -43,7 +45,6 @@ struct tca6416_keypad_chip {
> >
> > struct i2c_client *client;
> > struct input_dev *input;
> > - struct delayed_work dwork;
> > int io_size;
> > int irqnum;
> > u16 pinmask;
> > @@ -85,9 +86,9 @@ static int tca6416_read_reg(struct tca6416_keypad_chip *chip, int reg, u16 *val)
> > return 0;
> > }
> >
> > -static void tca6416_keys_scan(struct tca6416_keypad_chip *chip)
> > +static void tca6416_keys_scan(struct input_dev *input)
> > {
> > - struct input_dev *input = chip->input;
> > + struct tca6416_keypad_chip *chip = input_get_drvdata(input);
> > u16 reg_val, val;
> > int error, i, pin_index;
> >
> > @@ -122,33 +123,20 @@ static void tca6416_keys_scan(struct tca6416_keypad_chip *chip)
> > */
> > static irqreturn_t tca6416_keys_isr(int irq, void *dev_id)
> > {
> > - struct tca6416_keypad_chip *chip = dev_id;
> > -
> > - tca6416_keys_scan(chip);
> > + tca6416_keys_scan(dev_id);
> >
> > return IRQ_HANDLED;
> > }
> >
> > -static void tca6416_keys_work_func(struct work_struct *work)
> > -{
> > - struct tca6416_keypad_chip *chip =
> > - container_of(work, struct tca6416_keypad_chip, dwork.work);
> > -
> > - tca6416_keys_scan(chip);
> > - schedule_delayed_work(&chip->dwork, msecs_to_jiffies(100));
> > -}
> > -
> > static int tca6416_keys_open(struct input_dev *dev)
> > {
> > struct tca6416_keypad_chip *chip = input_get_drvdata(dev);
> >
> > - /* Get initial device state in case it has switches */
> > - tca6416_keys_scan(chip);
> > -
> > - if (chip->use_polling)
> > - schedule_delayed_work(&chip->dwork, msecs_to_jiffies(100));
> > - else
> > + if (!chip->use_polling) {
> > + /* Get initial device state in case it has switches */
> > + tca6416_keys_scan(dev);
> > enable_irq(chip->client->irq);
> > + }
> >
> > return 0;
> > }
> > @@ -157,9 +145,7 @@ static void tca6416_keys_close(struct input_dev *dev)
> > {
> > struct tca6416_keypad_chip *chip = input_get_drvdata(dev);
> >
> > - if (chip->use_polling)
> > - cancel_delayed_work_sync(&chip->dwork);
> > - else
> > + if (!chip->use_polling)
> > disable_irq(chip->client->irq);
> > }
> >
> > @@ -232,8 +218,6 @@ static int tca6416_keypad_probe(struct i2c_client *client)
> > chip->pinmask = pdata->pinmask;
> > chip->use_polling = pdata->use_polling;
> >
> > - INIT_DELAYED_WORK(&chip->dwork, tca6416_keys_work_func);
> > -
> > input->phys = "tca6416-keys/input0";
> > input->name = client->name;
> >
> > @@ -268,12 +252,20 @@ static int tca6416_keypad_probe(struct i2c_client *client)
> > return error;
> >
> > if (!chip->use_polling) {
>
> Sorry for my ignorant question but it seems counterituitive that we set
> up polling when chip->use_polling is false. Is this intended?
Nope, this is my brain fart. Thanks for noticing! I'll fix up the
condition before committing...
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 5/5] Input: tca6416-keypad - switch to using input core's polling features
From: Silvan Jegen @ 2023-07-25 19:43 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Yangtao Li, linux-kernel
In-Reply-To: <20230724053024.352054-5-dmitry.torokhov@gmail.com>
Hi
Just one question below.
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Instead of rolling custom polling implementation use input core
> facilities.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/keyboard/tca6416-keypad.c | 46 ++++++++++---------------
> 1 file changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/input/keyboard/tca6416-keypad.c b/drivers/input/keyboard/tca6416-keypad.c
> index ff665319791e..ebc8b9561266 100644
> --- a/drivers/input/keyboard/tca6416-keypad.c
> +++ b/drivers/input/keyboard/tca6416-keypad.c
> @@ -24,6 +24,8 @@
> #define TCA6416_INVERT 2
> #define TCA6416_DIRECTION 3
>
> +#define TCA6416_POLL_INTERVAL 100 /* msec */
> +
> static const struct i2c_device_id tca6416_id[] = {
> { "tca6416-keys", 16, },
> { "tca6408-keys", 8, },
> @@ -43,7 +45,6 @@ struct tca6416_keypad_chip {
>
> struct i2c_client *client;
> struct input_dev *input;
> - struct delayed_work dwork;
> int io_size;
> int irqnum;
> u16 pinmask;
> @@ -85,9 +86,9 @@ static int tca6416_read_reg(struct tca6416_keypad_chip *chip, int reg, u16 *val)
> return 0;
> }
>
> -static void tca6416_keys_scan(struct tca6416_keypad_chip *chip)
> +static void tca6416_keys_scan(struct input_dev *input)
> {
> - struct input_dev *input = chip->input;
> + struct tca6416_keypad_chip *chip = input_get_drvdata(input);
> u16 reg_val, val;
> int error, i, pin_index;
>
> @@ -122,33 +123,20 @@ static void tca6416_keys_scan(struct tca6416_keypad_chip *chip)
> */
> static irqreturn_t tca6416_keys_isr(int irq, void *dev_id)
> {
> - struct tca6416_keypad_chip *chip = dev_id;
> -
> - tca6416_keys_scan(chip);
> + tca6416_keys_scan(dev_id);
>
> return IRQ_HANDLED;
> }
>
> -static void tca6416_keys_work_func(struct work_struct *work)
> -{
> - struct tca6416_keypad_chip *chip =
> - container_of(work, struct tca6416_keypad_chip, dwork.work);
> -
> - tca6416_keys_scan(chip);
> - schedule_delayed_work(&chip->dwork, msecs_to_jiffies(100));
> -}
> -
> static int tca6416_keys_open(struct input_dev *dev)
> {
> struct tca6416_keypad_chip *chip = input_get_drvdata(dev);
>
> - /* Get initial device state in case it has switches */
> - tca6416_keys_scan(chip);
> -
> - if (chip->use_polling)
> - schedule_delayed_work(&chip->dwork, msecs_to_jiffies(100));
> - else
> + if (!chip->use_polling) {
> + /* Get initial device state in case it has switches */
> + tca6416_keys_scan(dev);
> enable_irq(chip->client->irq);
> + }
>
> return 0;
> }
> @@ -157,9 +145,7 @@ static void tca6416_keys_close(struct input_dev *dev)
> {
> struct tca6416_keypad_chip *chip = input_get_drvdata(dev);
>
> - if (chip->use_polling)
> - cancel_delayed_work_sync(&chip->dwork);
> - else
> + if (!chip->use_polling)
> disable_irq(chip->client->irq);
> }
>
> @@ -232,8 +218,6 @@ static int tca6416_keypad_probe(struct i2c_client *client)
> chip->pinmask = pdata->pinmask;
> chip->use_polling = pdata->use_polling;
>
> - INIT_DELAYED_WORK(&chip->dwork, tca6416_keys_work_func);
> -
> input->phys = "tca6416-keys/input0";
> input->name = client->name;
>
> @@ -268,12 +252,20 @@ static int tca6416_keypad_probe(struct i2c_client *client)
> return error;
>
> if (!chip->use_polling) {
Sorry for my ignorant question but it seems counterituitive that we set
up polling when chip->use_polling is false. Is this intended?
Cheers,
Silvan
> + error = input_setup_polling(input, tca6416_keys_scan);
> + if (error) {
> + dev_err(&client->dev, "Failed to setup polling\n");
> + return error;
> + }
> +
> + input_set_poll_interval(input, TCA6416_POLL_INTERVAL);
> + } else {
> error = devm_request_threaded_irq(&client->dev, client->irq,
> NULL, tca6416_keys_isr,
> IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT |
> IRQF_NO_AUTOEN,
> - "tca6416-keypad", chip);
> + "tca6416-keypad", input);
> if (error) {
> dev_dbg(&client->dev,
> "Unable to claim irq %d; error %d\n",
^ permalink raw reply
* Re: [PATCH] Input: mcs-touchkey - Fix uninitialized use of error in mcs_touchkey_probe()
From: Dmitry Torokhov @ 2023-07-25 16:54 UTC (permalink / raw)
To: Nathan Chancellor
Cc: ndesaulniers, trix, frank.li, linux-input, llvm, patches
In-Reply-To: <20230725-mcs_touchkey-fix-wuninitialized-v1-1-615db39af51c@kernel.org>
On Tue, Jul 25, 2023 at 08:37:56AM -0700, Nathan Chancellor wrote:
> Clang warns (or errors with CONFIG_WERROR=y):
>
> drivers/input/keyboard/mcs_touchkey.c:149:49: error: variable 'error' is uninitialized when used here [-Werror,-Wuninitialized]
> 149 | dev_err(&client->dev, "i2c read error[%d]\n", error);
> | ^~~~~
> include/linux/dev_printk.h:144:65: note: expanded from macro 'dev_err'
> 144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
> | ^~~~~~~~~~~
> include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
> 110 | _p_func(dev, fmt, ##__VA_ARGS__); \
> | ^~~~~~~~~~~
> drivers/input/keyboard/mcs_touchkey.c:110:11: note: initialize the variable 'error' to silence this warning
> 110 | int error;
> | ^
> | = 0
> 1 error generated.
>
> A refactoring updated the error handling in this block but did not
> update the dev_err() call to use fw_ver instead of error. Do so now to
> fix the warning and avoid printing uninitialized memory.
>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1893
> Fixes: e175eae16c1b ("Input: mcs-touchkey - convert to use devm_* api")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH] Input: mcs-touchkey - Fix uninitialized use of error in mcs_touchkey_probe()
From: Nathan Chancellor @ 2023-07-25 15:37 UTC (permalink / raw)
To: dmitry.torokhov
Cc: ndesaulniers, trix, frank.li, linux-input, llvm, patches,
Nathan Chancellor
Clang warns (or errors with CONFIG_WERROR=y):
drivers/input/keyboard/mcs_touchkey.c:149:49: error: variable 'error' is uninitialized when used here [-Werror,-Wuninitialized]
149 | dev_err(&client->dev, "i2c read error[%d]\n", error);
| ^~~~~
include/linux/dev_printk.h:144:65: note: expanded from macro 'dev_err'
144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~
include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
110 | _p_func(dev, fmt, ##__VA_ARGS__); \
| ^~~~~~~~~~~
drivers/input/keyboard/mcs_touchkey.c:110:11: note: initialize the variable 'error' to silence this warning
110 | int error;
| ^
| = 0
1 error generated.
A refactoring updated the error handling in this block but did not
update the dev_err() call to use fw_ver instead of error. Do so now to
fix the warning and avoid printing uninitialized memory.
Closes: https://github.com/ClangBuiltLinux/linux/issues/1893
Fixes: e175eae16c1b ("Input: mcs-touchkey - convert to use devm_* api")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
drivers/input/keyboard/mcs_touchkey.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/mcs_touchkey.c b/drivers/input/keyboard/mcs_touchkey.c
index f25e2b20e271..2410f676c7f9 100644
--- a/drivers/input/keyboard/mcs_touchkey.c
+++ b/drivers/input/keyboard/mcs_touchkey.c
@@ -146,7 +146,7 @@ static int mcs_touchkey_probe(struct i2c_client *client)
fw_ver = i2c_smbus_read_byte_data(client, fw_reg);
if (fw_ver < 0) {
- dev_err(&client->dev, "i2c read error[%d]\n", error);
+ dev_err(&client->dev, "i2c read error[%d]\n", fw_ver);
return fw_ver;
}
dev_info(&client->dev, "Firmware version: %d\n", fw_ver);
---
base-commit: 447c09544275663e1082f796b26c7959915c922a
change-id: 20230725-mcs_touchkey-fix-wuninitialized-4ffe9a2a9aa9
Best regards,
--
Nathan Chancellor <nathan@kernel.org>
^ 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