* [PATCH v4 06/11] HID: i2c-hid: Rearrange probe() to power things up later
From: Douglas Anderson @ 2023-07-27 17:16 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: linux-arm-msm, yangcong5, devicetree, Daniel Vetter, hsinyi,
Chris Morgan, linux-input, cros-qcom-dts-watchers,
Dmitry Torokhov, linux-kernel, dri-devel, Douglas Anderson
In-Reply-To: <20230727171750.633410-1-dianders@chromium.org>
In a future patch, we want to change i2c-hid not to necessarily power
up the touchscreen during probe. In preparation for that, rearrange
the probe function so that we put as much stuff _before_ powering up
the device as possible.
This change is expected to have no functional effect.
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
(no changes since v2)
Changes in v2:
- i2c_hid_core_initial_power_up() is now static.
drivers/hid/i2c-hid/i2c-hid-core.c | 124 ++++++++++++++++++-----------
1 file changed, 77 insertions(+), 47 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 19d985c20a5c..d29e6421ecba 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -855,7 +855,8 @@ static int i2c_hid_init_irq(struct i2c_client *client)
irqflags = IRQF_TRIGGER_LOW;
ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
- irqflags | IRQF_ONESHOT, client->name, ihid);
+ irqflags | IRQF_ONESHOT | IRQF_NO_AUTOEN,
+ client->name, ihid);
if (ret < 0) {
dev_warn(&client->dev,
"Could not register for %s interrupt, irq = %d,"
@@ -940,6 +941,72 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid)
ihid->ops->shutdown_tail(ihid->ops);
}
+/**
+ * i2c_hid_core_initial_power_up() - First time power up of the i2c-hid device.
+ * @ihid: The ihid object created during probe.
+ *
+ * This function is called at probe time.
+ *
+ * The initial power on is where we do some basic validation that the device
+ * exists, where we fetch the HID descriptor, and where we create the actual
+ * HID devices.
+ *
+ * Return: 0 or error code.
+ */
+static int i2c_hid_core_initial_power_up(struct i2c_hid *ihid)
+{
+ struct i2c_client *client = ihid->client;
+ struct hid_device *hid = ihid->hid;
+ int ret;
+
+ ret = i2c_hid_core_power_up(ihid);
+ if (ret)
+ return ret;
+
+ /* Make sure there is something at this address */
+ ret = i2c_smbus_read_byte(client);
+ if (ret < 0) {
+ i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
+ ret = -ENXIO;
+ goto err;
+ }
+
+ ret = i2c_hid_fetch_hid_descriptor(ihid);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "Failed to fetch the HID Descriptor\n");
+ goto err;
+ }
+
+ enable_irq(client->irq);
+
+ hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
+ hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
+ hid->product = le16_to_cpu(ihid->hdesc.wProductID);
+
+ hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor,
+ hid->product);
+
+ snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
+ client->name, (u16)hid->vendor, (u16)hid->product);
+ strscpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
+
+ ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
+
+ ret = hid_add_device(hid);
+ if (ret) {
+ if (ret != -ENODEV)
+ hid_err(client, "can't add hid device: %d\n", ret);
+ goto err;
+ }
+
+ return 0;
+
+err:
+ i2c_hid_core_power_down(ihid);
+ return ret;
+}
+
int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
u16 hid_descriptor_address, u32 quirks)
{
@@ -966,16 +1033,10 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
if (!ihid)
return -ENOMEM;
- ihid->ops = ops;
-
- ret = i2c_hid_core_power_up(ihid);
- if (ret)
- return ret;
-
i2c_set_clientdata(client, ihid);
+ ihid->ops = ops;
ihid->client = client;
-
ihid->wHIDDescRegister = cpu_to_le16(hid_descriptor_address);
init_waitqueue_head(&ihid->wait);
@@ -986,28 +1047,12 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
* real computation later. */
ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
if (ret < 0)
- goto err_powered;
-
+ return ret;
device_enable_async_suspend(&client->dev);
- /* Make sure there is something at this address */
- ret = i2c_smbus_read_byte(client);
- if (ret < 0) {
- i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
- ret = -ENXIO;
- goto err_powered;
- }
-
- ret = i2c_hid_fetch_hid_descriptor(ihid);
- if (ret < 0) {
- dev_err(&client->dev,
- "Failed to fetch the HID Descriptor\n");
- goto err_powered;
- }
-
ret = i2c_hid_init_irq(client);
if (ret < 0)
- goto err_powered;
+ goto err_buffers_allocated;
hid = hid_allocate_device();
if (IS_ERR(hid)) {
@@ -1021,26 +1066,11 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
hid->ll_driver = &i2c_hid_ll_driver;
hid->dev.parent = &client->dev;
hid->bus = BUS_I2C;
- hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
- hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
- hid->product = le16_to_cpu(ihid->hdesc.wProductID);
-
hid->initial_quirks = quirks;
- hid->initial_quirks |= i2c_hid_get_dmi_quirks(hid->vendor,
- hid->product);
-
- snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
- client->name, (u16)hid->vendor, (u16)hid->product);
- strscpy(hid->phys, dev_name(&client->dev), sizeof(hid->phys));
-
- ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
- ret = hid_add_device(hid);
- if (ret) {
- if (ret != -ENODEV)
- hid_err(client, "can't add hid device: %d\n", ret);
+ ret = i2c_hid_core_initial_power_up(ihid);
+ if (ret)
goto err_mem_free;
- }
return 0;
@@ -1050,9 +1080,9 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
err_irq:
free_irq(client->irq, ihid);
-err_powered:
- i2c_hid_core_power_down(ihid);
+err_buffers_allocated:
i2c_hid_free_buffers(ihid);
+
return ret;
}
EXPORT_SYMBOL_GPL(i2c_hid_core_probe);
@@ -1062,6 +1092,8 @@ void i2c_hid_core_remove(struct i2c_client *client)
struct i2c_hid *ihid = i2c_get_clientdata(client);
struct hid_device *hid;
+ i2c_hid_core_power_down(ihid);
+
hid = ihid->hid;
hid_destroy_device(hid);
@@ -1069,8 +1101,6 @@ void i2c_hid_core_remove(struct i2c_client *client)
if (ihid->bufsize)
i2c_hid_free_buffers(ihid);
-
- i2c_hid_core_power_down(ihid);
}
EXPORT_SYMBOL_GPL(i2c_hid_core_remove);
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v4 05/11] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS()
From: Douglas Anderson @ 2023-07-27 17:16 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: linux-arm-msm, yangcong5, devicetree, Daniel Vetter, hsinyi,
Chris Morgan, linux-input, cros-qcom-dts-watchers,
Dmitry Torokhov, linux-kernel, dri-devel, Douglas Anderson
In-Reply-To: <20230727171750.633410-1-dianders@chromium.org>
The SYSTEM_SLEEP_PM_OPS() allows us to get rid of '#ifdef
CONFIG_PM_SLEEP', as talked about in commit 1a3c7bb08826 ("PM: core:
Add new *_PM_OPS macros, deprecate old ones").
This change is expected to have no functional effect.
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
(no changes since v1)
drivers/hid/i2c-hid/i2c-hid-core.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index efbba0465eef..19d985c20a5c 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1085,7 +1085,6 @@ void i2c_hid_core_shutdown(struct i2c_client *client)
}
EXPORT_SYMBOL_GPL(i2c_hid_core_shutdown);
-#ifdef CONFIG_PM_SLEEP
static int i2c_hid_core_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -1138,10 +1137,9 @@ static int i2c_hid_core_resume(struct device *dev)
return hid_driver_reset_resume(hid);
}
-#endif
const struct dev_pm_ops i2c_hid_core_pm = {
- SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
+ SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
};
EXPORT_SYMBOL_GPL(i2c_hid_core_pm);
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v4 04/11] of: property: fw_devlink: Add a devlink for panel followers
From: Douglas Anderson @ 2023-07-27 17:16 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: linux-arm-msm, yangcong5, devicetree, Daniel Vetter, hsinyi,
Chris Morgan, linux-input, cros-qcom-dts-watchers,
Dmitry Torokhov, linux-kernel, dri-devel, Douglas Anderson,
Rob Herring
In-Reply-To: <20230727171750.633410-1-dianders@chromium.org>
Inform fw_devlink of the fact that a panel follower (like a
touchscreen) is effectively a consumer of the panel from the purposes
of fw_devlink.
NOTE: this patch isn't required for correctness but instead optimizes
probe order / helps avoid deferrals.
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Since this is so small, I'd presume it's OK for it to go through a DRM
tree with the proper Ack. That being said, this patch is just an
optimization and thus it could land completely separately from the
rest and they could all meet up in mainline.
(no changes since v2)
Changes in v2:
- ("Add a devlink for panel followers") new for v2.
drivers/of/property.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index ddc75cd50825..cf8dacf3e3b8 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1266,6 +1266,7 @@ DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
DEFINE_SIMPLE_PROP(leds, "leds", NULL)
DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
+DEFINE_SIMPLE_PROP(panel, "panel", NULL)
DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
@@ -1354,6 +1355,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
{ .parse_prop = parse_resets, },
{ .parse_prop = parse_leds, },
{ .parse_prop = parse_backlight, },
+ { .parse_prop = parse_panel, },
{ .parse_prop = parse_gpio_compat, },
{ .parse_prop = parse_interrupts, },
{ .parse_prop = parse_regulators, },
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v4 03/11] drm/panel: Add a way for other devices to follow panel state
From: Douglas Anderson @ 2023-07-27 17:16 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: linux-arm-msm, yangcong5, devicetree, Daniel Vetter, hsinyi,
Chris Morgan, linux-input, cros-qcom-dts-watchers,
Dmitry Torokhov, linux-kernel, dri-devel, Douglas Anderson
In-Reply-To: <20230727171750.633410-1-dianders@chromium.org>
These days, it's fairly common to see panels that have touchscreens
attached to them. The panel and the touchscreen can somewhat be
thought of as totally separate devices and, historically, this is how
Linux has treated them. However, treating them as separate isn't
necessarily the best way to model the two devices, it was just that
there was no better way. Specifically, there is little practical
reason to have the touchscreen powered on when the panel is turned
off, but if we model the devices separately we have no way to keep the
two devices' power states in sync with each other.
The issue described above makes it sound as if the problem here is
just about efficiency. We're wasting power keeping the touchscreen
powered up when the screen is off. While that's true, the problem can
go deeper. Specifically, hardware designers see that there's no reason
to have the touchscreen on while the screen is off and then build
hardware assuming that software would never turn the touchscreen on
while the screen is off.
In the very simplest case of hardware designs like this, the
touchscreen and the panel share some power rails. In most cases, this
turns out not to be terrible and is, again, just a little less
efficient. Specifically if we tell Linux that the touchscreen and the
panel are using the same rails then Linux will keep the rails on when
_either_ device is turned on. That ends to work OK-ish, but now if you
turn the panel off not only will the touchscreen remain powered, but
the power rails for the panel itself won't be switched off, burning
extra power.
The above two inefficiencies are _extra_ minor when you consider the
fact that laptops rarely spend much time with the screen off. The main
use case would be when an external screen (and presumably a power
supply) is attached.
Unfortunately, it gets worse from here. On sc7180-trogdor-homestar,
for instance, the display's TCON (timing controller) sometimes crashes
if you don't power cycle it whenever you stop and restart the video
stream (like during a modeset). The touchscreen keeping the power
rails on causes real problems. One proposal in the homestar timeframe
was to move the touchscreen to an always-on rail, dedicating the main
power rail to the panel. That caused _different_ problems as talked
about in commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the
reset line to the regulator"). The end result of all of this was to
add an extra regulator to the board, increasing cost.
Recently, Cong Yang posted a patch [1] where things are even worse.
The panel and touch controller on that system seem even more
intimately tied together and really can't be thought of separately.
To address this issue, let's start allowing devices to register
themselves as "panel followers". These devices will get called after a
panel has been powered on and before a panel is powered off. This
makes the panel the primary device in charge of the power state, which
matches how userspace uses it.
The panel follower API should be fairly straightforward to use. The
current code assumes that panel followers are using device tree and
have a "panel" property pointing to the panel to follow. More
flexibility and non-DT implementations could be added as needed.
Right now, panel followers can follow the prepare/unprepare functions.
There could be arguments made that, instead, they should follow
enable/disable. I've chosen prepare/unprepare for now since those
functions are guaranteed to power up/power down the panel and it seems
better to start the process earlier.
A bit of explaining about why this is a roll-your-own API instead of
using something more standard:
1. In standard APIs in Linux, parent devices are automatically powered
on when a child needs power. Applying that here, it would mean that
we'd force the panel on any time someone was listening to the
touchscreen. That, unfortunately, would have broken homestar's need
(if we hadn't changed the hardware, as per above) where the panel
absolutely needs to be able to power cycle itself. While one could
argue that homestar is broken hardware and we shouldn't have the
API do backflips for it, _officially_ the eDP timing guidelines
agree with homestar's needs and the panel power sequencing diagrams
show power going off. It's nice to be able to support this.
2. We could, conceibably, try to add a new flag to device_link causing
the parent to be in charge of power. Then we could at least use
normal pm_runtime APIs. This sounds great, except that we run into
problems with initial probe. As talked about in the later patch
("HID: i2c-hid: Support being a panel follower") the initial power
on of a panel follower might need to do things (like add
sub-devices) that aren't allowed in a runtime_resume function.
The above complexities explain why this API isn't using common
functions. That being said, this patch is very small and
self-contained, so if someone was later able to adapt it to using more
common APIs while solving the above issues then that could happen in
the future.
[1] https://lore.kernel.org/r/20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
(no changes since v3)
Changes in v3:
- Add is_panel_follower() as a convenience for clients.
Changes in v2:
- Add even more text to the commit message.
- A few comment cleanups.
drivers/gpu/drm/drm_panel.c | 173 +++++++++++++++++++++++++++++++++++-
include/drm/drm_panel.h | 80 +++++++++++++++++
2 files changed, 249 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 4e1c4e42575b..e814020bbcd3 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -58,6 +58,8 @@ void drm_panel_init(struct drm_panel *panel, struct device *dev,
const struct drm_panel_funcs *funcs, int connector_type)
{
INIT_LIST_HEAD(&panel->list);
+ INIT_LIST_HEAD(&panel->followers);
+ mutex_init(&panel->follower_lock);
panel->dev = dev;
panel->funcs = funcs;
panel->connector_type = connector_type;
@@ -105,6 +107,7 @@ EXPORT_SYMBOL(drm_panel_remove);
*/
int drm_panel_prepare(struct drm_panel *panel)
{
+ struct drm_panel_follower *follower;
int ret;
if (!panel)
@@ -115,14 +118,27 @@ int drm_panel_prepare(struct drm_panel *panel)
return 0;
}
+ mutex_lock(&panel->follower_lock);
+
if (panel->funcs && panel->funcs->prepare) {
ret = panel->funcs->prepare(panel);
if (ret < 0)
- return ret;
+ goto exit;
}
panel->prepared = true;
- return 0;
+ list_for_each_entry(follower, &panel->followers, list) {
+ ret = follower->funcs->panel_prepared(follower);
+ if (ret < 0)
+ dev_info(panel->dev, "%ps failed: %d\n",
+ follower->funcs->panel_prepared, ret);
+ }
+
+ ret = 0;
+exit:
+ mutex_unlock(&panel->follower_lock);
+
+ return ret;
}
EXPORT_SYMBOL(drm_panel_prepare);
@@ -139,6 +155,7 @@ EXPORT_SYMBOL(drm_panel_prepare);
*/
int drm_panel_unprepare(struct drm_panel *panel)
{
+ struct drm_panel_follower *follower;
int ret;
if (!panel)
@@ -149,14 +166,27 @@ int drm_panel_unprepare(struct drm_panel *panel)
return 0;
}
+ mutex_lock(&panel->follower_lock);
+
+ list_for_each_entry(follower, &panel->followers, list) {
+ ret = follower->funcs->panel_unpreparing(follower);
+ if (ret < 0)
+ dev_info(panel->dev, "%ps failed: %d\n",
+ follower->funcs->panel_unpreparing, ret);
+ }
+
if (panel->funcs && panel->funcs->unprepare) {
ret = panel->funcs->unprepare(panel);
if (ret < 0)
- return ret;
+ goto exit;
}
panel->prepared = false;
- return 0;
+ ret = 0;
+exit:
+ mutex_unlock(&panel->follower_lock);
+
+ return ret;
}
EXPORT_SYMBOL(drm_panel_unprepare);
@@ -342,6 +372,141 @@ int of_drm_get_panel_orientation(const struct device_node *np,
EXPORT_SYMBOL(of_drm_get_panel_orientation);
#endif
+/**
+ * drm_is_panel_follower() - Check if the device is a panel follower
+ * @dev: The 'struct device' to check
+ *
+ * This checks to see if a device needs to be power sequenced together with
+ * a panel using the panel follower API.
+ * At the moment panels can only be followed on device tree enabled systems.
+ * The "panel" property of the follower points to the panel to be followed.
+ *
+ * Return: true if we should be power sequenced with a panel; false otherwise.
+ */
+bool drm_is_panel_follower(struct device *dev)
+{
+ /*
+ * The "panel" property is actually a phandle, but for simplicity we
+ * don't bother trying to parse it here. We just need to know if the
+ * property is there.
+ */
+ return of_property_read_bool(dev->of_node, "panel");
+}
+EXPORT_SYMBOL(drm_is_panel_follower);
+
+/**
+ * drm_panel_add_follower() - Register something to follow panel state.
+ * @follower_dev: The 'struct device' for the follower.
+ * @follower: The panel follower descriptor for the follower.
+ *
+ * A panel follower is called right after preparing the panel and right before
+ * unpreparing the panel. It's primary intention is to power on an associated
+ * touchscreen, though it could be used for any similar devices. Multiple
+ * devices are allowed the follow the same panel.
+ *
+ * If a follower is added to a panel that's already been turned on, the
+ * follower's prepare callback is called right away.
+ *
+ * At the moment panels can only be followed on device tree enabled systems.
+ * The "panel" property of the follower points to the panel to be followed.
+ *
+ * Return: 0 or an error code. Note that -ENODEV means that we detected that
+ * follower_dev is not actually following a panel. The caller may
+ * choose to ignore this return value if following a panel is optional.
+ */
+int drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ struct device_node *panel_np;
+ struct drm_panel *panel;
+ int ret;
+
+ panel_np = of_parse_phandle(follower_dev->of_node, "panel", 0);
+ if (!panel_np)
+ return -ENODEV;
+
+ panel = of_drm_find_panel(panel_np);
+ of_node_put(panel_np);
+ if (IS_ERR(panel))
+ return PTR_ERR(panel);
+
+ get_device(panel->dev);
+ follower->panel = panel;
+
+ mutex_lock(&panel->follower_lock);
+
+ list_add_tail(&follower->list, &panel->followers);
+ if (panel->prepared) {
+ ret = follower->funcs->panel_prepared(follower);
+ if (ret < 0)
+ dev_info(panel->dev, "%ps failed: %d\n",
+ follower->funcs->panel_prepared, ret);
+ }
+
+ mutex_unlock(&panel->follower_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_panel_add_follower);
+
+/**
+ * drm_panel_remove_follower() - Reverse drm_panel_add_follower().
+ * @follower: The panel follower descriptor for the follower.
+ *
+ * Undo drm_panel_add_follower(). This includes calling the follower's
+ * unprepare function if we're removed from a panel that's currently prepared.
+ *
+ * Return: 0 or an error code.
+ */
+void drm_panel_remove_follower(struct drm_panel_follower *follower)
+{
+ struct drm_panel *panel = follower->panel;
+ int ret;
+
+ mutex_lock(&panel->follower_lock);
+
+ if (panel->prepared) {
+ ret = follower->funcs->panel_unpreparing(follower);
+ if (ret < 0)
+ dev_info(panel->dev, "%ps failed: %d\n",
+ follower->funcs->panel_unpreparing, ret);
+ }
+ list_del_init(&follower->list);
+
+ mutex_unlock(&panel->follower_lock);
+
+ put_device(panel->dev);
+}
+EXPORT_SYMBOL(drm_panel_remove_follower);
+
+static void drm_panel_remove_follower_void(void *follower)
+{
+ drm_panel_remove_follower(follower);
+}
+
+/**
+ * devm_drm_panel_add_follower() - devm version of drm_panel_add_follower()
+ * @follower_dev: The 'struct device' for the follower.
+ * @follower: The panel follower descriptor for the follower.
+ *
+ * Handles calling drm_panel_remove_follower() using devm on the follower_dev.
+ *
+ * Return: 0 or an error code.
+ */
+int devm_drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ int ret;
+
+ ret = drm_panel_add_follower(follower_dev, follower);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(follower_dev,
+ drm_panel_remove_follower_void, follower);
+}
+EXPORT_SYMBOL(devm_drm_panel_add_follower);
+
#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
/**
* drm_panel_of_backlight - use backlight device node for backlight
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index c6cf75909389..5ac67eeb0860 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -27,12 +27,14 @@
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/list.h>
+#include <linux/mutex.h>
struct backlight_device;
struct dentry;
struct device_node;
struct drm_connector;
struct drm_device;
+struct drm_panel_follower;
struct drm_panel;
struct display_timing;
@@ -144,6 +146,45 @@ struct drm_panel_funcs {
void (*debugfs_init)(struct drm_panel *panel, struct dentry *root);
};
+struct drm_panel_follower_funcs {
+ /**
+ * @panel_prepared:
+ *
+ * Called after the panel has been powered on.
+ */
+ int (*panel_prepared)(struct drm_panel_follower *follower);
+
+ /**
+ * @panel_unpreparing:
+ *
+ * Called before the panel is powered off.
+ */
+ int (*panel_unpreparing)(struct drm_panel_follower *follower);
+};
+
+struct drm_panel_follower {
+ /**
+ * @funcs:
+ *
+ * Dependent device callbacks; should be initted by the caller.
+ */
+ const struct drm_panel_follower_funcs *funcs;
+
+ /**
+ * @list
+ *
+ * Used for linking into panel's list; set by drm_panel_add_follower().
+ */
+ struct list_head list;
+
+ /**
+ * @panel
+ *
+ * The panel we're dependent on; set by drm_panel_add_follower().
+ */
+ struct drm_panel *panel;
+};
+
/**
* struct drm_panel - DRM panel object
*/
@@ -189,6 +230,20 @@ struct drm_panel {
*/
struct list_head list;
+ /**
+ * @followers:
+ *
+ * A list of struct drm_panel_follower dependent on this panel.
+ */
+ struct list_head followers;
+
+ /**
+ * @followers_lock:
+ *
+ * Lock for followers list.
+ */
+ struct mutex follower_lock;
+
/**
* @prepare_prev_first:
*
@@ -246,6 +301,31 @@ static inline int of_drm_get_panel_orientation(const struct device_node *np,
}
#endif
+#if defined(CONFIG_DRM_PANEL)
+bool drm_is_panel_follower(struct device *dev);
+int drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower);
+void drm_panel_remove_follower(struct drm_panel_follower *follower);
+int devm_drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower);
+#else
+static inline bool drm_is_panel_follower(struct device *dev)
+{
+ return false;
+}
+static inline int drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ return -ENODEV;
+}
+static inline void drm_panel_remove_follower(struct drm_panel_follower *follower) { }
+static inline int devm_drm_panel_add_follower(struct device *follower_dev,
+ struct drm_panel_follower *follower)
+{
+ return -ENODEV;
+}
+#endif
+
#if IS_ENABLED(CONFIG_DRM_PANEL) && (IS_BUILTIN(CONFIG_BACKLIGHT_CLASS_DEVICE) || \
(IS_MODULE(CONFIG_DRM) && IS_MODULE(CONFIG_BACKLIGHT_CLASS_DEVICE)))
int drm_panel_of_backlight(struct drm_panel *panel);
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v4 02/11] drm/panel: Check for already prepared/enabled in drm_panel
From: Douglas Anderson @ 2023-07-27 17:16 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: linux-arm-msm, yangcong5, devicetree, Daniel Vetter, hsinyi,
Chris Morgan, linux-input, cros-qcom-dts-watchers,
Dmitry Torokhov, linux-kernel, dri-devel, Douglas Anderson
In-Reply-To: <20230727171750.633410-1-dianders@chromium.org>
In a whole pile of panel drivers, we have code to make the
prepare/unprepare/enable/disable callbacks behave as no-ops if they've
already been called. It's silly to have this code duplicated
everywhere. Add it to the core instead so that we can eventually
delete it from all the drivers. Note: to get some idea of the
duplicated code, try:
git grep 'if.*>prepared' -- drivers/gpu/drm/panel
git grep 'if.*>enabled' -- drivers/gpu/drm/panel
NOTE: arguably, the right thing to do here is actually to skip this
patch and simply remove all the extra checks from the individual
drivers. Perhaps the checks were needed at some point in time in the
past but maybe they no longer are? Certainly as we continue
transitioning over to "panel_bridge" then we expect there to be much
less variety in how these calls are made. When we're called as part of
the bridge chain, things should be pretty simple. In fact, there was
some discussion in the past about these checks [1], including a
discussion about whether the checks were needed and whether the calls
ought to be refcounted. At the time, I decided not to mess with it
because it felt too risky.
Looking closer at it now, I'm fairly certain that nothing in the
existing codebase is expecting these calls to be refcounted. The only
real question is whether someone is already doing something to ensure
prepare()/unprepare() match and enabled()/disable() match. I would say
that, even if there is something else ensuring that things match,
there's enough complexity that adding an extra bool and an extra
double-check here is a good idea. Let's add a drm_warn() to let people
know that it's considered a minor error to take advantage of
drm_panel's double-checking but we'll still make things work fine.
We'll also add an entry to the official DRM todo list to remove the
now pointless check from the panels after this patch lands and,
eventually, fixup anyone who is triggering the new warning.
[1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid
Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This has Neil's Ack and Maxime's review and I could commit it to
drm-misc-next, but for now I'm holding off to land with the rest of
this series since the second drm patch in my series depends on this
one.
Once this lands somewhere, I'll take the action item to post patches
to delete the now pointless checks in the individual panels.
Changes in v4:
- Document further cleanup in the official DRM todo list.
Documentation/gpu/todo.rst | 24 ++++++++++++++++++
drivers/gpu/drm/drm_panel.c | 49 ++++++++++++++++++++++++++++++++-----
include/drm/drm_panel.h | 14 +++++++++++
3 files changed, 81 insertions(+), 6 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 68bdafa0284f..e3b272c97758 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -452,6 +452,30 @@ Contact: Thomas Zimmermann <tzimmermann@suse.de>
Level: Starter
+Clean up checks for already prepared/enabled in panels
+------------------------------------------------------
+
+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. 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
+
+In the patch ("drm/panel: Check for already prepared/enabled in drm_panel")
+we've moved this check to the core. Now we can most definitely remove the
+check from the individual panels and save a pile of code.
+
+In adition to removing the check from the individual panels, it is believed
+that even the core shouldn't need this check and that should be considered
+an error if other code ever relies on this check. The check in the core
+currently prints a warning whenever something is relying on this check with
+dev_warn(). After a little while, we likely want to promote this to a
+WARN(1) to help encourage folks not to rely on this behavior.
+
+Contact: Douglas Anderson <dianders@chromium.org>
+
+Level: Starter/Intermediate
+
Core refactorings
=================
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index f634371c717a..4e1c4e42575b 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -105,11 +105,22 @@ EXPORT_SYMBOL(drm_panel_remove);
*/
int drm_panel_prepare(struct drm_panel *panel)
{
+ int ret;
+
if (!panel)
return -EINVAL;
- if (panel->funcs && panel->funcs->prepare)
- return panel->funcs->prepare(panel);
+ if (panel->prepared) {
+ dev_warn(panel->dev, "Skipping prepare of already prepared panel\n");
+ return 0;
+ }
+
+ if (panel->funcs && panel->funcs->prepare) {
+ ret = panel->funcs->prepare(panel);
+ if (ret < 0)
+ return ret;
+ }
+ panel->prepared = true;
return 0;
}
@@ -128,11 +139,22 @@ EXPORT_SYMBOL(drm_panel_prepare);
*/
int drm_panel_unprepare(struct drm_panel *panel)
{
+ int ret;
+
if (!panel)
return -EINVAL;
- if (panel->funcs && panel->funcs->unprepare)
- return panel->funcs->unprepare(panel);
+ if (!panel->prepared) {
+ dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
+ return 0;
+ }
+
+ if (panel->funcs && panel->funcs->unprepare) {
+ ret = panel->funcs->unprepare(panel);
+ if (ret < 0)
+ return ret;
+ }
+ panel->prepared = false;
return 0;
}
@@ -155,11 +177,17 @@ int drm_panel_enable(struct drm_panel *panel)
if (!panel)
return -EINVAL;
+ if (panel->enabled) {
+ dev_warn(panel->dev, "Skipping enable of already enabled panel\n");
+ return 0;
+ }
+
if (panel->funcs && panel->funcs->enable) {
ret = panel->funcs->enable(panel);
if (ret < 0)
return ret;
}
+ panel->enabled = true;
ret = backlight_enable(panel->backlight);
if (ret < 0)
@@ -187,13 +215,22 @@ int drm_panel_disable(struct drm_panel *panel)
if (!panel)
return -EINVAL;
+ if (!panel->enabled) {
+ dev_warn(panel->dev, "Skipping disable of already disabled panel\n");
+ return 0;
+ }
+
ret = backlight_disable(panel->backlight);
if (ret < 0)
DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n",
ret);
- if (panel->funcs && panel->funcs->disable)
- return panel->funcs->disable(panel);
+ if (panel->funcs && panel->funcs->disable) {
+ ret = panel->funcs->disable(panel);
+ if (ret < 0)
+ return ret;
+ }
+ panel->enabled = false;
return 0;
}
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 432fab2347eb..c6cf75909389 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -198,6 +198,20 @@ struct drm_panel {
* the panel is powered up.
*/
bool prepare_prev_first;
+
+ /**
+ * @prepared:
+ *
+ * If true then the panel has been prepared.
+ */
+ bool prepared;
+
+ /**
+ * @enabled:
+ *
+ * If true then the panel has been enabled.
+ */
+ bool enabled;
};
void drm_panel_init(struct drm_panel *panel, struct device *dev,
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v4 01/11] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens
From: Douglas Anderson @ 2023-07-27 17:16 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: linux-arm-msm, yangcong5, devicetree, Daniel Vetter, hsinyi,
Chris Morgan, linux-input, cros-qcom-dts-watchers,
Dmitry Torokhov, linux-kernel, dri-devel, Douglas Anderson,
Krzysztof Kozlowski
In-Reply-To: <20230727171750.633410-1-dianders@chromium.org>
As talked about in the patch ("drm/panel: Add a way for other devices
to follow panel state"), touchscreens that are connected to panels are
generally expected to be power sequenced together with the panel
they're attached to. Today, nothing provides information allowing you
to find out that a touchscreen is connected to a panel. Let's add a
phandle for this.
The proerty is added to the generic touchscreen bindings and then
enabled in the bindings for the i2c-hid backed devices. This can and
should be added for other touchscreens in the future, but for now
let's start small.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
(no changes since v2)
Changes in v2:
- Move the description to the generic touchscreen.yaml.
- Update the desc to make it clearer it's only for integrated devices.
Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 5 +++++
.../devicetree/bindings/input/goodix,gt7375p.yaml | 5 +++++
Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 2 ++
.../devicetree/bindings/input/touchscreen/touchscreen.yaml | 7 +++++++
4 files changed, 19 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index 05e6f2df604c..3e2d216c6432 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -13,6 +13,9 @@ description:
Supports the Elan eKTH6915 touchscreen controller.
This touchscreen controller uses the i2c-hid protocol with a reset GPIO.
+allOf:
+ - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+
properties:
compatible:
items:
@@ -24,6 +27,8 @@ properties:
interrupts:
maxItems: 1
+ panel: true
+
reset-gpios:
description: Reset GPIO; not all touchscreens using eKTH6915 hook this up.
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
index 1edad1da1196..358cb8275bf1 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -14,6 +14,9 @@ description:
This touchscreen uses the i2c-hid protocol but has some non-standard
power sequencing required.
+allOf:
+ - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+
properties:
compatible:
oneOf:
@@ -30,6 +33,8 @@ properties:
interrupts:
maxItems: 1
+ panel: true
+
reset-gpios:
true
diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
index 7156b08f7645..138caad96a29 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
@@ -44,6 +44,8 @@ properties:
description: HID descriptor address
$ref: /schemas/types.yaml#/definitions/uint32
+ panel: true
+
post-power-on-delay-ms:
description: Time required by the device after enabling its regulators
or powering it on, before it is ready for communication.
diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
index 895592da9626..431c13335c40 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchscreen.yaml
@@ -10,6 +10,13 @@ maintainers:
- Dmitry Torokhov <dmitry.torokhov@gmail.com>
properties:
+ panel:
+ description: If this touchscreen is integrally connected to a panel, this
+ is a reference to that panel. The presence of this reference indicates
+ that the touchscreen should be power sequenced together with the panel
+ and that they may share power and/or reset signals.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
touchscreen-min-x:
description: minimum x coordinate reported
$ref: /schemas/types.yaml#/definitions/uint32
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply related
* [PATCH v4 00/11] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Douglas Anderson @ 2023-07-27 17:16 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: linux-arm-msm, yangcong5, devicetree, Daniel Vetter, hsinyi,
Chris Morgan, linux-input, cros-qcom-dts-watchers,
Dmitry Torokhov, linux-kernel, dri-devel, 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 4 of this series adds a new patch that suspends i2c-hid
devices at remove time even for non panel-followers to make things
consistent. It also attempts to isolate the panel follower code a bit
more as per Benjamin's feedback on v3 and adds an item to the DRM todo
list as per Maxime's request. As per Maxime's response to my v3 cover
letter, I added his Reviewed-by tag to all 10 patches that were part
of v3 (but left it off of the new i2c-hid patch in v4).
Version 3 of this series was a long time coming after v2. Maxime and I
had a very long discussion trying to figure out if there was a beter
way and in the end we didn't find one so he was OK with the series in
general [1]. After that got resolved, I tried to resolve Benjamin's
feedback but got stuck [2]. Eventually I made my best guess. The end
result was a v3 that wasn't that different from v2 but that had a tiny
bit more code split out.
Version 2 of this patch series didn't change too much. At a high level:
* I added all the forgotten "static" to functions.
* I've hopefully made the bindings better.
* I've integrated into fw_devlink.
* I cleaned up a few descriptions / comments.
As far as I can tell, as of v4 everyone is on the same page that this
patch series looks like a reasonable solution to the problem and we
just need to get all the nits fixed and figure out how to land it.
[1] https://lore.kernel.org/r/gkwymmfkdy2p2evz22wmbwgw42ii4wnvmvu64m3bghmj2jhv7x@4mbstjxnagxd
[2] https://lore.kernel.org/r/CAD=FV=VbdeomBGbWhppY+5TOSwt64GWBHga68OXFwsnO4gg4UA@mail.gmail.com
Changes in v4:
- Document further cleanup in the official DRM todo list.
- ("Suspend i2c-hid devices in remove") new for v4.
- Move panel follower alternative checks to wrapper functions.
- Rebase atop ("Suspend i2c-hid devices in remove").
Changes in v3:
- Add is_panel_follower() as a convenience for clients.
- Add "depends on DRM || !DRM" to Kconfig to avoid randconfig error.
- Split more of the panel follower code out of the core.
Changes in v2:
- Move the description to the generic touchscreen.yaml.
- Update the desc to make it clearer it's only for integrated devices.
- Add even more text to the commit message.
- A few comment cleanups.
- ("Add a devlink for panel followers") new for v2.
- i2c_hid_core_initial_power_up() is now static.
- i2c_hid_core_panel_prepared() and ..._unpreparing() are now static.
- ihid_core_panel_prepare_work() is now static.
- Improve documentation for smp_wmb().
Douglas Anderson (11):
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: Suspend i2c-hid devices in remove
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 +
Documentation/gpu/todo.rst | 24 ++
.../boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 1 +
.../dts/qcom/sc7180-trogdor-homestar.dtsi | 1 +
.../boot/dts/qcom/sc7180-trogdor-lazor.dtsi | 1 +
.../boot/dts/qcom/sc7180-trogdor-pompom.dtsi | 1 +
.../qcom/sc7180-trogdor-quackingstick.dtsi | 1 +
.../dts/qcom/sc7180-trogdor-wormdingler.dtsi | 1 +
drivers/gpu/drm/drm_panel.c | 218 ++++++++++-
drivers/hid/i2c-hid/Kconfig | 2 +
drivers/hid/i2c-hid/i2c-hid-core.c | 349 +++++++++++++-----
drivers/of/property.c | 2 +
include/drm/drm_panel.h | 94 +++++
16 files changed, 617 insertions(+), 97 deletions(-)
--
2.41.0.487.g6d72f3e995-goog
^ permalink raw reply
* [PATCH] Input: ads7846 - don't set ABS_PRESSURE when min == max
From: Benjamin Bara @ 2023-07-27 15:19 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Benjamin Bara
From: Benjamin Bara <benjamin.bara@skidata.com>
When the optional fields "pressure_min" and "pressure_max" are not set,
both fall back to 0, which results to the following libinput error:
ADS7846 Touchscreen: kernel bug: device has min == max on ABS_PRESSURE
Avoid it by only setting ABS_PRESSURE if the values differ.
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
drivers/input/touchscreen/ads7846.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index faea40dd66d0..2535424a5630 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -1281,7 +1281,7 @@ static int ads7846_probe(struct spi_device *spi)
pdata->y_min ? : 0,
pdata->y_max ? : MAX_12BIT,
0, 0);
- if (ts->model != 7845)
+ if (ts->model != 7845 && pdata->pressure_min != pdata->pressure_max)
input_set_abs_params(input_dev, ABS_PRESSURE,
pdata->pressure_min, pdata->pressure_max, 0, 0);
---
base-commit: 451cc82bd11eb6a374f4dbcfc1cf007eafea91ab
change-id: 20230727-ads7846-pressure-05edfb01887f
Best regards,
--
Benjamin Bara <benjamin.bara@skidata.com>
^ permalink raw reply related
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Fenglin Wu @ 2023-07-27 10:51 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <b0999940-8cd1-349d-9bf4-d6494145254c@linaro.org>
On 7/27/2023 4:29 PM, Krzysztof Kozlowski wrote:
> On 27/07/2023 09:54, Fenglin Wu wrote:
>>>>> + - enum:
>>>>> + - qcom,pm7550ba-vib
>>>>> + - const: qcom,pm7325b-vib
>>>>>
>>>>
>>>> Yes
>>>
>>> I wonder why this approved change turned out to something incorrect in
>>> your v3 patch...
>>>
>> Since I got review comments in the driver change and I was told to
>> refactor the driver before adding new HW support. I added the HW type
>> logic in the driver and I was thinking it might be good to add some
>> generic compatible strings to match with the HW type introduced in the
>> driver change.
>>
>> Anyway, I will update it to what you suggested in next patch.
>
> Drivers are not really related to bindings, so whatever HW type you add
> in driver, is not a reason to change bindings. Reason to change bindings
> could be for example: because hardware is like that.
>
Understood.
Sorry, I forgot to mention, in v3, I added the 'reg' value to the
register offset and no longer hard code the 16-bit register address,
that makes the vibrators inside PMI632/PM7250B/PM7325B/PM7550BA all
compatible, and that was another motivation of adding a generic
compatible string and make the others as the fallback.
This will be still the case in v4, I might keep it similar in v3 but
just drop "qcom,spmi-vib-gen1"
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator
From: Fenglin Wu @ 2023-07-27 9:40 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-arm-msm, linux-kernel,
krzysztof.kozlowski+dt, robh+dt, agross, andersson,
dmitry.baryshkov, Konrad Dybcio, Dmitry Torokhov, linux-input
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar
In-Reply-To: <a27ad44c-bbc9-0a2e-44fe-ee9b787d0cd4@linaro.org>
On 7/27/2023 5:22 PM, Krzysztof Kozlowski wrote:
> On 27/07/2023 09:43, Fenglin Wu wrote:
>>
>>
>> On 7/27/2023 3:07 PM, Krzysztof Kozlowski wrote:
>>> On 25/07/2023 08:16, Fenglin Wu wrote:
>>>>>>
>>>>>> -static const struct pm8xxx_regs pm8058_regs = {
>>>>>> - .drv_addr = 0x4A,
>>>>>> - .drv_mask = 0xf8,
>>>>>> - .drv_shift = 3,
>>>>>> - .drv_en_manual_mask = 0xfc,
>>>>>> +static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = {
>>>>>
>>>>> Change from const to non-const is wrong. How do you support multiple
>>>>> devices? No, this is way too fragile now.
>>>>>
>>>>
>>>> The register definition is no longer used as the match data, hw_type is
>>>> used.
>>>>
>>>> The last suggestion was getting the register base address from the DT
>>>> and it has to be added into the offset of SPMI vibrator registers
>>>> (either in the previous hard-coded format or the later the reg_filed
>>>> data structure), so it's not appropriated to make it constant.
>>>>
>>>> I don't understand this question: "How do you support multiple devices?"
>>>> For SSBI vibrator, since all the registers are fixed, and I would assume
>>>> that there is no chance to support multiple vibrator devices on the same
>>>> SSBI bus. If they are not on the same bus, the regmap device will be
>>>> different while the registers definition is the same, and we are still
>>>> able to support multiple devices, right?
>>>
>>> No, you have static memory. One device probes and changes static memory
>>> to reg+=base1. Second device probes and changes the same to reg+=base2.
>>
>> Thanks, got it. I can update it with following 2 options:
>>
>> 1) keep the register definition in 'reg_filed' data structure and make
>> it constant, copy it to a dynamically allocated memory before adding the
>> 'reg_base' to the '.reg' variable.
>>
>> 2) Define the register offsets as constant data and add the 'reg_base'
>> to the 'reg' while using 'regmap_read()'/'regmap_write()' functions.
>>
>> which one is the preferred way?
>
> Depends on the code. I am not sure if 2 would work with regmap_fields.
> OTOH, I wonder if the device could just create its own regmap instead of
> using parents? Then there would be no need of this offset dance.
>
> Anyway, adding offset only for some variants seems also not needed. You
> should add offset to each variant, because each device has this offset.
>
> Best regards,
> Krzysztof
> Thanks for the suggestion.
The Qualcomm SPMI device has to use the 'regmap' from its parent with 16
'reg_bits' and 8 'val_bits' config, the higher 8-bit 'reg_bits' is the
peripheral ID (PID) and it could be different in different PMICs even
for the same type of HW module, and (PID << 8) is the 'reg_base' here.
I assume that you are not in favor of copying the constant data into a
dynamic allocated memory, so I will go with option 2.
Thanks
^ permalink raw reply
* Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator
From: Krzysztof Kozlowski @ 2023-07-27 9:22 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, krzysztof.kozlowski+dt,
robh+dt, agross, andersson, dmitry.baryshkov, Konrad Dybcio,
Dmitry Torokhov, linux-input
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar
In-Reply-To: <4e416602-8dea-fa6d-d083-f93b730552c3@quicinc.com>
On 27/07/2023 09:43, Fenglin Wu wrote:
>
>
> On 7/27/2023 3:07 PM, Krzysztof Kozlowski wrote:
>> On 25/07/2023 08:16, Fenglin Wu wrote:
>>>>>
>>>>> -static const struct pm8xxx_regs pm8058_regs = {
>>>>> - .drv_addr = 0x4A,
>>>>> - .drv_mask = 0xf8,
>>>>> - .drv_shift = 3,
>>>>> - .drv_en_manual_mask = 0xfc,
>>>>> +static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = {
>>>>
>>>> Change from const to non-const is wrong. How do you support multiple
>>>> devices? No, this is way too fragile now.
>>>>
>>>
>>> The register definition is no longer used as the match data, hw_type is
>>> used.
>>>
>>> The last suggestion was getting the register base address from the DT
>>> and it has to be added into the offset of SPMI vibrator registers
>>> (either in the previous hard-coded format or the later the reg_filed
>>> data structure), so it's not appropriated to make it constant.
>>>
>>> I don't understand this question: "How do you support multiple devices?"
>>> For SSBI vibrator, since all the registers are fixed, and I would assume
>>> that there is no chance to support multiple vibrator devices on the same
>>> SSBI bus. If they are not on the same bus, the regmap device will be
>>> different while the registers definition is the same, and we are still
>>> able to support multiple devices, right?
>>
>> No, you have static memory. One device probes and changes static memory
>> to reg+=base1. Second device probes and changes the same to reg+=base2.
>
> Thanks, got it. I can update it with following 2 options:
>
> 1) keep the register definition in 'reg_filed' data structure and make
> it constant, copy it to a dynamically allocated memory before adding the
> 'reg_base' to the '.reg' variable.
>
> 2) Define the register offsets as constant data and add the 'reg_base'
> to the 'reg' while using 'regmap_read()'/'regmap_write()' functions.
>
> which one is the preferred way?
Depends on the code. I am not sure if 2 would work with regmap_fields.
OTOH, I wonder if the device could just create its own regmap instead of
using parents? Then there would be no need of this offset dance.
Anyway, adding offset only for some variants seems also not needed. You
should add offset to each variant, because each device has this offset.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Krzysztof Kozlowski @ 2023-07-27 8:29 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <f1286da7-05a6-c8aa-d13b-075c0fd45b77@quicinc.com>
On 27/07/2023 09:54, Fenglin Wu wrote:
>>>> + - enum:
>>>> + - qcom,pm7550ba-vib
>>>> + - const: qcom,pm7325b-vib
>>>>
>>>
>>> Yes
>>
>> I wonder why this approved change turned out to something incorrect in
>> your v3 patch...
>>
> Since I got review comments in the driver change and I was told to
> refactor the driver before adding new HW support. I added the HW type
> logic in the driver and I was thinking it might be good to add some
> generic compatible strings to match with the HW type introduced in the
> driver change.
>
> Anyway, I will update it to what you suggested in next patch.
Drivers are not really related to bindings, so whatever HW type you add
in driver, is not a reason to change bindings. Reason to change bindings
could be for example: because hardware is like that.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Fenglin Wu @ 2023-07-27 7:54 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <2fa3f27d-ff08-b923-2fb1-cf7cc888e5d5@linaro.org>
On 7/27/2023 3:10 PM, Krzysztof Kozlowski wrote:
> On 18/07/2023 10:02, Krzysztof Kozlowski wrote:
>> On 18/07/2023 09:59, Fenglin Wu wrote:
>>
>>>>> Just FYI,the change log was updated in the cover letter here:
>>>>> https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f
>>>>>
>>>>> Also the commit text and the driver change were also updated accordingly
>>>>> to address your review comment by removing 'pm7550ba-vib' compatible string.
>>>>
>>>> Removing compatible was never my feedback. Did you read:
>>>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
>>>> ?
>>>>
>>> Okay, so do you want me to add 'pm7550ba-vib' as a fallback compatible
>>> like this?
>>>
>>> properties:
>>> compatible:
>>> - enum:
>>> - - qcom,pm8058-vib
>>> - - qcom,pm8916-vib
>>> - - qcom,pm8921-vib
>>> - - qcom,pmi632-vib
>>> - - qcom,pm7250b-vib
>>> - - qcom,pm7325b-vib
>>> + oneOf:
>>> + - enum:
>>> + - qcom,pm8058-vib
>>> + - qcom,pm8916-vib
>>> + - qcom,pm8921-vib
>>> + - qcom,pmi632-vib
>>> + - qcom,pm7250b-vib
>>> + - qcom,pm7325b-vib
>>> + - items:
>>> + - enum:
>>> + - qcom,pm7550ba-vib
>>> + - const: qcom,pm7325b-vib
>>>
>>
>> Yes
>
> I wonder why this approved change turned out to something incorrect in
> your v3 patch...
>
Since I got review comments in the driver change and I was told to
refactor the driver before adding new HW support. I added the HW type
logic in the driver and I was thinking it might be good to add some
generic compatible strings to match with the HW type introduced in the
driver change.
Anyway, I will update it to what you suggested in next patch.
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator
From: Fenglin Wu @ 2023-07-27 7:43 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-arm-msm, linux-kernel,
krzysztof.kozlowski+dt, robh+dt, agross, andersson,
dmitry.baryshkov, Konrad Dybcio, Dmitry Torokhov, linux-input
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar
In-Reply-To: <2a09e743-7423-65b0-c70d-87ae8105182a@linaro.org>
On 7/27/2023 3:07 PM, Krzysztof Kozlowski wrote:
> On 25/07/2023 08:16, Fenglin Wu wrote:
>>>>
>>>> -static const struct pm8xxx_regs pm8058_regs = {
>>>> - .drv_addr = 0x4A,
>>>> - .drv_mask = 0xf8,
>>>> - .drv_shift = 3,
>>>> - .drv_en_manual_mask = 0xfc,
>>>> +static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = {
>>>
>>> Change from const to non-const is wrong. How do you support multiple
>>> devices? No, this is way too fragile now.
>>>
>>
>> The register definition is no longer used as the match data, hw_type is
>> used.
>>
>> The last suggestion was getting the register base address from the DT
>> and it has to be added into the offset of SPMI vibrator registers
>> (either in the previous hard-coded format or the later the reg_filed
>> data structure), so it's not appropriated to make it constant.
>>
>> I don't understand this question: "How do you support multiple devices?"
>> For SSBI vibrator, since all the registers are fixed, and I would assume
>> that there is no chance to support multiple vibrator devices on the same
>> SSBI bus. If they are not on the same bus, the regmap device will be
>> different while the registers definition is the same, and we are still
>> able to support multiple devices, right?
>
> No, you have static memory. One device probes and changes static memory
> to reg+=base1. Second device probes and changes the same to reg+=base2.
Thanks, got it. I can update it with following 2 options:
1) keep the register definition in 'reg_filed' data structure and make
it constant, copy it to a dynamically allocated memory before adding the
'reg_base' to the '.reg' variable.
2) Define the register offsets as constant data and add the 'reg_base'
to the 'reg' while using 'regmap_read()'/'regmap_write()' functions.
which one is the preferred way?
>
>> The similar story for SPMI vibrators and it can support multiple devices
>> if they are located on different SPMI bus, or even if they are on the
>> same SPMI bus but just having different SID or PID.
>
> Sorry, such code cannot go in. These must stay const and you must write
> driver without any static allocations or singleton-like patterns.
>
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator
From: Krzysztof Kozlowski @ 2023-07-27 7:07 UTC (permalink / raw)
To: kernel test robot, Fenglin Wu, linux-arm-msm, linux-kernel,
krzysztof.kozlowski+dt, robh+dt, agross, andersson,
dmitry.baryshkov, Konrad Dybcio, Dmitry Torokhov, linux-input
Cc: llvm, oe-kbuild-all, quic_collinsd, quic_subbaram, quic_kamalw,
jestar
In-Reply-To: <202307251741.PMtlVAgD-lkp@intel.com>
On 25/07/2023 12:01, kernel test robot wrote:
> Hi Fenglin,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on dtor-input/next]
> [also build test WARNING on dtor-input/for-linus linus/master v6.5-rc3 next-20230725]
> [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/Fenglin-Wu/input-pm8xxx-vib-refactor-to-easily-support-new-SPMI-vibrator/20230725-134504
> base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
> patch link: https://lore.kernel.org/r/20230725054138.129497-2-quic_fenglinw%40quicinc.com
> patch subject: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator
> config: x86_64-buildonly-randconfig-r002-20230725 (https://download.01.org/0day-ci/archive/20230725/202307251741.PMtlVAgD-lkp@intel.com/config)
> compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
> reproduce: (https://download.01.org/0day-ci/archive/20230725/202307251741.PMtlVAgD-lkp@intel.com/reproduce)
>
> 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/202307251741.PMtlVAgD-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
>>> drivers/input/misc/pm8xxx-vibrator.c:190:17: warning: cast to smaller integer type 'enum pm8xxx_vib_type' from 'const void *' [-Wvoid-pointer-to-enum-cast]
> vib->hw_type = (enum pm8xxx_vib_type)of_device_get_match_data(dev);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.
>
Remember to fix all the warnings.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: qcom,pm8xxx-vib: add more PMIC support
From: Krzysztof Kozlowski @ 2023-07-27 7:10 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar, quic_huliu
In-Reply-To: <dd5864ee-7df2-eb64-c7f2-0fb234900d6a@linaro.org>
On 18/07/2023 10:02, Krzysztof Kozlowski wrote:
> On 18/07/2023 09:59, Fenglin Wu wrote:
>
>>>> Just FYI,the change log was updated in the cover letter here:
>>>> https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-1-quic_fenglinw@quicinc.com/T/#m3819b50503ef19e0933a10bf797351a4af35537f
>>>>
>>>> Also the commit text and the driver change were also updated accordingly
>>>> to address your review comment by removing 'pm7550ba-vib' compatible string.
>>>
>>> Removing compatible was never my feedback. Did you read:
>>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
>>> ?
>>>
>> Okay, so do you want me to add 'pm7550ba-vib' as a fallback compatible
>> like this?
>>
>> properties:
>> compatible:
>> - enum:
>> - - qcom,pm8058-vib
>> - - qcom,pm8916-vib
>> - - qcom,pm8921-vib
>> - - qcom,pmi632-vib
>> - - qcom,pm7250b-vib
>> - - qcom,pm7325b-vib
>> + oneOf:
>> + - enum:
>> + - qcom,pm8058-vib
>> + - qcom,pm8916-vib
>> + - qcom,pm8921-vib
>> + - qcom,pmi632-vib
>> + - qcom,pm7250b-vib
>> + - qcom,pm7325b-vib
>> + - items:
>> + - enum:
>> + - qcom,pm7550ba-vib
>> + - const: qcom,pm7325b-vib
>>
>
> Yes
I wonder why this approved change turned out to something incorrect in
your v3 patch...
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 2/3] dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
From: Krzysztof Kozlowski @ 2023-07-27 7:09 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, krzysztof.kozlowski+dt,
robh+dt, agross, andersson, dmitry.baryshkov, Konrad Dybcio,
Dmitry Torokhov, linux-input, devicetree
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar
In-Reply-To: <3aaccc94-59b3-31d3-eac7-f8926f8c88ff@quicinc.com>
On 25/07/2023 08:26, Fenglin Wu wrote:
>
>
> On 7/25/2023 1:53 PM, Krzysztof Kozlowski wrote:
>> On 25/07/2023 07:41, Fenglin Wu wrote:
>>> Add compatible string 'qcom,spmi-vib-gen2' for vibrator module inside
>>> PMI632, PMI7250B, PM7325B, PM7550BA. Also, add 'qcom,spmi-vib-gen1'
>>> string for the SPMI vibrator inside PM8916 to maintain the completeness
>>> of the hardware version history for SPMI vibrators.
>>>
>>> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
>>> ---
>>> .../bindings/input/qcom,pm8xxx-vib.yaml | 18 ++++++++++++++----
>>> 1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>>> index c8832cd0d7da..ab778714ad29 100644
>>> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>>> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
>>> @@ -11,10 +11,20 @@ maintainers:
>>>
>>> properties:
>>> compatible:
>>> - enum:
>>> - - qcom,pm8058-vib
>>> - - qcom,pm8916-vib
>>> - - qcom,pm8921-vib
>>> + oneOf:
>>> + - enum:
>>> + - qcom,pm8058-vib
>>> + - qcom,pm8916-vib
>>> + - qcom,pm8921-vib
>>> + - qcom,spmi-vib-gen1
>>> + - qcom,spmi-vib-gen2
>>
>> Generic compatibles should not be alone. Drop both lines.
>
> Sure. I will remove 'qcom,spmi-vib-gen2'.
> Should I also keep 'qcom,spmi-vib-gen1' as generic compatible and move
> 'qcom,pm8916-vib' as its fallback as following?
I would drop all of generic ones. Entirely remove qcom,spmi-vib-gen2 and
qcom,spmi-vib-gen1.
Use device specific compatibles names only. As fallback and as first
compatible.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator
From: Krzysztof Kozlowski @ 2023-07-27 7:07 UTC (permalink / raw)
To: Fenglin Wu, linux-arm-msm, linux-kernel, krzysztof.kozlowski+dt,
robh+dt, agross, andersson, dmitry.baryshkov, Konrad Dybcio,
Dmitry Torokhov, linux-input
Cc: quic_collinsd, quic_subbaram, quic_kamalw, jestar
In-Reply-To: <053c9571-d709-2826-fced-a00dd7255b8b@quicinc.com>
On 25/07/2023 08:16, Fenglin Wu wrote:
>>>
>>> -static const struct pm8xxx_regs pm8058_regs = {
>>> - .drv_addr = 0x4A,
>>> - .drv_mask = 0xf8,
>>> - .drv_shift = 3,
>>> - .drv_en_manual_mask = 0xfc,
>>> +static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = {
>>
>> Change from const to non-const is wrong. How do you support multiple
>> devices? No, this is way too fragile now.
>>
>
> The register definition is no longer used as the match data, hw_type is
> used.
>
> The last suggestion was getting the register base address from the DT
> and it has to be added into the offset of SPMI vibrator registers
> (either in the previous hard-coded format or the later the reg_filed
> data structure), so it's not appropriated to make it constant.
>
> I don't understand this question: "How do you support multiple devices?"
> For SSBI vibrator, since all the registers are fixed, and I would assume
> that there is no chance to support multiple vibrator devices on the same
> SSBI bus. If they are not on the same bus, the regmap device will be
> different while the registers definition is the same, and we are still
> able to support multiple devices, right?
No, you have static memory. One device probes and changes static memory
to reg+=base1. Second device probes and changes the same to reg+=base2.
> The similar story for SPMI vibrators and it can support multiple devices
> if they are located on different SPMI bus, or even if they are on the
> same SPMI bus but just having different SID or PID.
Sorry, such code cannot go in. These must stay const and you must write
driver without any static allocations or singleton-like patterns.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 02/10] drm/panel: Check for already prepared/enabled in drm_panel
From: Maxime Ripard @ 2023-07-27 6:37 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, cros-qcom-dts-watchers, Chris Morgan,
linux-input, hsinyi, linux-arm-msm, dri-devel, linux-kernel,
Dmitry Torokhov, devicetree, Daniel Vetter, yangcong5
In-Reply-To: <CAD=FV=VcsTik+HD11xeDM2Jq9ispcX0-j5QtK8D1qUkrGabRGg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]
Hi,
On Wed, Jul 26, 2023 at 08:10:33AM -0700, Doug Anderson wrote:
> On Wed, Jul 26, 2023 at 5:41 AM Maxime Ripard <mripard@kernel.org> wrote:
> > On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> > > NOTE: arguably, the right thing to do here is actually to skip this
> > > patch and simply remove all the extra checks from the individual
> > > drivers. Perhaps the checks were needed at some point in time in the
> > > past but maybe they no longer are? Certainly as we continue
> > > transitioning over to "panel_bridge" then we expect there to be much
> > > less variety in how these calls are made. When we're called as part of
> > > the bridge chain, things should be pretty simple. In fact, there was
> > > some discussion in the past about these checks [1], including a
> > > discussion about whether the checks were needed and whether the calls
> > > ought to be refcounted. At the time, I decided not to mess with it
> > > because it felt too risky.
> >
> > Yeah, I'd agree here too. I've never found evidence that it was actually
> > needed and it really looks like cargo cult to me.
> >
> > And if it was needed, then I'm not sure we need refcounting either. We
> > don't have refcounting for atomic_enable / disable, we have a sound API
> > design that makes sure we don't fall into that trap :)
> >
> > > Looking closer at it now, I'm fairly certain that nothing in the
> > > existing codebase is expecting these calls to be refcounted. The only
> > > real question is whether someone is already doing something to ensure
> > > prepare()/unprepare() match and enabled()/disable() match. I would say
> > > that, even if there is something else ensuring that things match,
> > > there's enough complexity that adding an extra bool and an extra
> > > double-check here is a good idea. Let's add a drm_warn() to let people
> > > know that it's considered a minor error to take advantage of
> > > drm_panel's double-checking but we'll still make things work fine.
> >
> > I'm ok with this, if we follow-up in a couple of releases and remove it
> > and all the calls.
> >
> > Could you add a TODO item so that we can keep a track of it? A follow-up
> > is fine if you don't send a new version of that series.
>
> By this, I think you mean to add a "TODO" comment inline in the code?
No, sorry, I meant an entry in our TODO list: Documentation/gpu/todo.rst
> Also: I was thinking that we'd keep the check in "drm_panel.c" with
> the warning message indefinitely. You think it should be eventually
> removed? If we are truly thinking of removing it eventually, this
> feels like it should be a more serious warning message like a WARN(1,
> ...) to make it really obvious to people that they're relying on
> behavior that will eventually go away.
Yeah, it really feels like this is cargo-cult to me. Your approach seems
like a good short-term thing to do to warn everyone but eventually we'll
want it to go away.
So promoting it to a WARN could be a good thing, or let's say we do a
drm_warn for now, WARN next release, and gone in two?
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [dtor-input:next] BUILD SUCCESS 92e24e0e57f72e06c2df87116557331fd2d4dda2
From: kernel test robot @ 2023-07-27 5:38 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: 92e24e0e57f72e06c2df87116557331fd2d4dda2 Input: psmouse - add delay when deactivating for SMBus mode
elapsed time: 1444m
configs tested: 235
configs skipped: 20
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allyesconfig gcc
alpha defconfig gcc
alpha randconfig-r002-20230726 gcc
alpha randconfig-r004-20230726 gcc
alpha randconfig-r011-20230726 gcc
alpha randconfig-r013-20230726 gcc
alpha randconfig-r024-20230726 gcc
alpha randconfig-r032-20230726 gcc
arc allyesconfig gcc
arc defconfig gcc
arc haps_hs_smp_defconfig gcc
arc nsim_700_defconfig gcc
arc randconfig-r003-20230726 gcc
arc randconfig-r006-20230726 gcc
arc randconfig-r022-20230726 gcc
arc randconfig-r034-20230726 gcc
arc randconfig-r035-20230726 gcc
arc randconfig-r043-20230726 gcc
arm alldefconfig clang
arm allmodconfig gcc
arm allyesconfig gcc
arm defconfig gcc
arm h3600_defconfig gcc
arm randconfig-r004-20230726 clang
arm randconfig-r014-20230726 gcc
arm randconfig-r032-20230726 clang
arm randconfig-r046-20230726 gcc
arm s5pv210_defconfig clang
arm64 allyesconfig gcc
arm64 defconfig gcc
arm64 randconfig-r002-20230726 gcc
arm64 randconfig-r011-20230726 clang
arm64 randconfig-r013-20230726 clang
arm64 randconfig-r026-20230726 clang
csky defconfig gcc
csky randconfig-r026-20230726 gcc
csky randconfig-r033-20230726 gcc
hexagon randconfig-r014-20230726 clang
hexagon randconfig-r022-20230726 clang
hexagon randconfig-r023-20230726 clang
hexagon randconfig-r034-20230726 clang
hexagon randconfig-r036-20230726 clang
hexagon randconfig-r041-20230726 clang
hexagon randconfig-r045-20230726 clang
i386 allyesconfig gcc
i386 buildonly-randconfig-r004-20230726 gcc
i386 buildonly-randconfig-r005-20230726 gcc
i386 buildonly-randconfig-r006-20230726 gcc
i386 debian-10.3 gcc
i386 defconfig gcc
i386 randconfig-i001-20230726 gcc
i386 randconfig-i002-20230726 gcc
i386 randconfig-i003-20230726 gcc
i386 randconfig-i004-20230726 gcc
i386 randconfig-i005-20230726 gcc
i386 randconfig-i006-20230726 gcc
i386 randconfig-i011-20230726 clang
i386 randconfig-i011-20230727 gcc
i386 randconfig-i012-20230726 clang
i386 randconfig-i012-20230727 gcc
i386 randconfig-i013-20230726 clang
i386 randconfig-i013-20230727 gcc
i386 randconfig-i014-20230726 clang
i386 randconfig-i014-20230727 gcc
i386 randconfig-i015-20230726 clang
i386 randconfig-i015-20230727 gcc
i386 randconfig-i016-20230726 clang
i386 randconfig-i016-20230727 gcc
i386 randconfig-r006-20230726 gcc
i386 randconfig-r011-20230726 clang
i386 randconfig-r013-20230726 clang
i386 randconfig-r014-20230726 clang
i386 randconfig-r016-20230726 clang
i386 randconfig-r026-20230726 clang
i386 randconfig-r034-20230726 gcc
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch defconfig gcc
loongarch randconfig-r001-20230726 gcc
loongarch randconfig-r003-20230726 gcc
loongarch randconfig-r004-20230726 gcc
loongarch randconfig-r005-20230726 gcc
loongarch randconfig-r006-20230726 gcc
loongarch randconfig-r015-20230726 gcc
loongarch randconfig-r016-20230726 gcc
m68k allmodconfig gcc
m68k allyesconfig gcc
m68k amcore_defconfig gcc
m68k defconfig gcc
m68k m5307c3_defconfig gcc
m68k randconfig-r001-20230726 gcc
m68k randconfig-r002-20230726 gcc
m68k randconfig-r012-20230726 gcc
m68k randconfig-r024-20230726 gcc
m68k randconfig-r036-20230726 gcc
microblaze mmu_defconfig gcc
microblaze randconfig-r001-20230726 gcc
microblaze randconfig-r006-20230726 gcc
microblaze randconfig-r012-20230726 gcc
mips allmodconfig gcc
mips allyesconfig gcc
mips decstation_r4k_defconfig gcc
mips lemote2f_defconfig clang
mips loongson2k_defconfig clang
mips malta_qemu_32r6_defconfig clang
mips randconfig-r001-20230726 clang
mips randconfig-r012-20230726 gcc
mips randconfig-r023-20230726 gcc
mips randconfig-r032-20230726 clang
mips randconfig-r035-20230726 clang
mips rb532_defconfig gcc
mips rbtx49xx_defconfig clang
nios2 defconfig gcc
nios2 randconfig-r001-20230726 gcc
nios2 randconfig-r003-20230726 gcc
nios2 randconfig-r004-20230726 gcc
nios2 randconfig-r005-20230726 gcc
nios2 randconfig-r014-20230726 gcc
nios2 randconfig-r015-20230726 gcc
nios2 randconfig-r022-20230726 gcc
nios2 randconfig-r024-20230726 gcc
nios2 randconfig-r025-20230726 gcc
nios2 randconfig-r033-20230726 gcc
openrisc randconfig-r005-20230726 gcc
openrisc randconfig-r022-20230726 gcc
openrisc randconfig-r025-20230726 gcc
openrisc randconfig-r026-20230726 gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc generic-64bit_defconfig gcc
parisc randconfig-r002-20230726 gcc
parisc randconfig-r006-20230726 gcc
parisc randconfig-r011-20230726 gcc
parisc randconfig-r014-20230726 gcc
parisc randconfig-r025-20230726 gcc
parisc64 defconfig gcc
powerpc allmodconfig gcc
powerpc allnoconfig gcc
powerpc ebony_defconfig clang
powerpc ge_imp3a_defconfig clang
powerpc powernv_defconfig clang
powerpc ppa8548_defconfig clang
powerpc randconfig-r005-20230726 gcc
powerpc randconfig-r006-20230726 gcc
powerpc randconfig-r013-20230726 clang
powerpc randconfig-r025-20230726 clang
powerpc randconfig-r032-20230726 gcc
powerpc randconfig-r036-20230726 gcc
powerpc storcenter_defconfig gcc
riscv allmodconfig gcc
riscv allnoconfig clang
riscv allnoconfig gcc
riscv allyesconfig gcc
riscv defconfig gcc
riscv randconfig-r001-20230726 gcc
riscv randconfig-r005-20230726 gcc
riscv randconfig-r015-20230726 clang
riscv randconfig-r016-20230726 clang
riscv randconfig-r022-20230726 clang
riscv randconfig-r025-20230726 clang
riscv randconfig-r032-20230726 gcc
riscv randconfig-r035-20230726 gcc
riscv randconfig-r036-20230726 gcc
riscv randconfig-r042-20230726 clang
riscv rv32_defconfig gcc
s390 allmodconfig gcc
s390 allyesconfig gcc
s390 defconfig gcc
s390 randconfig-r004-20230726 gcc
s390 randconfig-r005-20230726 gcc
s390 randconfig-r014-20230726 clang
s390 randconfig-r032-20230726 gcc
s390 randconfig-r034-20230726 gcc
s390 randconfig-r044-20230726 clang
s390 zfcpdump_defconfig gcc
sh allmodconfig gcc
sh ecovec24-romimage_defconfig gcc
sh magicpanelr2_defconfig gcc
sh randconfig-r001-20230726 gcc
sh randconfig-r033-20230726 gcc
sh rsk7264_defconfig gcc
sh se7721_defconfig gcc
sh se7750_defconfig gcc
sh sh7710voipgw_defconfig gcc
sparc allyesconfig gcc
sparc defconfig gcc
sparc randconfig-r015-20230726 gcc
sparc randconfig-r016-20230726 gcc
sparc randconfig-r024-20230726 gcc
sparc randconfig-r026-20230726 gcc
sparc randconfig-r033-20230726 gcc
sparc64 randconfig-r013-20230726 gcc
sparc64 randconfig-r025-20230726 gcc
sparc64 randconfig-r031-20230726 gcc
um alldefconfig gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig clang
um defconfig gcc
um i386_defconfig gcc
um randconfig-r001-20230726 clang
um randconfig-r021-20230726 gcc
um randconfig-r035-20230726 clang
um x86_64_defconfig gcc
x86_64 allyesconfig gcc
x86_64 buildonly-randconfig-r001-20230726 gcc
x86_64 buildonly-randconfig-r002-20230726 gcc
x86_64 buildonly-randconfig-r003-20230726 gcc
x86_64 defconfig gcc
x86_64 kexec gcc
x86_64 randconfig-r005-20230726 gcc
x86_64 randconfig-r014-20230726 clang
x86_64 randconfig-x001-20230726 clang
x86_64 randconfig-x002-20230726 clang
x86_64 randconfig-x003-20230726 clang
x86_64 randconfig-x004-20230726 clang
x86_64 randconfig-x005-20230726 clang
x86_64 randconfig-x006-20230726 clang
x86_64 randconfig-x011-20230726 gcc
x86_64 randconfig-x012-20230726 gcc
x86_64 randconfig-x013-20230726 gcc
x86_64 randconfig-x014-20230726 gcc
x86_64 randconfig-x015-20230726 gcc
x86_64 randconfig-x016-20230726 gcc
x86_64 rhel-8.3-bpf gcc
x86_64 rhel-8.3-func gcc
x86_64 rhel-8.3-kselftests gcc
x86_64 rhel-8.3-kunit gcc
x86_64 rhel-8.3-ltp gcc
x86_64 rhel-8.3-rust clang
x86_64 rhel-8.3 gcc
xtensa randconfig-r002-20230726 gcc
xtensa randconfig-r006-20230726 gcc
xtensa randconfig-r015-20230726 gcc
xtensa randconfig-r023-20230726 gcc
xtensa randconfig-r025-20230726 gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v3 08/10] HID: i2c-hid: Support being a panel follower
From: Benjamin Tissoires @ 2023-07-26 16:45 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, Maxime Ripard,
Thomas Zimmermann, devicetree, cros-qcom-dts-watchers,
linux-arm-msm, yangcong5, Dmitry Torokhov, linux-kernel,
dri-devel, Chris Morgan, linux-input, hsinyi
In-Reply-To: <CAD=FV=VX=ACR3K+GYAvP8J4ebP4GtTpXQmX21NkJ4BJ7vN+o8w@mail.gmail.com>
On Jul 26 2023, Doug Anderson wrote:
> Hi,
>
> On Wed, Jul 26, 2023 at 1:57 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> >
> > > @@ -1143,7 +1208,14 @@ void i2c_hid_core_remove(struct i2c_client *client)
> > > struct i2c_hid *ihid = i2c_get_clientdata(client);
> > > struct hid_device *hid;
> > >
> > > - i2c_hid_core_power_down(ihid);
> > > + /*
> > > + * If we're a follower, the act of unfollowing will cause us to be
> > > + * powered down. Otherwise we need to manually do it.
> > > + */
> > > + if (ihid->is_panel_follower)
> > > + drm_panel_remove_follower(&ihid->panel_follower);
> >
> > That part is concerning, as we are now calling hid_drv->suspend() when removing
> > the device. It might or not have an impact (I'm not sure of it), but we
> > are effectively changing the path of commands sent to the device.
> >
> > hid-multitouch might call a feature in ->suspend, but the remove makes
> > that the physical is actually disconnected, so the function will fail,
> > and I'm not sure what is happening then.
>
> It's not too hard to change this if we're sure we want to. I could
> change how the panel follower API works, though I'd rather keep it how
> it is now for symmetry. Thus, if we want to do this I'd probably just
> set a boolean at the beginning of i2c_hid_core_remove() to avoid the
> suspend when the panel follower API calls us back.
I was more thinking on a boolean. No need to overload the API.
>
> That being said, are you sure you want me to do that?
>
> 1. My patch doesn't change the behavior of any existing hardware. It
> will only do anything for hardware that indicates it needs the panel
> follower logic. Presumably these people could confirm that the logic
> is OK for them, though I'll also admit that it's likely not many of
> them will test the remove() case.
Isn't trogdor (patch 10/10) already supported? Though you should be the
one making tests, so it should be fine ;)
>
> 2. Can you give more details about why you say that the function will
> fail? The first thing that the remove() function will do is to
> unfollow the panel and that can cause the suspend to happen. At the
> time this code runs all the normal communications should work and so
> there should be no problems calling into the suspend code.
Now that I think about it more, maybe I am too biased by USB where the
device remove would happened *after* the device has been physically
unplugged. And this doesn't apply of course in the I2C world.
>
> 3. You can correct me if I'm wrong, but I'd actually argue that
> calling the suspend code during remove actually fixes issues and we
> should probably do it for the non-panel-follower case as well. I think
> there are at least two benefits. One benefit is that if the i2c-hid
> device is on a power rail that can't turn off (either an always-on or
> a shared power rail) that we'll at least get the device in a low power
> state before we stop managing it with this driver. The second benefit
> is that it implicitly disables the interrupt and that fixes a
> potential crash at remove time(). The crash in the old code I'm
> imagining is:
>
> a) i2c_hid_core_remove() is called.
>
> b) We try to power down the i2c hid device, which might not do
> anything if the device is on an always-on rail.
>
> c) We call hid_destroy_device(), which frees the hid device.
>
> d) An interrupt comes in before the call to free_irq() and we try to
> dispatch it to the already freed hid device and crash.
>
>
> If you agree that my reasoning makes sense, I can add a separate patch
> before this one to suspend during remove.
Yep, I agree with you :)
Adding a separate patch would be nice, yes. Thanks!
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH v3 08/10] HID: i2c-hid: Support being a panel follower
From: Doug Anderson @ 2023-07-26 16:07 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, devicetree, cros-qcom-dts-watchers,
linux-arm-msm, yangcong5, Dmitry Torokhov, linux-kernel,
dri-devel, Chris Morgan, linux-input, hsinyi
In-Reply-To: <rorhwk3jx72twmqnxqb45uhm7azxxfirvferwyznbhbfmdf7ja@6k6ebhehmsn4>
Hi,
On Wed, Jul 26, 2023 at 1:57 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> > @@ -1143,7 +1208,14 @@ void i2c_hid_core_remove(struct i2c_client *client)
> > struct i2c_hid *ihid = i2c_get_clientdata(client);
> > struct hid_device *hid;
> >
> > - i2c_hid_core_power_down(ihid);
> > + /*
> > + * If we're a follower, the act of unfollowing will cause us to be
> > + * powered down. Otherwise we need to manually do it.
> > + */
> > + if (ihid->is_panel_follower)
> > + drm_panel_remove_follower(&ihid->panel_follower);
>
> That part is concerning, as we are now calling hid_drv->suspend() when removing
> the device. It might or not have an impact (I'm not sure of it), but we
> are effectively changing the path of commands sent to the device.
>
> hid-multitouch might call a feature in ->suspend, but the remove makes
> that the physical is actually disconnected, so the function will fail,
> and I'm not sure what is happening then.
It's not too hard to change this if we're sure we want to. I could
change how the panel follower API works, though I'd rather keep it how
it is now for symmetry. Thus, if we want to do this I'd probably just
set a boolean at the beginning of i2c_hid_core_remove() to avoid the
suspend when the panel follower API calls us back.
That being said, are you sure you want me to do that?
1. My patch doesn't change the behavior of any existing hardware. It
will only do anything for hardware that indicates it needs the panel
follower logic. Presumably these people could confirm that the logic
is OK for them, though I'll also admit that it's likely not many of
them will test the remove() case.
2. Can you give more details about why you say that the function will
fail? The first thing that the remove() function will do is to
unfollow the panel and that can cause the suspend to happen. At the
time this code runs all the normal communications should work and so
there should be no problems calling into the suspend code.
3. You can correct me if I'm wrong, but I'd actually argue that
calling the suspend code during remove actually fixes issues and we
should probably do it for the non-panel-follower case as well. I think
there are at least two benefits. One benefit is that if the i2c-hid
device is on a power rail that can't turn off (either an always-on or
a shared power rail) that we'll at least get the device in a low power
state before we stop managing it with this driver. The second benefit
is that it implicitly disables the interrupt and that fixes a
potential crash at remove time(). The crash in the old code I'm
imagining is:
a) i2c_hid_core_remove() is called.
b) We try to power down the i2c hid device, which might not do
anything if the device is on an always-on rail.
c) We call hid_destroy_device(), which frees the hid device.
d) An interrupt comes in before the call to free_irq() and we try to
dispatch it to the already freed hid device and crash.
If you agree that my reasoning makes sense, I can add a separate patch
before this one to suspend during remove.
-Doug
^ permalink raw reply
* Re: [PATCH v3 02/10] drm/panel: Check for already prepared/enabled in drm_panel
From: Doug Anderson @ 2023-07-26 15:10 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, cros-qcom-dts-watchers, Chris Morgan,
linux-input, hsinyi, linux-arm-msm, dri-devel, linux-kernel,
Dmitry Torokhov, devicetree, Daniel Vetter, yangcong5
In-Reply-To: <snx3fzvf3icauri2xuigydvpqxtzhp34mptdxvifi7jswm2evy@sx7jr7zwvjw5>
Hi,
On Wed, Jul 26, 2023 at 5:41 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> > NOTE: arguably, the right thing to do here is actually to skip this
> > patch and simply remove all the extra checks from the individual
> > drivers. Perhaps the checks were needed at some point in time in the
> > past but maybe they no longer are? Certainly as we continue
> > transitioning over to "panel_bridge" then we expect there to be much
> > less variety in how these calls are made. When we're called as part of
> > the bridge chain, things should be pretty simple. In fact, there was
> > some discussion in the past about these checks [1], including a
> > discussion about whether the checks were needed and whether the calls
> > ought to be refcounted. At the time, I decided not to mess with it
> > because it felt too risky.
>
> Yeah, I'd agree here too. I've never found evidence that it was actually
> needed and it really looks like cargo cult to me.
>
> And if it was needed, then I'm not sure we need refcounting either. We
> don't have refcounting for atomic_enable / disable, we have a sound API
> design that makes sure we don't fall into that trap :)
>
> > Looking closer at it now, I'm fairly certain that nothing in the
> > existing codebase is expecting these calls to be refcounted. The only
> > real question is whether someone is already doing something to ensure
> > prepare()/unprepare() match and enabled()/disable() match. I would say
> > that, even if there is something else ensuring that things match,
> > there's enough complexity that adding an extra bool and an extra
> > double-check here is a good idea. Let's add a drm_warn() to let people
> > know that it's considered a minor error to take advantage of
> > drm_panel's double-checking but we'll still make things work fine.
>
> I'm ok with this, if we follow-up in a couple of releases and remove it
> and all the calls.
>
> Could you add a TODO item so that we can keep a track of it? A follow-up
> is fine if you don't send a new version of that series.
By this, I think you mean to add a "TODO" comment inline in the code?
Also: I was thinking that we'd keep the check in "drm_panel.c" with
the warning message indefinitely. You think it should be eventually
removed? If we are truly thinking of removing it eventually, this
feels like it should be a more serious warning message like a WARN(1,
...) to make it really obvious to people that they're relying on
behavior that will eventually go away.
-Doug
^ permalink raw reply
* Re: [PATCH v3 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Maxime Ripard @ 2023-07-26 12:45 UTC (permalink / raw)
To: Douglas Anderson
Cc: cros-qcom-dts-watchers, devicetree, dri-devel, hsinyi,
linux-arm-msm, linux-input, linux-kernel, yangcong5,
Benjamin Tissoires, Bjorn Andersson, Chris Morgan, Conor Dooley,
Daniel Vetter, Dmitry Torokhov, Frank Rowand, Jiri Kosina,
Konrad Dybcio, Krzysztof Kozlowski, Maarten Lankhorst,
Maxime Ripard, Neil Armstrong, Rob Herring, Sam Ravnborg,
Thomas Zimmermann
In-Reply-To: <20230725203545.2260506-1-dianders@chromium.org>
On Tue, 25 Jul 2023 13:34:35 -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
>
> [ ... ]
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Thanks!
Maxime
^ permalink raw reply
* Re: [PATCH v3 02/10] drm/panel: Check for already prepared/enabled in drm_panel
From: Maxime Ripard @ 2023-07-26 12:41 UTC (permalink / raw)
To: Douglas Anderson
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, cros-qcom-dts-watchers, Chris Morgan,
linux-input, hsinyi, linux-arm-msm, dri-devel, linux-kernel,
Dmitry Torokhov, devicetree, Daniel Vetter, yangcong5
In-Reply-To: <20230725133443.v3.2.I59b417d4c29151cc2eff053369ec4822b606f375@changeid>
[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]
Hi,
On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> NOTE: arguably, the right thing to do here is actually to skip this
> patch and simply remove all the extra checks from the individual
> drivers. Perhaps the checks were needed at some point in time in the
> past but maybe they no longer are? Certainly as we continue
> transitioning over to "panel_bridge" then we expect there to be much
> less variety in how these calls are made. When we're called as part of
> the bridge chain, things should be pretty simple. In fact, there was
> some discussion in the past about these checks [1], including a
> discussion about whether the checks were needed and whether the calls
> ought to be refcounted. At the time, I decided not to mess with it
> because it felt too risky.
Yeah, I'd agree here too. I've never found evidence that it was actually
needed and it really looks like cargo cult to me.
And if it was needed, then I'm not sure we need refcounting either. We
don't have refcounting for atomic_enable / disable, we have a sound API
design that makes sure we don't fall into that trap :)
> Looking closer at it now, I'm fairly certain that nothing in the
> existing codebase is expecting these calls to be refcounted. The only
> real question is whether someone is already doing something to ensure
> prepare()/unprepare() match and enabled()/disable() match. I would say
> that, even if there is something else ensuring that things match,
> there's enough complexity that adding an extra bool and an extra
> double-check here is a good idea. Let's add a drm_warn() to let people
> know that it's considered a minor error to take advantage of
> drm_panel's double-checking but we'll still make things work fine.
I'm ok with this, if we follow-up in a couple of releases and remove it
and all the calls.
Could you add a TODO item so that we can keep a track of it? A follow-up
is fine if you don't send a new version of that series.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox