[0/9] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together

  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(-)
  

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
  

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(-)
>