| Message ID | 20230523193017.4109557-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together | expand |
Hi Doug, On Tue, May 23, 2023 at 12:27:54PM -0700, Douglas Anderson wrote: > > The big motivation for this patch series is mostly described in the patch > ("drm/panel: Add a way for other devices to follow panel state"), but to > quickly summarize here: for touchscreens that are connected to a panel we > need the ability to power sequence the two device together. This is not a > new need, but so far we've managed to get by through a combination of > inefficiency, added costs, or perhaps just a little bit of brokenness. > It's time to do better. This patch series allows us to do better. This seems to grow a new way of building relationship between panels and associated devices. Can we make device_link_*() work for us? Thanks.
Hi, On Tue, May 23, 2023 at 2:39 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Hi Doug, > > On Tue, May 23, 2023 at 12:27:54PM -0700, Douglas Anderson wrote: > > > > The big motivation for this patch series is mostly described in the patch > > ("drm/panel: Add a way for other devices to follow panel state"), but to > > quickly summarize here: for touchscreens that are connected to a panel we > > need the ability to power sequence the two device together. This is not a > > new need, but so far we've managed to get by through a combination of > > inefficiency, added costs, or perhaps just a little bit of brokenness. > > It's time to do better. This patch series allows us to do better. > > This seems to grow a new way of building relationship between panels and > associated devices. Can we make device_link_*() work for us? If you know of a way to make it work, that'd be great. ...but I don't _think_ it would be that easy. I haven't spent much time with the device_link APIs, though, so please correct me if I'm wrong. I guess my main issue with device_link would be that that ordering feels backward. Specifically, the device we're acting on (the one we're turning off and on) is the panel. We typically turn the panel off and on at times (during a modeset, when the lid is closed, or just if the system is idle). When this happens we'd like to remove power from both the panel and the touchscreen. Userspace is just not setup to power off touchscreens in tandem with the panel and sometimes (like in the case of a modeset) it might not even know it needs to. With device_link I believe that the "child" device is in charge of powering the parent. I think that would mean that to use device_link we'd need to make the panel be the child of the touchscreen. Then, I guess we'd tell the touchscreen not to power itself on if it had children and just wait for a child to power it on? It feels really awkward partly because the panel doesn't feel like it should be a child of the touchscreen and partially because it seems weird that the touchscreen would somehow need to know not to request power for itself when it has a child. ...if we're willing to accept the backwardness as described above and we can find a hack to keep the touchscreen from powering itself up, then I think things could be made to work OK-ish. I can try to investigate that route if it doesn't feel too ugly. ...oh, except for another problem. The initial power up of the i2c-hid device would also be a problem here. I guess with device_link we'd need to put all the power up work into "runtime resume". The problem is that upon initial power up we create "HID" sub-devices and (as far as I could tell) you can't do that during a runtime resume. :( We could put a special case to power the touchscreen up before the panel at probe time, but that could have other consequences? If we don't go with the backwardness then I think we're a bit stuck with some of the original problems. Specifically it means that unless userspace knows to turn off the touchscreen that the touchscreen would force the panel to never be power cycled. There's at least one panel (the one on sc7180-trogdor-homestar) where that's known to be a problem. It also means that we're back to drawing extra power if the touchscreen is left "on" but the panel power is turned off. Let me know if I'm misunderstanding. -Doug
Hi, On 23/05/2023 21:27, Douglas Anderson wrote: > In a whole pile of panel drivers, we have code to make the > prepare/unprepare/enable/disable callbacks behave as no-ops if they've > already been called. It's silly to have this code duplicated > everywhere. Add it to the core instead so that we can eventually > delete it from all the drivers. Note: to get some idea of the > duplicated code, try: > git grep 'if.*>prepared' -- drivers/gpu/drm/panel > git grep 'if.*>enabled' -- drivers/gpu/drm/panel > > NOTE: arguably, the right thing to do here is actually to skip this > patch and simply remove all the extra checks from the individual > drivers. Perhaps the checks were needed at some point in time in the > past but maybe they no longer are? Certainly as we continue > transitioning over to "panel_bridge" then we expect there to be much > less variety in how these calls are made. When we're called as part of > the bridge chain, things should be pretty simple. In fact, there was > some discussion in the past about these checks [1], including a > discussion about whether the checks were needed and whether the calls > ought to be refcounted. At the time, I decided not to mess with it > because it felt too risky. > > Looking closer at it now, I'm fairly certain that nothing in the > existing codebase is expecting these calls to be refcounted. The only > real question is whether someone is already doing something to ensure > prepare()/unprepare() match and enabled()/disable() match. I would say > that, even if there is something else ensuring that things match, > there's enough complexity that adding an extra bool and an extra > double-check here is a good idea. Let's add a drm_warn() to let people > know that it's considered a minor error to take advantage of > drm_panel's double-checking but we'll still make things work fine. > > [1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/drm_panel.c | 49 ++++++++++++++++++++++++++++++++----- > include/drm/drm_panel.h | 14 +++++++++++ > 2 files changed, 57 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index f634371c717a..4e1c4e42575b 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -105,11 +105,22 @@ EXPORT_SYMBOL(drm_panel_remove); > */ > int drm_panel_prepare(struct drm_panel *panel) > { > + int ret; > + > if (!panel) > return -EINVAL; > > - if (panel->funcs && panel->funcs->prepare) > - return panel->funcs->prepare(panel); > + if (panel->prepared) { > + dev_warn(panel->dev, "Skipping prepare of already prepared panel\n"); > + return 0; > + } > + > + if (panel->funcs && panel->funcs->prepare) { > + ret = panel->funcs->prepare(panel); > + if (ret < 0) > + return ret; > + } > + panel->prepared = true; > > return 0; > } > @@ -128,11 +139,22 @@ EXPORT_SYMBOL(drm_panel_prepare); > */ > int drm_panel_unprepare(struct drm_panel *panel) > { > + int ret; > + > if (!panel) > return -EINVAL; > > - if (panel->funcs && panel->funcs->unprepare) > - return panel->funcs->unprepare(panel); > + if (!panel->prepared) { > + dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n"); > + return 0; > + } > + > + if (panel->funcs && panel->funcs->unprepare) { > + ret = panel->funcs->unprepare(panel); > + if (ret < 0) > + return ret; > + } > + panel->prepared = false; > > return 0; > } > @@ -155,11 +177,17 @@ int drm_panel_enable(struct drm_panel *panel) > if (!panel) > return -EINVAL; > > + if (panel->enabled) { > + dev_warn(panel->dev, "Skipping enable of already enabled panel\n"); > + return 0; > + } > + > if (panel->funcs && panel->funcs->enable) { > ret = panel->funcs->enable(panel); > if (ret < 0) > return ret; > } > + panel->enabled = true; > > ret = backlight_enable(panel->backlight); > if (ret < 0) > @@ -187,13 +215,22 @@ int drm_panel_disable(struct drm_panel *panel) > if (!panel) > return -EINVAL; > > + if (!panel->enabled) { > + dev_warn(panel->dev, "Skipping disable of already disabled panel\n"); > + return 0; > + } > + > ret = backlight_disable(panel->backlight); > if (ret < 0) > DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n", > ret); > > - if (panel->funcs && panel->funcs->disable) > - return panel->funcs->disable(panel); > + if (panel->funcs && panel->funcs->disable) { > + ret = panel->funcs->disable(panel); > + if (ret < 0) > + return ret; > + } > + panel->enabled = false; > > return 0; > } > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index 432fab2347eb..c6cf75909389 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -198,6 +198,20 @@ struct drm_panel { > * the panel is powered up. > */ > bool prepare_prev_first; > + > + /** > + * @prepared: > + * > + * If true then the panel has been prepared. > + */ > + bool prepared; > + > + /** > + * @enabled: > + * > + * If true then the panel has been enabled. > + */ > + bool enabled; > }; > > void drm_panel_init(struct drm_panel *panel, struct device *dev, LGTM and let's cleanup the panel drivers Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
On Tue, May 23, 2023 at 12:27:56PM -0700, Douglas Anderson wrote: > 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 For some reason I haven't dug into in greater detail on my RK3326 and RK3568 boards I hit an issue on suspend/shutdown whereby the unprepare is called multiple times. I suspect it's the dw-mipi-dsi-rockchip.c driver and the dw-mipi-dsi.c drivers both calling the unprepare, but I haven't been able to debug it completely yet. > 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 Regulator unbalanced disables are a bane of my existence. For the panel-newvision-nv3051d.c driver I upstreamed a few releases ago I was told to not include the is_enabled logic and as a result I get a warning on suspend or shutdown when it disables the panel. Unprepare gets called twice and that results in an unbalanced regulator disable. > real question is whether someone is already doing something to ensure > prepare()/unprepare() match and enabled()/disable() match. I would say > that, even if there is something else ensuring that things match, > there's enough complexity that adding an extra bool and an extra > double-check here is a good idea. Let's add a drm_warn() to let people > know that it's considered a minor error to take advantage of > drm_panel's double-checking but we'll still make things work fine. > > [1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/drm_panel.c | 49 ++++++++++++++++++++++++++++++++----- > include/drm/drm_panel.h | 14 +++++++++++ > 2 files changed, 57 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index f634371c717a..4e1c4e42575b 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -105,11 +105,22 @@ EXPORT_SYMBOL(drm_panel_remove); > */ > int drm_panel_prepare(struct drm_panel *panel) > { > + int ret; > + > if (!panel) > return -EINVAL; > > - if (panel->funcs && panel->funcs->prepare) > - return panel->funcs->prepare(panel); > + if (panel->prepared) { > + dev_warn(panel->dev, "Skipping prepare of already prepared panel\n"); > + return 0; > + } > + > + if (panel->funcs && panel->funcs->prepare) { > + ret = panel->funcs->prepare(panel); > + if (ret < 0) > + return ret; > + } > + panel->prepared = true; > > return 0; > } > @@ -128,11 +139,22 @@ EXPORT_SYMBOL(drm_panel_prepare); > */ > int drm_panel_unprepare(struct drm_panel *panel) > { > + int ret; > + > if (!panel) > return -EINVAL; > > - if (panel->funcs && panel->funcs->unprepare) > - return panel->funcs->unprepare(panel); > + if (!panel->prepared) { > + dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n"); > + return 0; > + } > + > + if (panel->funcs && panel->funcs->unprepare) { > + ret = panel->funcs->unprepare(panel); > + if (ret < 0) > + return ret; > + } > + panel->prepared = false; > > return 0; > } > @@ -155,11 +177,17 @@ int drm_panel_enable(struct drm_panel *panel) > if (!panel) > return -EINVAL; > > + if (panel->enabled) { > + dev_warn(panel->dev, "Skipping enable of already enabled panel\n"); > + return 0; > + } > + > if (panel->funcs && panel->funcs->enable) { > ret = panel->funcs->enable(panel); > if (ret < 0) > return ret; > } > + panel->enabled = true; > > ret = backlight_enable(panel->backlight); > if (ret < 0) > @@ -187,13 +215,22 @@ int drm_panel_disable(struct drm_panel *panel) > if (!panel) > return -EINVAL; > > + if (!panel->enabled) { > + dev_warn(panel->dev, "Skipping disable of already disabled panel\n"); > + return 0; > + } > + > ret = backlight_disable(panel->backlight); > if (ret < 0) > DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n", > ret); > > - if (panel->funcs && panel->funcs->disable) > - return panel->funcs->disable(panel); > + if (panel->funcs && panel->funcs->disable) { > + ret = panel->funcs->disable(panel); > + if (ret < 0) > + return ret; > + } > + panel->enabled = false; > > return 0; > } > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index 432fab2347eb..c6cf75909389 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -198,6 +198,20 @@ struct drm_panel { > * the panel is powered up. > */ > bool prepare_prev_first; > + > + /** > + * @prepared: > + * > + * If true then the panel has been prepared. > + */ > + bool prepared; > + > + /** > + * @enabled: > + * > + * If true then the panel has been enabled. > + */ > + bool enabled; > }; > > void drm_panel_init(struct drm_panel *panel, struct device *dev, > -- > 2.40.1.698.g37aff9b760-goog > Thank you for looking into this more. It's one of the last QOL bugs for some devices I'm working on, even if the end result is no big deal (the other QOL bug involves a WARN on probing a rotated panel).
Hi, On Wed, May 24, 2023 at 2:52 AM Neil Armstrong <neil.armstrong@linaro.org> wrote: > > Hi, > > On 23/05/2023 21:27, Douglas Anderson wrote: > > In a whole pile of panel drivers, we have code to make the > > prepare/unprepare/enable/disable callbacks behave as no-ops if they've > > already been called. It's silly to have this code duplicated > > everywhere. Add it to the core instead so that we can eventually > > delete it from all the drivers. Note: to get some idea of the > > duplicated code, try: > > git grep 'if.*>prepared' -- drivers/gpu/drm/panel > > git grep 'if.*>enabled' -- drivers/gpu/drm/panel > > > > NOTE: arguably, the right thing to do here is actually to skip this > > patch and simply remove all the extra checks from the individual > > drivers. Perhaps the checks were needed at some point in time in the > > past but maybe they no longer are? Certainly as we continue > > transitioning over to "panel_bridge" then we expect there to be much > > less variety in how these calls are made. When we're called as part of > > the bridge chain, things should be pretty simple. In fact, there was > > some discussion in the past about these checks [1], including a > > discussion about whether the checks were needed and whether the calls > > ought to be refcounted. At the time, I decided not to mess with it > > because it felt too risky. > > > > Looking closer at it now, I'm fairly certain that nothing in the > > existing codebase is expecting these calls to be refcounted. The only > > real question is whether someone is already doing something to ensure > > prepare()/unprepare() match and enabled()/disable() match. I would say > > that, even if there is something else ensuring that things match, > > there's enough complexity that adding an extra bool and an extra > > double-check here is a good idea. Let's add a drm_warn() to let people > > know that it's considered a minor error to take advantage of > > drm_panel's double-checking but we'll still make things work fine. > > > > [1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/gpu/drm/drm_panel.c | 49 ++++++++++++++++++++++++++++++++----- > > include/drm/drm_panel.h | 14 +++++++++++ > > 2 files changed, 57 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > index f634371c717a..4e1c4e42575b 100644 > > --- a/drivers/gpu/drm/drm_panel.c > > +++ b/drivers/gpu/drm/drm_panel.c > > @@ -105,11 +105,22 @@ EXPORT_SYMBOL(drm_panel_remove); > > */ > > int drm_panel_prepare(struct drm_panel *panel) > > { > > + int ret; > > + > > if (!panel) > > return -EINVAL; > > > > - if (panel->funcs && panel->funcs->prepare) > > - return panel->funcs->prepare(panel); > > + if (panel->prepared) { > > + dev_warn(panel->dev, "Skipping prepare of already prepared panel\n"); > > + return 0; > > + } > > + > > + if (panel->funcs && panel->funcs->prepare) { > > + ret = panel->funcs->prepare(panel); > > + if (ret < 0) > > + return ret; > > + } > > + panel->prepared = true; > > > > return 0; > > } > > @@ -128,11 +139,22 @@ EXPORT_SYMBOL(drm_panel_prepare); > > */ > > int drm_panel_unprepare(struct drm_panel *panel) > > { > > + int ret; > > + > > if (!panel) > > return -EINVAL; > > > > - if (panel->funcs && panel->funcs->unprepare) > > - return panel->funcs->unprepare(panel); > > + if (!panel->prepared) { > > + dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n"); > > + return 0; > > + } > > + > > + if (panel->funcs && panel->funcs->unprepare) { > > + ret = panel->funcs->unprepare(panel); > > + if (ret < 0) > > + return ret; > > + } > > + panel->prepared = false; > > > > return 0; > > } > > @@ -155,11 +177,17 @@ int drm_panel_enable(struct drm_panel *panel) > > if (!panel) > > return -EINVAL; > > > > + if (panel->enabled) { > > + dev_warn(panel->dev, "Skipping enable of already enabled panel\n"); > > + return 0; > > + } > > + > > if (panel->funcs && panel->funcs->enable) { > > ret = panel->funcs->enable(panel); > > if (ret < 0) > > return ret; > > } > > + panel->enabled = true; > > > > ret = backlight_enable(panel->backlight); > > if (ret < 0) > > @@ -187,13 +215,22 @@ int drm_panel_disable(struct drm_panel *panel) > > if (!panel) > > return -EINVAL; > > > > + if (!panel->enabled) { > > + dev_warn(panel->dev, "Skipping disable of already disabled panel\n"); > > + return 0; > > + } > > + > > ret = backlight_disable(panel->backlight); > > if (ret < 0) > > DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n", > > ret); > > > > - if (panel->funcs && panel->funcs->disable) > > - return panel->funcs->disable(panel); > > + if (panel->funcs && panel->funcs->disable) { > > + ret = panel->funcs->disable(panel); > > + if (ret < 0) > > + return ret; > > + } > > + panel->enabled = false; > > > > return 0; > > } > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > > index 432fab2347eb..c6cf75909389 100644 > > --- a/include/drm/drm_panel.h > > +++ b/include/drm/drm_panel.h > > @@ -198,6 +198,20 @@ struct drm_panel { > > * the panel is powered up. > > */ > > bool prepare_prev_first; > > + > > + /** > > + * @prepared: > > + * > > + * If true then the panel has been prepared. > > + */ > > + bool prepared; > > + > > + /** > > + * @enabled: > > + * > > + * If true then the panel has been enabled. > > + */ > > + bool enabled; > > }; > > > > void drm_panel_init(struct drm_panel *panel, struct device *dev, > > LGTM and let's cleanup the panel drivers > > Acked-by: Neil Armstrong <neil.armstrong@linaro.org> Thanks! For now I'll hold off on landing to see where this series ends up. If the series ends up looking good we'll have to coordinate landing the various bits between the drm and the hid trees and the second drm patch in my series depends on this one> If my series implodes I'll land this one on its own with your Ack. In any case, once this lands somewhere I'll take an AI to cleanup the panels. -Doug
Hi, On Wed, May 24, 2023 at 9:19 AM Chris Morgan <macroalpha82@gmail.com> wrote: > > Thank you for looking into this more. It's one of the last QOL bugs > for some devices I'm working on, even if the end result is no big > deal (the other QOL bug involves a WARN on probing a rotated panel). Glad it'll be helpful! For the WARN on probing a rotated panel I thought there was some progress on that front. Commit e3ea1806e4ad ("drm/bridge: panel: Set orientation on panel_bridge connector") or commit 15b9ca1641f0 ("drm: Config orientation property if panel provides it") didn't help you?
Hi, On Tue, May 23, 2023 at 12:31 PM Douglas Anderson <dianders@chromium.org> wrote: > > As talked about in the patch ("drm/panel: Add a way for other devices > to follow panel state"), we really want to keep the power states of a > touchscreen and the panel it's attached to in sync with each other. In > that spirit, add support to i2c-hid to be a panel follower. This will > let the i2c-hid driver get informed when the panel is powered on and > off. From there we can match the i2c-hid device's power state to that > of the panel. > > NOTE: this patch specifically _doesn't_ use pm_runtime to keep track > of / manage the power state of the i2c-hid device, even though my > first instinct said that would be the way to go. Specific problems > with using pm_runtime(): > * The initial power up couldn't happen in a runtime resume function > since it create sub-devices and, apparently, that's not good to do > in your resume function. > * Managing our power state with pm_runtime meant fighting to make the > right thing happen at system suspend to prevent the system from > trying to resume us only to suspend us again. While this might be > able to be solved, it added complexity. > Overall the code without pm_runtime() ended up being smaller and > easier to understand. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/hid/i2c-hid/i2c-hid-core.c | 82 +++++++++++++++++++++++++++++- > 1 file changed, 81 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index 34c0d98b4976..f1bb89377e8d 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -38,6 +38,8 @@ > #include <linux/mutex.h> > #include <asm/unaligned.h> > > +#include <drm/drm_panel.h> > + > #include "../hid-ids.h" > #include "i2c-hid.h" > > @@ -107,6 +109,8 @@ struct i2c_hid { > struct mutex reset_lock; > > struct i2chid_ops *ops; > + struct drm_panel_follower panel_follower; > + bool is_panel_follower; > }; > > static const struct i2c_hid_quirks { > @@ -1058,6 +1062,34 @@ int i2c_hid_core_initial_power_up(struct i2c_hid *ihid) > return ret; > } > > +int i2c_hid_core_panel_prepared(struct drm_panel_follower *follower) As pointed out by the kernel test robot, I clearly missed making several functions "static" in this patch series. :( I'll fix that in v2, but for now I'll hold off sending v2 to wait for additional feedback. -Doug
On 23.05.2023 21:27, Douglas Anderson wrote: > > The big motivation for this patch series is mostly described in the patch > ("drm/panel: Add a way for other devices to follow panel state"), but to > quickly summarize here: for touchscreens that are connected to a panel we > need the ability to power sequence the two device together. This is not a > new need, but so far we've managed to get by through a combination of > inefficiency, added costs, or perhaps just a little bit of brokenness. > It's time to do better. This patch series allows us to do better. Panels with integrated touchscreens have been shipping in mainstream devices since at least 2016. Thanks a lot for looking into this! Konrad > > Assuming that people think this patch series looks OK, we'll have to > figure out the right way to land it. The panel patches and i2c-hid > patches will go through very different trees and so either we'll need > an Ack from one side or the other or someone to create a tag for the > other tree to pull in. This will _probably_ require the true drm-misc > maintainers to get involved, not a lowly committer. ;-) > > > Douglas Anderson (9): > dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed > panels > drm/panel: Check for already prepared/enabled in drm_panel > drm/panel: Add a way for other devices to follow panel state > HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() > HID: i2c-hid: Rearrange probe() to power things up later > HID: i2c-hid: Make suspend and resume into helper functions > HID: i2c-hid: Support being a panel follower > HID: i2c-hid: Do panel follower work on the system_wq > arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels > > .../bindings/input/elan,ekth6915.yaml | 6 + > .../bindings/input/goodix,gt7375p.yaml | 6 + > .../bindings/input/hid-over-i2c.yaml | 6 + > .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 1 + > .../dts/qcom/sc7180-trogdor-homestar.dtsi | 1 + > .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi | 1 + > .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi | 1 + > .../qcom/sc7180-trogdor-quackingstick.dtsi | 1 + > .../dts/qcom/sc7180-trogdor-wormdingler.dtsi | 1 + > drivers/gpu/drm/drm_panel.c | 194 +++++++++- > drivers/hid/i2c-hid/i2c-hid-core.c | 330 +++++++++++++----- > include/drm/drm_panel.h | 89 +++++ > 12 files changed, 542 insertions(+), 95 deletions(-) >
The big motivation for this patch series is mostly described in the patch ("drm/panel: Add a way for other devices to follow panel state"), but to quickly summarize here: for touchscreens that are connected to a panel we need the ability to power sequence the two device together. This is not a new need, but so far we've managed to get by through a combination of inefficiency, added costs, or perhaps just a little bit of brokenness. It's time to do better. This patch series allows us to do better. Assuming that people think this patch series looks OK, we'll have to figure out the right way to land it. The panel patches and i2c-hid patches will go through very different trees and so either we'll need an Ack from one side or the other or someone to create a tag for the other tree to pull in. This will _probably_ require the true drm-misc maintainers to get involved, not a lowly committer. ;-) Douglas Anderson (9): dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed panels drm/panel: Check for already prepared/enabled in drm_panel drm/panel: Add a way for other devices to follow panel state HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() HID: i2c-hid: Rearrange probe() to power things up later HID: i2c-hid: Make suspend and resume into helper functions HID: i2c-hid: Support being a panel follower HID: i2c-hid: Do panel follower work on the system_wq arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels .../bindings/input/elan,ekth6915.yaml | 6 + .../bindings/input/goodix,gt7375p.yaml | 6 + .../bindings/input/hid-over-i2c.yaml | 6 + .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 1 + .../dts/qcom/sc7180-trogdor-homestar.dtsi | 1 + .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi | 1 + .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi | 1 + .../qcom/sc7180-trogdor-quackingstick.dtsi | 1 + .../dts/qcom/sc7180-trogdor-wormdingler.dtsi | 1 + drivers/gpu/drm/drm_panel.c | 194 +++++++++- drivers/hid/i2c-hid/i2c-hid-core.c | 330 +++++++++++++----- include/drm/drm_panel.h | 89 +++++ 12 files changed, 542 insertions(+), 95 deletions(-)