* [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
@ 2023-06-07 21:49 Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens Douglas Anderson
` (10 more replies)
0 siblings, 11 replies; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, 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 2 of this patch series doesn'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. ;-)
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 | 196 +++++++++-
drivers/hid/i2c-hid/i2c-hid-core.c | 338 +++++++++++++-----
drivers/of/property.c | 2 +
include/drm/drm_panel.h | 89 +++++
14 files changed, 555 insertions(+), 95 deletions(-)
--
2.41.0.162.gfafddb0af9-goog
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-09 15:53 ` Krzysztof Kozlowski
2023-06-07 21:49 ` [PATCH v2 02/10] drm/panel: Check for already prepared/enabled in drm_panel Douglas Anderson
` (9 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
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 ce18d7dadae2..72212382d702 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.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 02/10] drm/panel: Check for already prepared/enabled in drm_panel
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 03/10] drm/panel: Add a way for other devices to follow panel state Douglas Anderson
` (8 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 03/10] drm/panel: Add a way for other devices to follow panel state
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 02/10] drm/panel: Check for already prepared/enabled in drm_panel Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers Douglas Anderson
` (7 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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] 20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v2:
- Add even more text to the commit message.
- A few comment cleanups.
drivers/gpu/drm/drm_panel.c | 151 +++++++++++++++++++++++++++++++++++-
include/drm/drm_panel.h | 75 ++++++++++++++++++
2 files changed, 222 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 4e1c4e42575b..d103eda89f8e 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,119 @@ int of_drm_get_panel_orientation(const struct device_node *np,
EXPORT_SYMBOL(of_drm_get_panel_orientation);
#endif
+/**
+ * drm_panel_add_follower() - Register something to follow panel 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..e0a4d2f6f7fb 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -27,12 +27,14 @@
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/list.h>
+#include <linux/mutex.h>
struct backlight_device;
struct dentry;
struct device_node;
struct drm_connector;
struct drm_device;
+struct drm_panel_follower;
struct drm_panel;
struct display_timing;
@@ -144,6 +146,45 @@ struct drm_panel_funcs {
void (*debugfs_init)(struct drm_panel *panel, struct dentry *root);
};
+struct drm_panel_follower_funcs {
+ /**
+ * @panel_prepared:
+ *
+ * Called after the panel has been powered on.
+ */
+ int (*panel_prepared)(struct drm_panel_follower *follower);
+
+ /**
+ * @panel_unpreparing:
+ *
+ * Called before the panel is powered off.
+ */
+ int (*panel_unpreparing)(struct drm_panel_follower *follower);
+};
+
+struct drm_panel_follower {
+ /**
+ * @funcs:
+ *
+ * Dependent device callbacks; should be initted by the caller.
+ */
+ const struct drm_panel_follower_funcs *funcs;
+
+ /**
+ * @list
+ *
+ * Used for linking into panel's list; set by drm_panel_add_follower().
+ */
+ struct list_head list;
+
+ /**
+ * @panel
+ *
+ * The panel we're dependent on; set by drm_panel_add_follower().
+ */
+ struct drm_panel *panel;
+};
+
/**
* struct drm_panel - DRM panel object
*/
@@ -189,6 +230,20 @@ struct drm_panel {
*/
struct list_head list;
+ /**
+ * @followers:
+ *
+ * A list of struct drm_panel_follower dependent on this panel.
+ */
+ struct list_head followers;
+
+ /**
+ * @followers_lock:
+ *
+ * Lock for followers list.
+ */
+ struct mutex follower_lock;
+
/**
* @prepare_prev_first:
*
@@ -246,6 +301,26 @@ static inline int of_drm_get_panel_orientation(const struct device_node *np,
}
#endif
+#if defined(CONFIG_DRM_PANEL)
+int drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower);
+void drm_panel_remove_follower(struct drm_panel_follower *follower);
+int devm_drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower);
+#else
+static inline int drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ return -ENODEV;
+}
+static inline void drm_panel_remove_follower(struct drm_panel_follower *follower) { }
+static inline int devm_drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ return -ENODEV;
+}
+#endif
+
#if IS_ENABLED(CONFIG_DRM_PANEL) && (IS_BUILTIN(CONFIG_BACKLIGHT_CLASS_DEVICE) || \
(IS_MODULE(CONFIG_DRM) && IS_MODULE(CONFIG_BACKLIGHT_CLASS_DEVICE)))
int drm_panel_of_backlight(struct drm_panel *panel);
--
2.41.0.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
` (2 preceding siblings ...)
2023-06-07 21:49 ` [PATCH v2 03/10] drm/panel: Add a way for other devices to follow panel state Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-09 16:10 ` Rob Herring
2023-06-07 21:49 ` [PATCH v2 05/10] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() Douglas Anderson
` (6 subsequent siblings)
10 siblings, 1 reply; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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.
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.
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.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 05/10] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS()
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
` (3 preceding siblings ...)
2023-06-07 21:49 ` [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 06/10] HID: i2c-hid: Rearrange probe() to power things up later Douglas Anderson
` (5 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 06/10] HID: i2c-hid: Rearrange probe() to power things up later
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
` (4 preceding siblings ...)
2023-06-07 21:49 ` [PATCH v2 05/10] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 07/10] HID: i2c-hid: Make suspend and resume into helper functions Douglas Anderson
` (4 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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>
---
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.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 07/10] HID: i2c-hid: Make suspend and resume into helper functions
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
` (5 preceding siblings ...)
2023-06-07 21:49 ` [PATCH v2 06/10] HID: i2c-hid: Rearrange probe() to power things up later Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower Douglas Anderson
` (3 subsequent siblings)
10 siblings, 0 replies; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
` (6 preceding siblings ...)
2023-06-07 21:49 ` [PATCH v2 07/10] HID: i2c-hid: Make suspend and resume into helper functions Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-08 5:18 ` kernel test robot
` (2 more replies)
2023-06-07 21:49 ` [PATCH v2 09/10] HID: i2c-hid: Do panel follower work on the system_wq Douglas Anderson
` (2 subsequent siblings)
10 siblings, 3 replies; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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 v2:
- i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
drivers/hid/i2c-hid/i2c-hid-core.c | 82 +++++++++++++++++++++++++++++-
1 file changed, 81 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index fa8a1ca43d7f..368db3ae612f 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -38,6 +38,8 @@
#include <linux/mutex.h>
#include <asm/unaligned.h>
+#include <drm/drm_panel.h>
+
#include "../hid-ids.h"
#include "i2c-hid.h"
@@ -107,6 +109,8 @@ struct i2c_hid {
struct mutex reset_lock;
struct i2chid_ops *ops;
+ struct drm_panel_follower panel_follower;
+ bool is_panel_follower;
};
static const struct i2c_hid_quirks {
@@ -1058,6 +1062,34 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
return ret;
}
+static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
+{
+ struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
+ struct hid_device *hid = ihid->hid;
+
+ /*
+ * hid->version is set on the first power up. If it's still zero then
+ * this is the first power on so we should perform initial power up
+ * steps.
+ */
+ if (!hid->version)
+ return i2c_hid_core_initial_power_up(ihid);
+
+ return i2c_hid_core_resume(ihid);
+}
+
+static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower)
+{
+ struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
+
+ return i2c_hid_core_suspend(ihid);
+}
+
+static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = {
+ .panel_prepared = i2c_hid_core_panel_prepared,
+ .panel_unpreparing = i2c_hid_core_panel_unpreparing,
+};
+
int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
u16 hid_descriptor_address, u32 quirks)
{
@@ -1119,6 +1151,41 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
hid->bus = BUS_I2C;
hid->initial_quirks = quirks;
+ /*
+ * See if we're following a panel. If drm_panel_add_follower()
+ * returns no error then we are.
+ */
+ ihid->panel_follower.funcs = &i2c_hid_core_panel_follower_funcs;
+ ret = drm_panel_add_follower(&client->dev, &ihid->panel_follower);
+ if (!ret) {
+ /* We're a follower. That means we'll power things up later. */
+ ihid->is_panel_follower = true;
+
+ /*
+ * If we're not in control of our own power up/power down then
+ * we can't do the logic to manage wakeups. Give a warning if
+ * a user thought that was possible then force the capability
+ * off.
+ */
+ if (device_can_wakeup(&client->dev)) {
+ dev_warn(&client->dev, "Can't wakeup if following panel\n");
+ device_set_wakeup_capable(&client->dev, false);
+ }
+
+ return 0;
+ }
+
+ /*
+ * -ENODEV means that we're not following a panel, so any other error
+ * is a real problem (like -EPROBE_DEFER, -ENOMEM, ...).
+ */
+ if (ret != -ENODEV)
+ goto err_mem_free;
+
+ /*
+ * We're not following a panel. That's fine and means that we
+ * can power up right away.
+ */
ret = i2c_hid_core_initial_power_up(ihid);
if (ret)
goto err_mem_free;
@@ -1143,7 +1210,14 @@ void i2c_hid_core_remove(struct i2c_client *client)
struct i2c_hid *ihid = i2c_get_clientdata(client);
struct hid_device *hid;
- i2c_hid_core_power_down(ihid);
+ /*
+ * If we're a follower, the act of unfollowing will cause us to be
+ * powered down. Otherwise we need to manually do it.
+ */
+ if (ihid->is_panel_follower)
+ drm_panel_remove_follower(&ihid->panel_follower);
+ else
+ i2c_hid_core_power_down(ihid);
hid = ihid->hid;
hid_destroy_device(hid);
@@ -1171,6 +1245,9 @@ static int i2c_hid_core_pm_suspend(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct i2c_hid *ihid = i2c_get_clientdata(client);
+ if (ihid->is_panel_follower)
+ return 0;
+
return i2c_hid_core_suspend(ihid);
}
@@ -1179,6 +1256,9 @@ static int i2c_hid_core_pm_resume(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct i2c_hid *ihid = i2c_get_clientdata(client);
+ if (ihid->is_panel_follower)
+ return 0;
+
return i2c_hid_core_resume(ihid);
}
--
2.41.0.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 09/10] HID: i2c-hid: Do panel follower work on the system_wq
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
` (7 preceding siblings ...)
2023-06-07 21:49 ` [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 10/10] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels Douglas Anderson
2023-06-08 7:17 ` [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Maxime Ripard
10 siblings, 0 replies; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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>
---
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 368db3ae612f..de1a0624be08 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);
}
@@ -1124,6 +1165,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.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2 10/10] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
` (8 preceding siblings ...)
2023-06-07 21:49 ` [PATCH v2 09/10] HID: i2c-hid: Do panel follower work on the system_wq Douglas Anderson
@ 2023-06-07 21:49 ` Douglas Anderson
2023-06-08 7:17 ` [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Maxime Ripard
10 siblings, 0 replies; 32+ messages in thread
From: Douglas Anderson @ 2023-06-07 21:49 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan, Douglas Anderson
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 8e7b42f843d4..0785873d1345 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-quackingstick.dtsi
@@ -99,6 +99,7 @@ ap_ts: touchscreen@10 {
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+ panel = <&panel>;
post-power-on-delay-ms = <20>;
hid-descr-addr = <0x0001>;
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
index 262d6691abd9..f70f5b42c845 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-wormdingler.dtsi
@@ -154,6 +154,7 @@ ap_ts: touchscreen@1 {
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_EDGE_FALLING>;
+ panel = <&panel>;
post-power-on-delay-ms = <70>;
hid-descr-addr = <0x0001>;
--
2.41.0.162.gfafddb0af9-goog
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-07 21:49 ` [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower Douglas Anderson
@ 2023-06-08 5:18 ` kernel test robot
2023-06-08 7:14 ` kernel test robot
2023-06-08 15:36 ` Benjamin Tissoires
2 siblings, 0 replies; 32+ messages in thread
From: kernel test robot @ 2023-06-08 5:18 UTC (permalink / raw)
To: Douglas Anderson, 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: oe-kbuild-all, Douglas Anderson, devicetree,
cros-qcom-dts-watchers, linux-arm-msm, yangcong5, Dmitry Torokhov,
linux-kernel, dri-devel, Chris Morgan, linux-input, hsinyi
Hi Douglas,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on hid/for-next dtor-input/next dtor-input/for-linus]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-touchscreens/20230608-055515
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230607144931.v2.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15%40changeid
patch subject: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
config: arc-randconfig-r021-20230607 (https://download.01.org/0day-ci/archive/20230608/202306081344.M0jNn0Ce-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git remote add robh https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
git fetch robh for-next
git checkout robh/for-next
b4 shazam https://lore.kernel.org/r/20230607144931.v2.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15@changeid
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306081344.M0jNn0Ce-lkp@intel.com/
All errors (new ones prefixed by >>):
`.exit.text' referenced in section `__jump_table' of lib/test_dynamic_debug.o: defined in discarded section `.exit.text' of lib/test_dynamic_debug.o
`.exit.text' referenced in section `__jump_table' of lib/test_dynamic_debug.o: defined in discarded section `.exit.text' of lib/test_dynamic_debug.o
`.exit.text' referenced in section `__jump_table' of drivers/misc/phantom.o: defined in discarded section `.exit.text' of drivers/misc/phantom.o
`.exit.text' referenced in section `__jump_table' of drivers/misc/phantom.o: defined in discarded section `.exit.text' of drivers/misc/phantom.o
`.exit.text' referenced in section `__jump_table' of drivers/target/target_core_configfs.o: defined in discarded section `.exit.text' of drivers/target/target_core_configfs.o
`.exit.text' referenced in section `__jump_table' of drivers/target/target_core_configfs.o: defined in discarded section `.exit.text' of drivers/target/target_core_configfs.o
arceb-elf-ld: drivers/hid/i2c-hid/i2c-hid-core.o: in function `i2c_hid_core_remove':
drivers/hid/i2c-hid/i2c-hid-core.c:1218: undefined reference to `drm_panel_remove_follower'
>> arceb-elf-ld: drivers/hid/i2c-hid/i2c-hid-core.c:1218: undefined reference to `drm_panel_remove_follower'
arceb-elf-ld: drivers/hid/i2c-hid/i2c-hid-core.o: in function `i2c_hid_core_probe':
drivers/hid/i2c-hid/i2c-hid-core.c:1159: undefined reference to `drm_panel_add_follower'
>> arceb-elf-ld: drivers/hid/i2c-hid/i2c-hid-core.c:1159: undefined reference to `drm_panel_add_follower'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-07 21:49 ` [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower Douglas Anderson
2023-06-08 5:18 ` kernel test robot
@ 2023-06-08 7:14 ` kernel test robot
2023-06-08 15:10 ` Doug Anderson
2023-06-08 15:36 ` Benjamin Tissoires
2 siblings, 1 reply; 32+ messages in thread
From: kernel test robot @ 2023-06-08 7:14 UTC (permalink / raw)
To: Douglas Anderson, 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: llvm, oe-kbuild-all, Douglas Anderson, devicetree,
cros-qcom-dts-watchers, linux-arm-msm, yangcong5, Dmitry Torokhov,
linux-kernel, dri-devel, Chris Morgan, linux-input, hsinyi
Hi Douglas,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on hid/for-next dtor-input/next dtor-input/for-linus drm-misc/drm-misc-next linus/master v6.4-rc5 next-20230607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-touchscreens/20230608-055515
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230607144931.v2.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15%40changeid
patch subject: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
config: i386-randconfig-i003-20230607 (https://download.01.org/0day-ci/archive/20230608/202306081419.Dzz0T4iW-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git remote add robh https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
git fetch robh for-next
git checkout robh/for-next
b4 shazam https://lore.kernel.org/r/20230607144931.v2.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15@changeid
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306081419.Dzz0T4iW-lkp@intel.com/
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: drm_panel_add_follower
>>> referenced by i2c-hid-core.c:1159 (drivers/hid/i2c-hid/i2c-hid-core.c:1159)
>>> drivers/hid/i2c-hid/i2c-hid-core.o:(i2c_hid_core_probe) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: drm_panel_remove_follower
>>> referenced by i2c-hid-core.c:1218 (drivers/hid/i2c-hid/i2c-hid-core.c:1218)
>>> drivers/hid/i2c-hid/i2c-hid-core.o:(i2c_hid_core_remove) in archive vmlinux.a
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
` (9 preceding siblings ...)
2023-06-07 21:49 ` [PATCH v2 10/10] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels Douglas Anderson
@ 2023-06-08 7:17 ` Maxime Ripard
2023-06-08 14:38 ` Doug Anderson
10 siblings, 1 reply; 32+ messages in thread
From: Maxime Ripard @ 2023-06-08 7:17 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, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]
Hi Douglas,
On Wed, Jun 07, 2023 at 02:49:22PM -0700, Douglas Anderson wrote:
>
> The big motivation for this patch series is mostly described in the patch
> ("drm/panel: Add a way for other devices to follow panel state"), but to
> quickly summarize here: for touchscreens that are connected to a panel we
> need the ability to power sequence the two device together. This is not a
> new need, but so far we've managed to get by through a combination of
> inefficiency, added costs, or perhaps just a little bit of brokenness.
> It's time to do better. This patch series allows us to do better.
>
> 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 2 of this patch series doesn'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. ;-)
Thanks for working on this.
I haven't seen in any of your commit messages how the panels were
actually "packaged" together?
Do a panel model typically come together with the i2c-hid support, or is
it added at manufacture time?
If it's the latter, it's indeed a fairly loose connection and we need
your work.
If it's the former though and we don't expect a given panel reference to
always (or never) come with a touchscreen attached, I guess we can have
something much simpler with a bunch of helpers that would register a
i2c-hid device and would be called by the panel driver itself.
And then, since everything is self-contained managing the power state
becomes easier as well.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
2023-06-08 7:17 ` [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Maxime Ripard
@ 2023-06-08 14:38 ` Doug Anderson
2023-06-12 16:03 ` Maxime Ripard
0 siblings, 1 reply; 32+ messages in thread
From: Doug Anderson @ 2023-06-08 14:38 UTC (permalink / raw)
To: Maxime Ripard
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, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
Hi,
On Thu, Jun 8, 2023 at 12:17 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi Douglas,
>
> On Wed, Jun 07, 2023 at 02:49:22PM -0700, Douglas Anderson wrote:
> >
> > The big motivation for this patch series is mostly described in the patch
> > ("drm/panel: Add a way for other devices to follow panel state"), but to
> > quickly summarize here: for touchscreens that are connected to a panel we
> > need the ability to power sequence the two device together. This is not a
> > new need, but so far we've managed to get by through a combination of
> > inefficiency, added costs, or perhaps just a little bit of brokenness.
> > It's time to do better. This patch series allows us to do better.
> >
> > 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 2 of this patch series doesn'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. ;-)
>
> Thanks for working on this.
>
> I haven't seen in any of your commit messages how the panels were
> actually "packaged" together?
>
> Do a panel model typically come together with the i2c-hid support, or is
> it added at manufacture time?
>
> If it's the latter, it's indeed a fairly loose connection and we need
> your work.
>
> If it's the former though and we don't expect a given panel reference to
> always (or never) come with a touchscreen attached,
Thanks for your reply. Let me see what I can do to bring clarity.
In at least some of the cases, I believe that the panel and the
touchscreen _are_ logically distinct components, even if they've been
glued together at some stage in manufacturing. Even on one of the
"poster child" boards that I talk about in patch #3, the early
versions of "homestar", I believe this to be the case. However, even
if the panel and touchscreen are separate components then they still
could be connected to the main board in a way that they share power
and/or reset signals. In my experience, in every case where they do
the EEs expect that the panel is power sequenced first and then the
touchscreen is power sequenced second. The EEs look at the power
sequencing requirements of the panel and touchscreen, see that there
is a valid power sequence protocol where they can share rails, and
design the board that way. Even if the touchscreen and panel are
logically separate, the moment the board designers hook them up to the
same power rails and/or reset signals they become tied. This is well
supported by my patch series.
The case that really motivated my patch series, though, is the case
that Cong Yang recently has been working on. I think most of the
discussion is in his original patch series [1]. Cong Yang's patch
series is largely focused on supporting the "ILI9882T" chip and some
panels that it's used with. I found a datasheet for that, and the
title from the first page is illustrative: "In-cell IC Integrates TFT
LCD Driver and Capacitive Touch Controller into a Two Chip Cascade".
This is an integrated solution that's designed to handle both the LCD
and the touchscreen.
[1] https://lore.kernel.org/lkml/20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com/
> I guess we can have
> something much simpler with a bunch of helpers that would register a
> i2c-hid device and would be called by the panel driver itself.
>
> And then, since everything is self-contained managing the power state
> becomes easier as well.
Can you give me more details about how you think this would work?
When you say that the panel would register an i2c-hid device itself,
do you mean that we'd do something like give a phandle to the i2c bus
to the panel and then the panel would manually instantiate the i2c-hid
device on it? ...and I guess it would need to be a "subclass" of
i2c-hid that knew about the connection to the panel code? This
subclass and the panel code would communicate with each other about
power sequencing needs through some private API (like MFD devices
usually do?). Assuming I'm understanding correctly, I think that could
work. Is it cleaner than my current approach, though?
I guess, alternatively, we could put the "panel" directly under the
i2c bus in this case. That would probably work for Cong Yang's current
needs, but we'd end up in trouble if we ever had a similar situation
with an eDP panel since eDP panels need to be under the DP-AUX bus.
I guess overall, though, while I think this approach could solve Cong
Yang's needs, I still feel like it's worth solving the case where
board designers have made panel and touchscreens "coupled" by having
them rely on the same power rails and/or reset signals.
-Doug
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-08 7:14 ` kernel test robot
@ 2023-06-08 15:10 ` Doug Anderson
0 siblings, 0 replies; 32+ messages in thread
From: Doug Anderson @ 2023-06-08 15:10 UTC (permalink / raw)
To: kernel test robot
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, llvm, oe-kbuild-all, devicetree,
cros-qcom-dts-watchers, linux-arm-msm, yangcong5, Dmitry Torokhov,
linux-kernel, dri-devel, Chris Morgan, linux-input, hsinyi
Hi,
On Thu, Jun 8, 2023 at 12:15 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Douglas,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on robh/for-next]
> [also build test ERROR on hid/for-next dtor-input/next dtor-input/for-linus drm-misc/drm-misc-next linus/master v6.4-rc5 next-20230607]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Douglas-Anderson/dt-bindings-HID-i2c-hid-Add-panel-property-to-i2c-hid-backed-touchscreens/20230608-055515
> base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> patch link: https://lore.kernel.org/r/20230607144931.v2.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15%40changeid
> patch subject: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
> config: i386-randconfig-i003-20230607 (https://download.01.org/0day-ci/archive/20230608/202306081419.Dzz0T4iW-lkp@intel.com/config)
> compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
> reproduce (this is a W=1 build):
> mkdir -p ~/bin
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git remote add robh https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> git fetch robh for-next
> git checkout robh/for-next
> b4 shazam https://lore.kernel.org/r/20230607144931.v2.8.Ib1a98309c455cd7e26b931c69993d4fba33bbe15@changeid
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=i386 olddefconfig
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202306081419.Dzz0T4iW-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> >> ld.lld: error: undefined symbol: drm_panel_add_follower
> >>> referenced by i2c-hid-core.c:1159 (drivers/hid/i2c-hid/i2c-hid-core.c:1159)
> >>> drivers/hid/i2c-hid/i2c-hid-core.o:(i2c_hid_core_probe) in archive vmlinux.a
> --
> >> ld.lld: error: undefined symbol: drm_panel_remove_follower
> >>> referenced by i2c-hid-core.c:1218 (drivers/hid/i2c-hid/i2c-hid-core.c:1218)
> >>> drivers/hid/i2c-hid/i2c-hid-core.o:(i2c_hid_core_remove) in archive vmlinux.a
Thanks for the report! Ugh, I guess I forgot that even though
DRM_PANEL is bool, it gets bundled up into all of DRM which can be a
module. Assuming that this series looks mostly the same in the next
version, I'll plan to add this:
depends on DRM || !DRM # if DRM=m, this can't be 'y'
...to each of the i2c-hid subclasses.
-Doug
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-07 21:49 ` [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower Douglas Anderson
2023-06-08 5:18 ` kernel test robot
2023-06-08 7:14 ` kernel test robot
@ 2023-06-08 15:36 ` Benjamin Tissoires
2023-06-08 16:42 ` Doug Anderson
2023-06-26 22:49 ` Doug Anderson
2 siblings, 2 replies; 32+ messages in thread
From: Benjamin Tissoires @ 2023-06-08 15:36 UTC (permalink / raw)
To: Douglas Anderson
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
On Jun 07 2023, Douglas Anderson wrote:
>
> As talked about in the patch ("drm/panel: Add a way for other devices
> to follow panel state"), we really want to keep the power states of a
> touchscreen and the panel it's attached to in sync with each other. In
> that spirit, add support to i2c-hid to be a panel follower. This will
> let the i2c-hid driver get informed when the panel is powered on and
> off. From there we can match the i2c-hid device's power state to that
> of the panel.
>
> NOTE: this patch specifically _doesn't_ use pm_runtime to keep track
> of / manage the power state of the i2c-hid device, even though my
> first instinct said that would be the way to go. Specific problems
> with using pm_runtime():
> * The initial power up couldn't happen in a runtime resume function
> since it create sub-devices and, apparently, that's not good to do
> in your resume function.
> * Managing our power state with pm_runtime meant fighting to make the
> right thing happen at system suspend to prevent the system from
> trying to resume us only to suspend us again. While this might be
> able to be solved, it added complexity.
> Overall the code without pm_runtime() ended up being smaller and
> easier to understand.
Generally speaking, I'm not that happy when we need to coordinate with
other subsystems for bringing up resources...
Anyway, a remark inlined (at least):
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
>
> drivers/hid/i2c-hid/i2c-hid-core.c | 82 +++++++++++++++++++++++++++++-
> 1 file changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index fa8a1ca43d7f..368db3ae612f 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -38,6 +38,8 @@
> #include <linux/mutex.h>
> #include <asm/unaligned.h>
>
> +#include <drm/drm_panel.h>
> +
> #include "../hid-ids.h"
> #include "i2c-hid.h"
>
> @@ -107,6 +109,8 @@ struct i2c_hid {
> struct mutex reset_lock;
>
> struct i2chid_ops *ops;
> + struct drm_panel_follower panel_follower;
> + bool is_panel_follower;
> };
>
> static const struct i2c_hid_quirks {
> @@ -1058,6 +1062,34 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
> return ret;
> }
>
> +static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
> +{
> + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
> + struct hid_device *hid = ihid->hid;
> +
> + /*
> + * hid->version is set on the first power up. If it's still zero then
> + * this is the first power on so we should perform initial power up
> + * steps.
> + */
> + if (!hid->version)
> + return i2c_hid_core_initial_power_up(ihid);
> +
> + return i2c_hid_core_resume(ihid);
> +}
> +
> +static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower)
> +{
> + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
> +
> + return i2c_hid_core_suspend(ihid);
> +}
> +
> +static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = {
> + .panel_prepared = i2c_hid_core_panel_prepared,
> + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> +};
Can we make that above block at least behind a Kconfig?
i2c-hid is often used for touchpads, and the notion of drm panel has
nothing to do with them. So I'd be more confident if we could disable
that code if not required.
Actually, I'd be even more happier if it were in a different compilation
unit. Not necessary a different module, but at least a different file.
Cheers,
Benjamin
> +
> int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> u16 hid_descriptor_address, u32 quirks)
> {
> @@ -1119,6 +1151,41 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> hid->bus = BUS_I2C;
> hid->initial_quirks = quirks;
>
> + /*
> + * See if we're following a panel. If drm_panel_add_follower()
> + * returns no error then we are.
> + */
> + ihid->panel_follower.funcs = &i2c_hid_core_panel_follower_funcs;
> + ret = drm_panel_add_follower(&client->dev, &ihid->panel_follower);
> + if (!ret) {
> + /* We're a follower. That means we'll power things up later. */
> + ihid->is_panel_follower = true;
> +
> + /*
> + * If we're not in control of our own power up/power down then
> + * we can't do the logic to manage wakeups. Give a warning if
> + * a user thought that was possible then force the capability
> + * off.
> + */
> + if (device_can_wakeup(&client->dev)) {
> + dev_warn(&client->dev, "Can't wakeup if following panel\n");
> + device_set_wakeup_capable(&client->dev, false);
> + }
> +
> + return 0;
> + }
> +
> + /*
> + * -ENODEV means that we're not following a panel, so any other error
> + * is a real problem (like -EPROBE_DEFER, -ENOMEM, ...).
> + */
> + if (ret != -ENODEV)
> + goto err_mem_free;
> +
> + /*
> + * We're not following a panel. That's fine and means that we
> + * can power up right away.
> + */
> ret = i2c_hid_core_initial_power_up(ihid);
> if (ret)
> goto err_mem_free;
> @@ -1143,7 +1210,14 @@ void i2c_hid_core_remove(struct i2c_client *client)
> struct i2c_hid *ihid = i2c_get_clientdata(client);
> struct hid_device *hid;
>
> - i2c_hid_core_power_down(ihid);
> + /*
> + * If we're a follower, the act of unfollowing will cause us to be
> + * powered down. Otherwise we need to manually do it.
> + */
> + if (ihid->is_panel_follower)
> + drm_panel_remove_follower(&ihid->panel_follower);
> + else
> + i2c_hid_core_power_down(ihid);
>
> hid = ihid->hid;
> hid_destroy_device(hid);
> @@ -1171,6 +1245,9 @@ static int i2c_hid_core_pm_suspend(struct device *dev)
> struct i2c_client *client = to_i2c_client(dev);
> struct i2c_hid *ihid = i2c_get_clientdata(client);
>
> + if (ihid->is_panel_follower)
> + return 0;
> +
> return i2c_hid_core_suspend(ihid);
> }
>
> @@ -1179,6 +1256,9 @@ static int i2c_hid_core_pm_resume(struct device *dev)
> struct i2c_client *client = to_i2c_client(dev);
> struct i2c_hid *ihid = i2c_get_clientdata(client);
>
> + if (ihid->is_panel_follower)
> + return 0;
> +
> return i2c_hid_core_resume(ihid);
> }
>
> --
> 2.41.0.162.gfafddb0af9-goog
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-08 15:36 ` Benjamin Tissoires
@ 2023-06-08 16:42 ` Doug Anderson
2023-06-09 9:27 ` Benjamin Tissoires
2023-06-26 22:49 ` Doug Anderson
1 sibling, 1 reply; 32+ messages in thread
From: Doug Anderson @ 2023-06-08 16:42 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
Hi,
On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
>
> On Jun 07 2023, Douglas Anderson wrote:
> >
> > As talked about in the patch ("drm/panel: Add a way for other devices
> > to follow panel state"), we really want to keep the power states of a
> > touchscreen and the panel it's attached to in sync with each other. In
> > that spirit, add support to i2c-hid to be a panel follower. This will
> > let the i2c-hid driver get informed when the panel is powered on and
> > off. From there we can match the i2c-hid device's power state to that
> > of the panel.
> >
> > NOTE: this patch specifically _doesn't_ use pm_runtime to keep track
> > of / manage the power state of the i2c-hid device, even though my
> > first instinct said that would be the way to go. Specific problems
> > with using pm_runtime():
> > * The initial power up couldn't happen in a runtime resume function
> > since it create sub-devices and, apparently, that's not good to do
> > in your resume function.
> > * Managing our power state with pm_runtime meant fighting to make the
> > right thing happen at system suspend to prevent the system from
> > trying to resume us only to suspend us again. While this might be
> > able to be solved, it added complexity.
> > Overall the code without pm_runtime() ended up being smaller and
> > easier to understand.
>
> Generally speaking, I'm not that happy when we need to coordinate with
> other subsystems for bringing up resources...
Yeah, I'd agree that it's not amazingly elegant. Unfortunately, I
couldn't find any existing clean frameworks that would do what was
needed, which is (presumably) why this problem hasn't been solved
before. I could try to come up with a grand abstraction / new
framework, but that doesn't seem like a great choice either unless we
expect more users...
> Anyway, a remark inlined (at least):
>
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v2:
> > - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
> >
> > drivers/hid/i2c-hid/i2c-hid-core.c | 82 +++++++++++++++++++++++++++++-
> > 1 file changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> > index fa8a1ca43d7f..368db3ae612f 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > @@ -38,6 +38,8 @@
> > #include <linux/mutex.h>
> > #include <asm/unaligned.h>
> >
> > +#include <drm/drm_panel.h>
> > +
> > #include "../hid-ids.h"
> > #include "i2c-hid.h"
> >
> > @@ -107,6 +109,8 @@ struct i2c_hid {
> > struct mutex reset_lock;
> >
> > struct i2chid_ops *ops;
> > + struct drm_panel_follower panel_follower;
> > + bool is_panel_follower;
> > };
> >
> > static const struct i2c_hid_quirks {
> > @@ -1058,6 +1062,34 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
> > return ret;
> > }
> >
> > +static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
> > +{
> > + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
> > + struct hid_device *hid = ihid->hid;
> > +
> > + /*
> > + * hid->version is set on the first power up. If it's still zero then
> > + * this is the first power on so we should perform initial power up
> > + * steps.
> > + */
> > + if (!hid->version)
> > + return i2c_hid_core_initial_power_up(ihid);
> > +
> > + return i2c_hid_core_resume(ihid);
> > +}
> > +
> > +static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower)
> > +{
> > + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
> > +
> > + return i2c_hid_core_suspend(ihid);
> > +}
> > +
> > +static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = {
> > + .panel_prepared = i2c_hid_core_panel_prepared,
> > + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> > +};
>
> Can we make that above block at least behind a Kconfig?
>
> i2c-hid is often used for touchpads, and the notion of drm panel has
> nothing to do with them. So I'd be more confident if we could disable
> that code if not required.
Happy to put it behind a Kconfig. I'll plan on that for v3. I'll stub
the functions out if there is no Kconfig, but plan to still leave
structure members just to avoid uglifying the sources too much.
> Actually, I'd be even more happier if it were in a different compilation
> unit. Not necessary a different module, but at least a different file.
I suspect that it's not worth it, but I'll do this if you feel
strongly about it.
I guess the simplest way I can think of to move this to its own file
would be to put the whole private data structure (struct i2c_hid) in a
private header file and then add prototypes for i2c_hid_core_resume()
and i2c_hid_core_suspend() there. Then I could add something like
i2c_hid_core_handle_panel_follower() that would have all the
registration logic. I'd still need special cases in the core
suspend/resume/remove code unless I add a level of abstraction. While
the level of abstraction is more "pure", it also would make the code
harder to follow.
Unless I hear a strong opinion (or if this series changes
significantly), I'll plan to keep things in the same file and just use
a Kconfig.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-08 16:42 ` Doug Anderson
@ 2023-06-09 9:27 ` Benjamin Tissoires
2023-06-09 15:01 ` Doug Anderson
0 siblings, 1 reply; 32+ messages in thread
From: Benjamin Tissoires @ 2023-06-09 9:27 UTC (permalink / raw)
To: Doug Anderson
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
On Thu, Jun 8, 2023 at 6:43 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> >
> > On Jun 07 2023, Douglas Anderson wrote:
> > >
> > > As talked about in the patch ("drm/panel: Add a way for other devices
> > > to follow panel state"), we really want to keep the power states of a
> > > touchscreen and the panel it's attached to in sync with each other. In
> > > that spirit, add support to i2c-hid to be a panel follower. This will
> > > let the i2c-hid driver get informed when the panel is powered on and
> > > off. From there we can match the i2c-hid device's power state to that
> > > of the panel.
> > >
> > > NOTE: this patch specifically _doesn't_ use pm_runtime to keep track
> > > of / manage the power state of the i2c-hid device, even though my
> > > first instinct said that would be the way to go. Specific problems
> > > with using pm_runtime():
> > > * The initial power up couldn't happen in a runtime resume function
> > > since it create sub-devices and, apparently, that's not good to do
> > > in your resume function.
> > > * Managing our power state with pm_runtime meant fighting to make the
> > > right thing happen at system suspend to prevent the system from
> > > trying to resume us only to suspend us again. While this might be
> > > able to be solved, it added complexity.
> > > Overall the code without pm_runtime() ended up being smaller and
> > > easier to understand.
> >
> > Generally speaking, I'm not that happy when we need to coordinate with
> > other subsystems for bringing up resources...
>
> Yeah, I'd agree that it's not amazingly elegant. Unfortunately, I
> couldn't find any existing clean frameworks that would do what was
> needed, which is (presumably) why this problem hasn't been solved
> before. I could try to come up with a grand abstraction / new
> framework, but that doesn't seem like a great choice either unless we
> expect more users...
>
>
> > Anyway, a remark inlined (at least):
> >
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
> > >
> > > drivers/hid/i2c-hid/i2c-hid-core.c | 82 +++++++++++++++++++++++++++++-
> > > 1 file changed, 81 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> > > index fa8a1ca43d7f..368db3ae612f 100644
> > > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > > @@ -38,6 +38,8 @@
> > > #include <linux/mutex.h>
> > > #include <asm/unaligned.h>
> > >
> > > +#include <drm/drm_panel.h>
> > > +
> > > #include "../hid-ids.h"
> > > #include "i2c-hid.h"
> > >
> > > @@ -107,6 +109,8 @@ struct i2c_hid {
> > > struct mutex reset_lock;
> > >
> > > struct i2chid_ops *ops;
> > > + struct drm_panel_follower panel_follower;
> > > + bool is_panel_follower;
> > > };
> > >
> > > static const struct i2c_hid_quirks {
> > > @@ -1058,6 +1062,34 @@ static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
> > > return ret;
> > > }
> > >
> > > +static int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower)
> > > +{
> > > + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
> > > + struct hid_device *hid = ihid->hid;
> > > +
> > > + /*
> > > + * hid->version is set on the first power up. If it's still zero then
> > > + * this is the first power on so we should perform initial power up
> > > + * steps.
> > > + */
> > > + if (!hid->version)
> > > + return i2c_hid_core_initial_power_up(ihid);
> > > +
> > > + return i2c_hid_core_resume(ihid);
> > > +}
> > > +
> > > +static int i2c_hid_core_panel_unpreparing(struct drm_panel_follower *follower)
> > > +{
> > > + struct i2c_hid *ihid = container_of(follower, struct i2c_hid, panel_follower);
> > > +
> > > + return i2c_hid_core_suspend(ihid);
> > > +}
> > > +
> > > +static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = {
> > > + .panel_prepared = i2c_hid_core_panel_prepared,
> > > + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> > > +};
> >
> > Can we make that above block at least behind a Kconfig?
> >
> > i2c-hid is often used for touchpads, and the notion of drm panel has
> > nothing to do with them. So I'd be more confident if we could disable
> > that code if not required.
>
> Happy to put it behind a Kconfig. I'll plan on that for v3. I'll stub
> the functions out if there is no Kconfig, but plan to still leave
> structure members just to avoid uglifying the sources too much.
>
>
> > Actually, I'd be even more happier if it were in a different compilation
> > unit. Not necessary a different module, but at least a different file.
>
> I suspect that it's not worth it, but I'll do this if you feel
> strongly about it.
>
> I guess the simplest way I can think of to move this to its own file
> would be to put the whole private data structure (struct i2c_hid) in a
> private header file and then add prototypes for i2c_hid_core_resume()
> and i2c_hid_core_suspend() there. Then I could add something like
> i2c_hid_core_handle_panel_follower() that would have all the
> registration logic. I'd still need special cases in the core
> suspend/resume/remove code unless I add a level of abstraction. While
> the level of abstraction is more "pure", it also would make the code
> harder to follow.
>
> Unless I hear a strong opinion (or if this series changes
> significantly), I'll plan to keep things in the same file and just use
> a Kconfig.
>
Right, a separate file might not be the best then :(
Do you envision this to be used on the ACPI side of i2c-hid? Because
if this is OF only, then maybe it would be interesting to put it there
(in i2c-hid-of.c), instead of having it in the core. IIRC i2c-hid-of
also has ways to set up/down regulators, so maybe it'll be better
there?
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-09 9:27 ` Benjamin Tissoires
@ 2023-06-09 15:01 ` Doug Anderson
0 siblings, 0 replies; 32+ messages in thread
From: Doug Anderson @ 2023-06-09 15:01 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
Hi,
On Fri, Jun 9, 2023 at 2:27 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> > I suspect that it's not worth it, but I'll do this if you feel
> > strongly about it.
> >
> > I guess the simplest way I can think of to move this to its own file
> > would be to put the whole private data structure (struct i2c_hid) in a
> > private header file and then add prototypes for i2c_hid_core_resume()
> > and i2c_hid_core_suspend() there. Then I could add something like
> > i2c_hid_core_handle_panel_follower() that would have all the
> > registration logic. I'd still need special cases in the core
> > suspend/resume/remove code unless I add a level of abstraction. While
> > the level of abstraction is more "pure", it also would make the code
> > harder to follow.
> >
> > Unless I hear a strong opinion (or if this series changes
> > significantly), I'll plan to keep things in the same file and just use
> > a Kconfig.
> >
>
> Right, a separate file might not be the best then :(
>
> Do you envision this to be used on the ACPI side of i2c-hid? Because
> if this is OF only, then maybe it would be interesting to put it there
> (in i2c-hid-of.c), instead of having it in the core. IIRC i2c-hid-of
> also has ways to set up/down regulators, so maybe it'll be better
> there?
There is no reason why this problem would be limited to devices using
devicetree. Even if ACPI could somehow magically power sequence the
touchscreen and panel together, doing it behind the back of the kernel
driver would be a bad idea anyway so folks using ACPI would need the
same code. I don't have tons of experience with ACPI nor how to hook
this up there, but I purposely made the API for registering the panel
follower such that the client doesn't pass anything devicetree
specific. If someone could figure out how to detect a link between a
panel and a touchscreen for ACPI and add this code to
drm_panel_add_follower() then it would automatically work for the ACPI
case as well.
-Doug
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens
2023-06-07 21:49 ` [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens Douglas Anderson
@ 2023-06-09 15:53 ` Krzysztof Kozlowski
0 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-09 15:53 UTC (permalink / raw)
To: Douglas Anderson, 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: dri-devel, Dmitry Torokhov, linux-input, Daniel Vetter,
linux-kernel, hsinyi, cros-qcom-dts-watchers, devicetree,
yangcong5, linux-arm-msm, Chris Morgan
On 07/06/2023 23:49, Douglas Anderson wrote:
> 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.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - Move the description to the generic touchscreen.yaml.
> - Update the desc to make it clearer it's only for integrated devices.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers
2023-06-07 21:49 ` [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers Douglas Anderson
@ 2023-06-09 16:10 ` Rob Herring
0 siblings, 0 replies; 32+ messages in thread
From: Rob Herring @ 2023-06-09 16:10 UTC (permalink / raw)
To: Douglas Anderson
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
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
On Wed, Jun 07, 2023 at 02:49:26PM -0700, Douglas Anderson wrote:
> 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.
>
> 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.
>
> Changes in v2:
> - ("Add a devlink for panel followers") new for v2.
>
> drivers/of/property.c | 2 ++
> 1 file changed, 2 insertions(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
2023-06-08 14:38 ` Doug Anderson
@ 2023-06-12 16:03 ` Maxime Ripard
2023-06-12 21:13 ` Doug Anderson
0 siblings, 1 reply; 32+ messages in thread
From: Maxime Ripard @ 2023-06-12 16:03 UTC (permalink / raw)
To: Doug 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, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
[-- Attachment #1: Type: text/plain, Size: 6517 bytes --]
Hi Doug,
On Thu, Jun 08, 2023 at 07:38:58AM -0700, Doug Anderson wrote:
> On Thu, Jun 8, 2023 at 12:17 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi Douglas,
> >
> > On Wed, Jun 07, 2023 at 02:49:22PM -0700, Douglas Anderson wrote:
> > >
> > > The big motivation for this patch series is mostly described in the patch
> > > ("drm/panel: Add a way for other devices to follow panel state"), but to
> > > quickly summarize here: for touchscreens that are connected to a panel we
> > > need the ability to power sequence the two device together. This is not a
> > > new need, but so far we've managed to get by through a combination of
> > > inefficiency, added costs, or perhaps just a little bit of brokenness.
> > > It's time to do better. This patch series allows us to do better.
> > >
> > > 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 2 of this patch series doesn'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. ;-)
> >
> > Thanks for working on this.
> >
> > I haven't seen in any of your commit messages how the panels were
> > actually "packaged" together?
> >
> > Do a panel model typically come together with the i2c-hid support, or is
> > it added at manufacture time?
> >
> > If it's the latter, it's indeed a fairly loose connection and we need
> > your work.
> >
> > If it's the former though and we don't expect a given panel reference to
> > always (or never) come with a touchscreen attached,
>
> Thanks for your reply. Let me see what I can do to bring clarity.
>
> In at least some of the cases, I believe that the panel and the
> touchscreen _are_ logically distinct components, even if they've been
> glued together at some stage in manufacturing. Even on one of the
> "poster child" boards that I talk about in patch #3, the early
> versions of "homestar", I believe this to be the case. However, even
> if the panel and touchscreen are separate components then they still
> could be connected to the main board in a way that they share power
> and/or reset signals. In my experience, in every case where they do
> the EEs expect that the panel is power sequenced first and then the
> touchscreen is power sequenced second. The EEs look at the power
> sequencing requirements of the panel and touchscreen, see that there
> is a valid power sequence protocol where they can share rails, and
> design the board that way. Even if the touchscreen and panel are
> logically separate, the moment the board designers hook them up to the
> same power rails and/or reset signals they become tied. This is well
> supported by my patch series.
>
> The case that really motivated my patch series, though, is the case
> that Cong Yang recently has been working on. I think most of the
> discussion is in his original patch series [1]. Cong Yang's patch
> series is largely focused on supporting the "ILI9882T" chip and some
> panels that it's used with. I found a datasheet for that, and the
> title from the first page is illustrative: "In-cell IC Integrates TFT
> LCD Driver and Capacitive Touch Controller into a Two Chip Cascade".
> This is an integrated solution that's designed to handle both the LCD
> and the touchscreen.
>
> [1] https://lore.kernel.org/lkml/20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com/
Ok, I think we're on the same page at the hardware level then :)
> > I guess we can have
> > something much simpler with a bunch of helpers that would register a
> > i2c-hid device and would be called by the panel driver itself.
> >
> > And then, since everything is self-contained managing the power state
> > becomes easier as well.
>
> Can you give me more details about how you think this would work?
>
> When you say that the panel would register an i2c-hid device itself,
> do you mean that we'd do something like give a phandle to the i2c bus
> to the panel and then the panel would manually instantiate the i2c-hid
> device on it? ...and I guess it would need to be a "subclass" of
> i2c-hid that knew about the connection to the panel code? This
> subclass and the panel code would communicate with each other about
> power sequencing needs through some private API (like MFD devices
> usually do?). Assuming I'm understanding correctly, I think that could
> work.
I guess what I had in mind is to do something similar to what we're
doing with hdmi-codec already for example.
We have several logical components already, in separate drivers, that
still need some cooperation.
If the panel and touchscreen are on the same i2c bus, I think we could
even just get a reference to the panel i2c adapter, get a reference, and
pass that to i2c-hid (with a nice layer of helpers).
What I'm trying to say is: could we just make it work by passing a bunch
of platform_data, 2-3 callbacks and a device registration from the panel
driver directly?
> Is it cleaner than my current approach, though?
"cleaner" is subjective, really, but it's a more "mainstream" approach
that one can follow more easily through function calls.
> I guess, alternatively, we could put the "panel" directly under the
> i2c bus in this case. That would probably work for Cong Yang's current
> needs, but we'd end up in trouble if we ever had a similar situation
> with an eDP panel since eDP panels need to be under the DP-AUX bus.
I don't know DP-AUX very well, what is the issue that you're mentioning?
> I guess overall, though, while I think this approach could solve Cong
> Yang's needs, I still feel like it's worth solving the case where
> board designers have made panel and touchscreens "coupled" by having
> them rely on the same power rails and/or reset signals.
Sure, I definitely want that too :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
2023-06-12 16:03 ` Maxime Ripard
@ 2023-06-12 21:13 ` Doug Anderson
2023-06-13 12:06 ` Maxime Ripard
0 siblings, 1 reply; 32+ messages in thread
From: Doug Anderson @ 2023-06-12 21:13 UTC (permalink / raw)
To: Maxime Ripard
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, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
Hi,
On Mon, Jun 12, 2023 at 9:03 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> > > I guess we can have
> > > something much simpler with a bunch of helpers that would register a
> > > i2c-hid device and would be called by the panel driver itself.
> > >
> > > And then, since everything is self-contained managing the power state
> > > becomes easier as well.
> >
> > Can you give me more details about how you think this would work?
> >
> > When you say that the panel would register an i2c-hid device itself,
> > do you mean that we'd do something like give a phandle to the i2c bus
> > to the panel and then the panel would manually instantiate the i2c-hid
> > device on it? ...and I guess it would need to be a "subclass" of
> > i2c-hid that knew about the connection to the panel code? This
> > subclass and the panel code would communicate with each other about
> > power sequencing needs through some private API (like MFD devices
> > usually do?). Assuming I'm understanding correctly, I think that could
> > work.
>
> I guess what I had in mind is to do something similar to what we're
> doing with hdmi-codec already for example.
By this you mean "rockchip,hdmi-codec" and "mediatek,hdmi-codec", right?
> We have several logical components already, in separate drivers, that
> still need some cooperation.
>
> If the panel and touchscreen are on the same i2c bus, I think we could
> even just get a reference to the panel i2c adapter, get a reference, and
> pass that to i2c-hid (with a nice layer of helpers).
Just for reference: the panel and touchscreen aren't on the same i2c
bus. In the cases that I've looked at the panel is either controlled
entirely by eDP or MIPI signals and isn't on any i2c bus at all. The
touchscreen is on the i2c bus in the cases I've looked at, though I
suppose I could imagine one that used a different bus.
> What I'm trying to say is: could we just make it work by passing a bunch
> of platform_data, 2-3 callbacks and a device registration from the panel
> driver directly?
I think I'm still confused about what you're proposing. Sorry! :( Let
me try rephrasing why I'm confused and perhaps we can get on the same
page. :-)
First, I guess I'm confused about how you have one of these devices
"register" the other device.
I can understand how one device might "register" its sub-devices in
the MFD case. To make it concrete, we can look at a PMIC like
max77686.c. The parent MFD device gets probed and then it's in charge
of creating all of its sub-devices. These sub-devices are intimately
tied to one another. They have shared data structures and can
coordinate power sequencing and whatnot. All good.
...but here, we really have something different in two fundamental ways:
a) In this case, the two components (panel and touchscreen) both use
separate primary communication methods. In DT the primary
communication method determines where the device is described in the
hierarchy. For eDP, this means that the DT node for the panel should
be under the eDP controller. For an i2c touchscreen, this means that
the DT node for the touchscreen should be under the i2c controller.
Describing things like this causes the eDP controller to "register"
the panel and the i2c controller to "register" the touchscreen. If we
wanted the panel driver to "register" the touchscreen then it would
get really awkward. Do we leave the touchscreen DT node under the i2c
controller but somehow tell the i2c subsytem not to register it? Do we
try to dynamically construct the touchscreen i2c node? Do we make a
fake i2c controller under our panel DT node and somehow tell the i2c
core to look at it?
b) Things are different because the two devices here are not nearly as
intimately tied to one another. At least in the case of "homestar",
the only reason that the devices were tied to one another was because
the board designers chose to share power rails, but otherwise the
drivers were both generic.
In any case, is there any chance that we're in violent agreement and
that if you dig into my design more you might like it? Other than the
fact that the panel doesn't "register" the touchscreen device, it
kinda sounds as if what my patches are already doing is roughly what
you're describing. The touchscreen and panel driver are really just
coordinating with each other through a shared data structure (struct
drm_panel_follower) that has a few callback functions. Just like with
"hdmi-codec", the devices probe separately but find each other through
a phandle. The coordination between the two happens through a few
simple helper functions.
> > Is it cleaner than my current approach, though?
>
> "cleaner" is subjective, really, but it's a more "mainstream" approach
> that one can follow more easily through function calls.
>
> > I guess, alternatively, we could put the "panel" directly under the
> > i2c bus in this case. That would probably work for Cong Yang's current
> > needs, but we'd end up in trouble if we ever had a similar situation
> > with an eDP panel since eDP panels need to be under the DP-AUX bus.
>
> I don't know DP-AUX very well, what is the issue that you're mentioning?
Hopefully I've explained what I meant above (see point "a)").
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
2023-06-12 21:13 ` Doug Anderson
@ 2023-06-13 12:06 ` Maxime Ripard
2023-06-13 15:56 ` Doug Anderson
0 siblings, 1 reply; 32+ messages in thread
From: Maxime Ripard @ 2023-06-13 12:06 UTC (permalink / raw)
To: Doug 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, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
[-- Attachment #1: Type: text/plain, Size: 6926 bytes --]
On Mon, Jun 12, 2023 at 02:13:46PM -0700, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 12, 2023 at 9:03 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > > > I guess we can have
> > > > something much simpler with a bunch of helpers that would register a
> > > > i2c-hid device and would be called by the panel driver itself.
> > > >
> > > > And then, since everything is self-contained managing the power state
> > > > becomes easier as well.
> > >
> > > Can you give me more details about how you think this would work?
> > >
> > > When you say that the panel would register an i2c-hid device itself,
> > > do you mean that we'd do something like give a phandle to the i2c bus
> > > to the panel and then the panel would manually instantiate the i2c-hid
> > > device on it? ...and I guess it would need to be a "subclass" of
> > > i2c-hid that knew about the connection to the panel code? This
> > > subclass and the panel code would communicate with each other about
> > > power sequencing needs through some private API (like MFD devices
> > > usually do?). Assuming I'm understanding correctly, I think that could
> > > work.
> >
> > I guess what I had in mind is to do something similar to what we're
> > doing with hdmi-codec already for example.
>
> By this you mean "rockchip,hdmi-codec" and "mediatek,hdmi-codec", right?
No, sorry it was a bit ambiguous. I meant how we instantiate the
hdmi-codec driver here for example:
https://elixir.bootlin.com/linux/v6.3.7/source/drivers/gpu/drm/exynos/exynos_hdmi.c#L1665
https://elixir.bootlin.com/linux/v6.3.7/source/drivers/gpu/drm/vc4/vc4_hdmi.c#L2539
https://elixir.bootlin.com/linux/v6.3.7/source/drivers/gpu/drm/tegra/hdmi.c#L1525
> > We have several logical components already, in separate drivers, that
> > still need some cooperation.
> >
> > If the panel and touchscreen are on the same i2c bus, I think we could
> > even just get a reference to the panel i2c adapter, get a reference, and
> > pass that to i2c-hid (with a nice layer of helpers).
>
> Just for reference: the panel and touchscreen aren't on the same i2c
> bus. In the cases that I've looked at the panel is either controlled
> entirely by eDP or MIPI signals and isn't on any i2c bus at all. The
> touchscreen is on the i2c bus in the cases I've looked at, though I
> suppose I could imagine one that used a different bus.
Ok, so we would indeed need a phandle to the i2c controller
> > What I'm trying to say is: could we just make it work by passing a bunch
> > of platform_data, 2-3 callbacks and a device registration from the panel
> > driver directly?
>
> I think I'm still confused about what you're proposing. Sorry! :( Let
> me try rephrasing why I'm confused and perhaps we can get on the same
> page. :-)
>
> First, I guess I'm confused about how you have one of these devices
> "register" the other device.
>
> I can understand how one device might "register" its sub-devices in
> the MFD case. To make it concrete, we can look at a PMIC like
> max77686.c. The parent MFD device gets probed and then it's in charge
> of creating all of its sub-devices. These sub-devices are intimately
> tied to one another. They have shared data structures and can
> coordinate power sequencing and whatnot. All good.
We don't necessarily need to use MFD, but yeah, we could just register a
device for the i2c-hid driver to probe from (using
i2c_new_client_device?)
> ...but here, we really have something different in two fundamental ways:
>
> a) In this case, the two components (panel and touchscreen) both use
> separate primary communication methods. In DT the primary
> communication method determines where the device is described in the
> hierarchy. For eDP, this means that the DT node for the panel should
> be under the eDP controller. For an i2c touchscreen, this means that
> the DT node for the touchscreen should be under the i2c controller.
> Describing things like this causes the eDP controller to "register"
> the panel and the i2c controller to "register" the touchscreen. If we
> wanted the panel driver to "register" the touchscreen then it would
> get really awkward. Do we leave the touchscreen DT node under the i2c
> controller but somehow tell the i2c subsytem not to register it? Do we
> try to dynamically construct the touchscreen i2c node? Do we make a
> fake i2c controller under our panel DT node and somehow tell the i2c
> core to look at it?
I would expect not to have any DT node for the touchscreen, but we would
register a new i2c device on the bus that it's connected to.
In essence, it's also fairly similar to what we're doing with
i2c_new_ancillary_device() on some bridges. Except the primary device
isn't necessarily controlled through the I2C bus (but could be, I'm
pretty sure we have that situation for RGB or LVDS panels too).
The plus side would also be that we don't really need a DT to make it
work either. We just need the panel driver to probe somehow and a
pointer to the i2c_adapter.
> b) Things are different because the two devices here are not nearly as
> intimately tied to one another. At least in the case of "homestar",
> the only reason that the devices were tied to one another was because
> the board designers chose to share power rails, but otherwise the
> drivers were both generic.
Yeah, and that's fine I guess?
> In any case, is there any chance that we're in violent agreement
Is it even violent? Sorry if it came across that way, it's really isn't
on my end.
> and that if you dig into my design more you might like it? Other than
> the fact that the panel doesn't "register" the touchscreen device, it
> kinda sounds as if what my patches are already doing is roughly what
> you're describing. The touchscreen and panel driver are really just
> coordinating with each other through a shared data structure (struct
> drm_panel_follower) that has a few callback functions. Just like with
> "hdmi-codec", the devices probe separately but find each other through
> a phandle. The coordination between the two happens through a few
> simple helper functions.
I guess we very much agree on the end-goal, and I'd really like to get
this addressed somehow. There's a couple of things I'm not really
sold on with your proposal though:
- It creates a ad-hoc KMS API for some problem that looks fairly
generic. It's also redundant with the notifier mechanism without
using it (probably for the best though).
- MIPI-DSI panel probe sequence is already fairly complex and fragile
(See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges).
I'd rather avoid creating a new dependency in that graph.
- And yeah, to some extent it's inconsistent with how we dealt with
secondary devices in KMS so far.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
2023-06-13 12:06 ` Maxime Ripard
@ 2023-06-13 15:56 ` Doug Anderson
2023-06-21 16:31 ` Doug Anderson
2023-06-23 9:08 ` Maxime Ripard
0 siblings, 2 replies; 32+ messages in thread
From: Doug Anderson @ 2023-06-13 15:56 UTC (permalink / raw)
To: Maxime Ripard
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, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
Hi,
On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> > > What I'm trying to say is: could we just make it work by passing a bunch
> > > of platform_data, 2-3 callbacks and a device registration from the panel
> > > driver directly?
> >
> > I think I'm still confused about what you're proposing. Sorry! :( Let
> > me try rephrasing why I'm confused and perhaps we can get on the same
> > page. :-)
> >
> > First, I guess I'm confused about how you have one of these devices
> > "register" the other device.
> >
> > I can understand how one device might "register" its sub-devices in
> > the MFD case. To make it concrete, we can look at a PMIC like
> > max77686.c. The parent MFD device gets probed and then it's in charge
> > of creating all of its sub-devices. These sub-devices are intimately
> > tied to one another. They have shared data structures and can
> > coordinate power sequencing and whatnot. All good.
>
> We don't necessarily need to use MFD, but yeah, we could just register a
> device for the i2c-hid driver to probe from (using
> i2c_new_client_device?)
I think this can work for devices where the panel and touchscreen are
truly integrated where the panel driver knows enough about the related
touchscreen to fully describe and instantiate it. It doesn't work
quite as well for cases where the power and reset lines are shared
just because of what a given board designer did. To handle that, each
panel driver would need to get enough DT properties added to it so
that it could fully describe any arbitrary touchscreen, right?
Let's think about the generic panel-edp driver. This driver runs the
panel on many sc7180-trogdor laptops, including coachz, lazor, and
pompom. All three of those boards have a shared power rail for the
touchscreen and panel. If you look at "sc7180-trogdor-coachz.dtsi",
you can see the touchscreen currently looks like this:
ap_ts: touchscreen@5d {
compatible = "goodix,gt7375p";
reg = <0x5d>;
pinctrl-names = "default";
pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
vdd-supply = <&pp3300_ts>;
};
In "sc7180-trogdor-lazor.dtsi" we have:
ap_ts: touchscreen@10 {
compatible = "hid-over-i2c";
reg = <0x10>;
pinctrl-names = "default";
pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
interrupt-parent = <&tlmm>;
interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
post-power-on-delay-ms = <20>;
hid-descr-addr = <0x0001>;
vdd-supply = <&pp3300_ts>;
};
In both cases "pp3300_ts" is simply another name for "pp3300_dx_edp"
So I think to do what you propose, we need to add this information to
the panel-edp DT node so that it could dynamically construct the i2c
device for the touchscreen:
a) Which touchscreen is actually connected (generic hid-over-i2c,
goodix, ...). I guess this would be a "compatible" string?
b) Which i2c bus that device is hooked up to.
c) Which i2c address that device is hooked up to.
d) What the touchscreen interrupt GPIO is.
e) Possibly what the "hid-descr-addr" for the touchscreen is.
f) Any extra timing information needed to be passed to the touchscreen
driver, like "post-power-on-delay-ms"
The "pinctrl" stuff would be easy to subsume into the panel's DT node,
at least. ...and, in this case, we could skip the "vdd-supply" since
the panel and eDP are sharing power rails (which is what got us into
this situation). ...but, the above is still a lot. At this point, it
would make sense to have a sub-node under the panel to describe it,
which we could do but it starts to feel weird. We'd essentially be
describing an i2c device but not under the i2c controller it belongs
to.
I guess I'd also say that the above design also need additional code
if/when someone had a touchscreen that used a different communication
method, like SPI.
So I guess the tl;dr of all the above is that I think it could all work if:
1. We described the touchscreen in a sub-node of the panel.
2. We added a property to the panel saying what the true parent of the
touchscreen was (an I2C controller, a SPI controller, ...) and what
type of controller it was ("SPI" vs "I2C").
3. We added some generic helpers that panels could call that would
understand how to instantiate the touchscreen under the appropriate
controller.
4. From there, we added a new private / generic API between panels and
touchscreens letting them know that the panel was turning on/off.
That seems much more complex to me, though. It also seems like an
awkward way to describe it in DT.
> > In any case, is there any chance that we're in violent agreement
>
> Is it even violent? Sorry if it came across that way, it's really isn't
> on my end.
Sorry, maybe a poor choice of words on my end. I've heard that term
thrown about when two people spend a lot of time discussing something
/ trying to persuade the other person only to find out in the end that
they were both on the same side of the issue. ;-)
> > and that if you dig into my design more you might like it? Other than
> > the fact that the panel doesn't "register" the touchscreen device, it
> > kinda sounds as if what my patches are already doing is roughly what
> > you're describing. The touchscreen and panel driver are really just
> > coordinating with each other through a shared data structure (struct
> > drm_panel_follower) that has a few callback functions. Just like with
> > "hdmi-codec", the devices probe separately but find each other through
> > a phandle. The coordination between the two happens through a few
> > simple helper functions.
>
> I guess we very much agree on the end-goal, and I'd really like to get
> this addressed somehow. There's a couple of things I'm not really
> sold on with your proposal though:
>
> - It creates a ad-hoc KMS API for some problem that looks fairly
> generic. It's also redundant with the notifier mechanism without
> using it (probably for the best though).
>
> - MIPI-DSI panel probe sequence is already fairly complex and fragile
> (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges).
> I'd rather avoid creating a new dependency in that graph.
>
> - And yeah, to some extent it's inconsistent with how we dealt with
> secondary devices in KMS so far.
Hmmmm. To a large extent, my current implementation actually has no
impact on the DRM probe sequence. The panel itself never looks for the
touchscreen code and everything DRM-related can register without a
care in the world. From reading your bullet points, I guess that's
both a strength and a weakness of my current proposal. It's really
outside the world of bridge chains and DRM components which makes it a
special snowflake that people need to understand on its own. ...but,
at the same time, the fact that it is outside all the rest of that
stuff means it doesn't add complexity to an already complex system.
I guess I'd point to the panel backlight as a preexisting design
that's not totally unlike what I'm doing. The backlight is not part of
the DRM bridge chain and doesn't fit in like other components. This
actually makes sense since the backlight doesn't take in or put out
video data and it's simply something associated with the panel. The
backlight also has a loose connection to the panel driver and a given
panel could be associated with any number of different backlight
drivers depending on the board design. I guess one difference between
the backlight and what I'm doing with "panel follower" is that we
typically don't let the panel probe until after the backlight has
probed. In the case of my "panel follower" proposal it's the opposite.
As per above, from a DRM probe point of view this actually makes my
proposal less intrusive. I guess also a difference between backlight
and "panel follower" is that I allow an arbitrary number of followers
but there's only one backlight.
One additional note: if I actually make the panel probe function start
registering the touchscreen, that actually _does_ add more complexity
to the already complex DRM probe ordering. It's yet another thing that
could fail and/or defer...
Also, I'm curious: would my proposal be more or less palatable if I
made it less generic? Instead of "panel follower", I could hardcode it
to "touchscreen" and then remove all the list management. From a DRM
point of view this would make it even more like the preexisting
"backlight" except for the ordering difference.
-Doug
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
2023-06-13 15:56 ` Doug Anderson
@ 2023-06-21 16:31 ` Doug Anderson
2023-06-23 9:08 ` Maxime Ripard
1 sibling, 0 replies; 32+ messages in thread
From: Doug Anderson @ 2023-06-21 16:31 UTC (permalink / raw)
To: Maxime Ripard
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, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
Maxime,
On Tue, Jun 13, 2023 at 8:56 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > > > What I'm trying to say is: could we just make it work by passing a bunch
> > > > of platform_data, 2-3 callbacks and a device registration from the panel
> > > > driver directly?
> > >
> > > I think I'm still confused about what you're proposing. Sorry! :( Let
> > > me try rephrasing why I'm confused and perhaps we can get on the same
> > > page. :-)
> > >
> > > First, I guess I'm confused about how you have one of these devices
> > > "register" the other device.
> > >
> > > I can understand how one device might "register" its sub-devices in
> > > the MFD case. To make it concrete, we can look at a PMIC like
> > > max77686.c. The parent MFD device gets probed and then it's in charge
> > > of creating all of its sub-devices. These sub-devices are intimately
> > > tied to one another. They have shared data structures and can
> > > coordinate power sequencing and whatnot. All good.
> >
> > We don't necessarily need to use MFD, but yeah, we could just register a
> > device for the i2c-hid driver to probe from (using
> > i2c_new_client_device?)
>
> I think this can work for devices where the panel and touchscreen are
> truly integrated where the panel driver knows enough about the related
> touchscreen to fully describe and instantiate it. It doesn't work
> quite as well for cases where the power and reset lines are shared
> just because of what a given board designer did. To handle that, each
> panel driver would need to get enough DT properties added to it so
> that it could fully describe any arbitrary touchscreen, right?
>
> Let's think about the generic panel-edp driver. This driver runs the
> panel on many sc7180-trogdor laptops, including coachz, lazor, and
> pompom. All three of those boards have a shared power rail for the
> touchscreen and panel. If you look at "sc7180-trogdor-coachz.dtsi",
> you can see the touchscreen currently looks like this:
>
> ap_ts: touchscreen@5d {
> compatible = "goodix,gt7375p";
> reg = <0x5d>;
> pinctrl-names = "default";
> pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
>
> interrupt-parent = <&tlmm>;
> interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
>
> reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
>
> vdd-supply = <&pp3300_ts>;
> };
>
> In "sc7180-trogdor-lazor.dtsi" we have:
>
> ap_ts: touchscreen@10 {
> compatible = "hid-over-i2c";
> reg = <0x10>;
> pinctrl-names = "default";
> pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
>
> interrupt-parent = <&tlmm>;
> interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
>
> post-power-on-delay-ms = <20>;
> hid-descr-addr = <0x0001>;
>
> vdd-supply = <&pp3300_ts>;
> };
>
> In both cases "pp3300_ts" is simply another name for "pp3300_dx_edp"
>
> So I think to do what you propose, we need to add this information to
> the panel-edp DT node so that it could dynamically construct the i2c
> device for the touchscreen:
>
> a) Which touchscreen is actually connected (generic hid-over-i2c,
> goodix, ...). I guess this would be a "compatible" string?
>
> b) Which i2c bus that device is hooked up to.
>
> c) Which i2c address that device is hooked up to.
>
> d) What the touchscreen interrupt GPIO is.
>
> e) Possibly what the "hid-descr-addr" for the touchscreen is.
>
> f) Any extra timing information needed to be passed to the touchscreen
> driver, like "post-power-on-delay-ms"
>
> The "pinctrl" stuff would be easy to subsume into the panel's DT node,
> at least. ...and, in this case, we could skip the "vdd-supply" since
> the panel and eDP are sharing power rails (which is what got us into
> this situation). ...but, the above is still a lot. At this point, it
> would make sense to have a sub-node under the panel to describe it,
> which we could do but it starts to feel weird. We'd essentially be
> describing an i2c device but not under the i2c controller it belongs
> to.
>
> I guess I'd also say that the above design also need additional code
> if/when someone had a touchscreen that used a different communication
> method, like SPI.
>
>
> So I guess the tl;dr of all the above is that I think it could all work if:
>
> 1. We described the touchscreen in a sub-node of the panel.
>
> 2. We added a property to the panel saying what the true parent of the
> touchscreen was (an I2C controller, a SPI controller, ...) and what
> type of controller it was ("SPI" vs "I2C").
>
> 3. We added some generic helpers that panels could call that would
> understand how to instantiate the touchscreen under the appropriate
> controller.
>
> 4. From there, we added a new private / generic API between panels and
> touchscreens letting them know that the panel was turning on/off.
>
> That seems much more complex to me, though. It also seems like an
> awkward way to describe it in DT.
>
>
> > > In any case, is there any chance that we're in violent agreement
> >
> > Is it even violent? Sorry if it came across that way, it's really isn't
> > on my end.
>
> Sorry, maybe a poor choice of words on my end. I've heard that term
> thrown about when two people spend a lot of time discussing something
> / trying to persuade the other person only to find out in the end that
> they were both on the same side of the issue. ;-)
>
>
> > > and that if you dig into my design more you might like it? Other than
> > > the fact that the panel doesn't "register" the touchscreen device, it
> > > kinda sounds as if what my patches are already doing is roughly what
> > > you're describing. The touchscreen and panel driver are really just
> > > coordinating with each other through a shared data structure (struct
> > > drm_panel_follower) that has a few callback functions. Just like with
> > > "hdmi-codec", the devices probe separately but find each other through
> > > a phandle. The coordination between the two happens through a few
> > > simple helper functions.
> >
> > I guess we very much agree on the end-goal, and I'd really like to get
> > this addressed somehow. There's a couple of things I'm not really
> > sold on with your proposal though:
> >
> > - It creates a ad-hoc KMS API for some problem that looks fairly
> > generic. It's also redundant with the notifier mechanism without
> > using it (probably for the best though).
> >
> > - MIPI-DSI panel probe sequence is already fairly complex and fragile
> > (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges).
> > I'd rather avoid creating a new dependency in that graph.
> >
> > - And yeah, to some extent it's inconsistent with how we dealt with
> > secondary devices in KMS so far.
>
> Hmmmm. To a large extent, my current implementation actually has no
> impact on the DRM probe sequence. The panel itself never looks for the
> touchscreen code and everything DRM-related can register without a
> care in the world. From reading your bullet points, I guess that's
> both a strength and a weakness of my current proposal. It's really
> outside the world of bridge chains and DRM components which makes it a
> special snowflake that people need to understand on its own. ...but,
> at the same time, the fact that it is outside all the rest of that
> stuff means it doesn't add complexity to an already complex system.
>
> I guess I'd point to the panel backlight as a preexisting design
> that's not totally unlike what I'm doing. The backlight is not part of
> the DRM bridge chain and doesn't fit in like other components. This
> actually makes sense since the backlight doesn't take in or put out
> video data and it's simply something associated with the panel. The
> backlight also has a loose connection to the panel driver and a given
> panel could be associated with any number of different backlight
> drivers depending on the board design. I guess one difference between
> the backlight and what I'm doing with "panel follower" is that we
> typically don't let the panel probe until after the backlight has
> probed. In the case of my "panel follower" proposal it's the opposite.
> As per above, from a DRM probe point of view this actually makes my
> proposal less intrusive. I guess also a difference between backlight
> and "panel follower" is that I allow an arbitrary number of followers
> but there's only one backlight.
>
> One additional note: if I actually make the panel probe function start
> registering the touchscreen, that actually _does_ add more complexity
> to the already complex DRM probe ordering. It's yet another thing that
> could fail and/or defer...
>
> Also, I'm curious: would my proposal be more or less palatable if I
> made it less generic? Instead of "panel follower", I could hardcode it
> to "touchscreen" and then remove all the list management. From a DRM
> point of view this would make it even more like the preexisting
> "backlight" except for the ordering difference.
I'm trying to figure out what the next steps are here. I can send a v3
and address Benjamin's comments on the i2c-hid side, but I'd like to
get some resolution on our conversation here, too. Did my thoughts
above make sense? I know that "panel follower" isn't a
beautiful/elegant framework, but IMO it isn't not too bad. It
accomplishes the goal and mostly stays out of the way.
If you don't have time to dig into all of this now, would you object
if I can find someone else willing to review my series from a drm
perspective?
Thanks!
-Doug
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
2023-06-13 15:56 ` Doug Anderson
2023-06-21 16:31 ` Doug Anderson
@ 2023-06-23 9:08 ` Maxime Ripard
1 sibling, 0 replies; 32+ messages in thread
From: Maxime Ripard @ 2023-06-23 9:08 UTC (permalink / raw)
To: Doug 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, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
[-- Attachment #1: Type: text/plain, Size: 9423 bytes --]
On Tue, Jun 13, 2023 at 08:56:39AM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > > > What I'm trying to say is: could we just make it work by passing a bunch
> > > > of platform_data, 2-3 callbacks and a device registration from the panel
> > > > driver directly?
> > >
> > > I think I'm still confused about what you're proposing. Sorry! :( Let
> > > me try rephrasing why I'm confused and perhaps we can get on the same
> > > page. :-)
> > >
> > > First, I guess I'm confused about how you have one of these devices
> > > "register" the other device.
> > >
> > > I can understand how one device might "register" its sub-devices in
> > > the MFD case. To make it concrete, we can look at a PMIC like
> > > max77686.c. The parent MFD device gets probed and then it's in charge
> > > of creating all of its sub-devices. These sub-devices are intimately
> > > tied to one another. They have shared data structures and can
> > > coordinate power sequencing and whatnot. All good.
> >
> > We don't necessarily need to use MFD, but yeah, we could just register a
> > device for the i2c-hid driver to probe from (using
> > i2c_new_client_device?)
>
> I think this can work for devices where the panel and touchscreen are
> truly integrated where the panel driver knows enough about the related
> touchscreen to fully describe and instantiate it. It doesn't work
> quite as well for cases where the power and reset lines are shared
> just because of what a given board designer did. To handle that, each
> panel driver would need to get enough DT properties added to it so
> that it could fully describe any arbitrary touchscreen, right?
>
> Let's think about the generic panel-edp driver. This driver runs the
> panel on many sc7180-trogdor laptops, including coachz, lazor, and
> pompom. All three of those boards have a shared power rail for the
> touchscreen and panel. If you look at "sc7180-trogdor-coachz.dtsi",
> you can see the touchscreen currently looks like this:
>
> ap_ts: touchscreen@5d {
> compatible = "goodix,gt7375p";
> reg = <0x5d>;
> pinctrl-names = "default";
> pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
>
> interrupt-parent = <&tlmm>;
> interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
>
> reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
>
> vdd-supply = <&pp3300_ts>;
> };
>
> In "sc7180-trogdor-lazor.dtsi" we have:
>
> ap_ts: touchscreen@10 {
> compatible = "hid-over-i2c";
> reg = <0x10>;
> pinctrl-names = "default";
> pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
>
> interrupt-parent = <&tlmm>;
> interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
>
> post-power-on-delay-ms = <20>;
> hid-descr-addr = <0x0001>;
>
> vdd-supply = <&pp3300_ts>;
> };
>
> In both cases "pp3300_ts" is simply another name for "pp3300_dx_edp"
>
> So I think to do what you propose, we need to add this information to
> the panel-edp DT node so that it could dynamically construct the i2c
> device for the touchscreen:
>
> a) Which touchscreen is actually connected (generic hid-over-i2c,
> goodix, ...). I guess this would be a "compatible" string?
>
> b) Which i2c bus that device is hooked up to.
>
> c) Which i2c address that device is hooked up to.
>
> d) What the touchscreen interrupt GPIO is.
>
> e) Possibly what the "hid-descr-addr" for the touchscreen is.
>
> f) Any extra timing information needed to be passed to the touchscreen
> driver, like "post-power-on-delay-ms"
>
> The "pinctrl" stuff would be easy to subsume into the panel's DT node,
> at least. ...and, in this case, we could skip the "vdd-supply" since
> the panel and eDP are sharing power rails (which is what got us into
> this situation). ...but, the above is still a lot. At this point, it
> would make sense to have a sub-node under the panel to describe it,
> which we could do but it starts to feel weird. We'd essentially be
> describing an i2c device but not under the i2c controller it belongs
> to.
>
> I guess I'd also say that the above design also need additional code
> if/when someone had a touchscreen that used a different communication
> method, like SPI.
>
> So I guess the tl;dr of all the above is that I think it could all work if:
>
> 1. We described the touchscreen in a sub-node of the panel.
>
> 2. We added a property to the panel saying what the true parent of the
> touchscreen was (an I2C controller, a SPI controller, ...) and what
> type of controller it was ("SPI" vs "I2C").
>
> 3. We added some generic helpers that panels could call that would
> understand how to instantiate the touchscreen under the appropriate
> controller.
>
> 4. From there, we added a new private / generic API between panels and
> touchscreens letting them know that the panel was turning on/off.
>
> That seems much more complex to me, though. It also seems like an
> awkward way to describe it in DT.
Yeah, I guess you're right. I wish we had something simpler, but I can't
think of any better way.
Sorry for the distraction.
> > > In any case, is there any chance that we're in violent agreement
> >
> > Is it even violent? Sorry if it came across that way, it's really isn't
> > on my end.
>
> Sorry, maybe a poor choice of words on my end. I've heard that term
> thrown about when two people spend a lot of time discussing something
> / trying to persuade the other person only to find out in the end that
> they were both on the same side of the issue. ;-)
>
> > > and that if you dig into my design more you might like it? Other than
> > > the fact that the panel doesn't "register" the touchscreen device, it
> > > kinda sounds as if what my patches are already doing is roughly what
> > > you're describing. The touchscreen and panel driver are really just
> > > coordinating with each other through a shared data structure (struct
> > > drm_panel_follower) that has a few callback functions. Just like with
> > > "hdmi-codec", the devices probe separately but find each other through
> > > a phandle. The coordination between the two happens through a few
> > > simple helper functions.
> >
> > I guess we very much agree on the end-goal, and I'd really like to get
> > this addressed somehow. There's a couple of things I'm not really
> > sold on with your proposal though:
> >
> > - It creates a ad-hoc KMS API for some problem that looks fairly
> > generic. It's also redundant with the notifier mechanism without
> > using it (probably for the best though).
> >
> > - MIPI-DSI panel probe sequence is already fairly complex and fragile
> > (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges).
> > I'd rather avoid creating a new dependency in that graph.
> >
> > - And yeah, to some extent it's inconsistent with how we dealt with
> > secondary devices in KMS so far.
>
> Hmmmm. To a large extent, my current implementation actually has no
> impact on the DRM probe sequence. The panel itself never looks for the
> touchscreen code and everything DRM-related can register without a
> care in the world. From reading your bullet points, I guess that's
> both a strength and a weakness of my current proposal. It's really
> outside the world of bridge chains and DRM components which makes it a
> special snowflake that people need to understand on its own. ...but,
> at the same time, the fact that it is outside all the rest of that
> stuff means it doesn't add complexity to an already complex system.
>
> I guess I'd point to the panel backlight as a preexisting design
> that's not totally unlike what I'm doing. The backlight is not part of
> the DRM bridge chain and doesn't fit in like other components. This
> actually makes sense since the backlight doesn't take in or put out
> video data and it's simply something associated with the panel. The
> backlight also has a loose connection to the panel driver and a given
> panel could be associated with any number of different backlight
> drivers depending on the board design. I guess one difference between
> the backlight and what I'm doing with "panel follower" is that we
> typically don't let the panel probe until after the backlight has
> probed. In the case of my "panel follower" proposal it's the opposite.
> As per above, from a DRM probe point of view this actually makes my
> proposal less intrusive. I guess also a difference between backlight
> and "panel follower" is that I allow an arbitrary number of followers
> but there's only one backlight.
>
> One additional note: if I actually make the panel probe function start
> registering the touchscreen, that actually _does_ add more complexity
> to the already complex DRM probe ordering. It's yet another thing that
> could fail and/or defer...
>
> Also, I'm curious: would my proposal be more or less palatable if I
> made it less generic? Instead of "panel follower", I could hardcode it
> to "touchscreen" and then remove all the list management. From a DRM
> point of view this would make it even more like the preexisting
> "backlight" except for the ordering difference.
No, that's fine. I guess I don't have any objections to your work, so
feel free to send a v2 :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-08 15:36 ` Benjamin Tissoires
2023-06-08 16:42 ` Doug Anderson
@ 2023-06-26 22:49 ` Doug Anderson
2023-07-17 18:15 ` Doug Anderson
1 sibling, 1 reply; 32+ messages in thread
From: Doug Anderson @ 2023-06-26 22:49 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
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?
Thanks!
-Doug
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-06-26 22:49 ` Doug Anderson
@ 2023-07-17 18:15 ` Doug Anderson
2023-07-25 20:41 ` Doug Anderson
0 siblings, 1 reply; 32+ messages in thread
From: Doug Anderson @ 2023-07-17 18:15 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
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.
Thanks for your time!
-Doug
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-07-17 18:15 ` Doug Anderson
@ 2023-07-25 20:41 ` Doug Anderson
2023-07-26 8:07 ` Benjamin Tissoires
0 siblings, 1 reply; 32+ messages in thread
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
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 [flat|nested] 32+ messages in thread
* Re: [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower
2023-07-25 20:41 ` Doug Anderson
@ 2023-07-26 8:07 ` Benjamin Tissoires
0 siblings, 0 replies; 32+ messages in thread
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
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 [flat|nested] 32+ messages in thread
end of thread, other threads:[~2023-07-26 8:18 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens Douglas Anderson
2023-06-09 15:53 ` Krzysztof Kozlowski
2023-06-07 21:49 ` [PATCH v2 02/10] drm/panel: Check for already prepared/enabled in drm_panel Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 03/10] drm/panel: Add a way for other devices to follow panel state Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers Douglas Anderson
2023-06-09 16:10 ` Rob Herring
2023-06-07 21:49 ` [PATCH v2 05/10] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 06/10] HID: i2c-hid: Rearrange probe() to power things up later Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 07/10] HID: i2c-hid: Make suspend and resume into helper functions Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower Douglas Anderson
2023-06-08 5:18 ` kernel test robot
2023-06-08 7:14 ` kernel test robot
2023-06-08 15:10 ` Doug Anderson
2023-06-08 15:36 ` Benjamin Tissoires
2023-06-08 16:42 ` Doug Anderson
2023-06-09 9:27 ` Benjamin Tissoires
2023-06-09 15:01 ` Doug Anderson
2023-06-26 22:49 ` Doug Anderson
2023-07-17 18:15 ` Doug Anderson
2023-07-25 20:41 ` Doug Anderson
2023-07-26 8:07 ` Benjamin Tissoires
2023-06-07 21:49 ` [PATCH v2 09/10] HID: i2c-hid: Do panel follower work on the system_wq Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 10/10] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels Douglas Anderson
2023-06-08 7:17 ` [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Maxime Ripard
2023-06-08 14:38 ` Doug Anderson
2023-06-12 16:03 ` Maxime Ripard
2023-06-12 21:13 ` Doug Anderson
2023-06-13 12:06 ` Maxime Ripard
2023-06-13 15:56 ` Doug Anderson
2023-06-21 16:31 ` Doug Anderson
2023-06-23 9:08 ` Maxime Ripard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).