| Message ID | 20230607215224.2067679-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together | expand |
Hi Douglas, On Wed, Jun 07, 2023 at 02:49:22PM -0700, Douglas Anderson wrote: > > The big motivation for this patch series is mostly described in the patch > ("drm/panel: Add a way for other devices to follow panel state"), but to > quickly summarize here: for touchscreens that are connected to a panel we > need the ability to power sequence the two device together. This is not a > new need, but so far we've managed to get by through a combination of > inefficiency, added costs, or perhaps just a little bit of brokenness. > It's time to do better. This patch series allows us to do better. > > Assuming that people think this patch series looks OK, we'll have to > figure out the right way to land it. The panel patches and i2c-hid > patches will go through very different trees and so either we'll need > an Ack from one side or the other or someone to create a tag for the > other tree to pull in. This will _probably_ require the true drm-misc > maintainers to get involved, not a lowly committer. ;-) > > Version 2 of this patch series doesn't change too much. At a high level: > * I added all the forgotten "static" to functions. > * I've hopefully made the bindings better. > * I've integrated into fw_devlink. > * I cleaned up a few descriptions / comments. > > This still needs someone to say that the idea looks OK or to suggest > an alternative that solves the problems. ;-) Thanks for working on this. I haven't seen in any of your commit messages how the panels were actually "packaged" together? Do a panel model typically come together with the i2c-hid support, or is it added at manufacture time? If it's the latter, it's indeed a fairly loose connection and we need your work. If it's the former though and we don't expect a given panel reference to always (or never) come with a touchscreen attached, I guess we can have something much simpler with a bunch of helpers that would register a i2c-hid device and would be called by the panel driver itself. And then, since everything is self-contained managing the power state becomes easier as well. Maxime
Hi, On Thu, Jun 8, 2023 at 12:17 AM Maxime Ripard <mripard@kernel.org> wrote: > > Hi Douglas, > > On Wed, Jun 07, 2023 at 02:49:22PM -0700, Douglas Anderson wrote: > > > > The big motivation for this patch series is mostly described in the patch > > ("drm/panel: Add a way for other devices to follow panel state"), but to > > quickly summarize here: for touchscreens that are connected to a panel we > > need the ability to power sequence the two device together. This is not a > > new need, but so far we've managed to get by through a combination of > > inefficiency, added costs, or perhaps just a little bit of brokenness. > > It's time to do better. This patch series allows us to do better. > > > > Assuming that people think this patch series looks OK, we'll have to > > figure out the right way to land it. The panel patches and i2c-hid > > patches will go through very different trees and so either we'll need > > an Ack from one side or the other or someone to create a tag for the > > other tree to pull in. This will _probably_ require the true drm-misc > > maintainers to get involved, not a lowly committer. ;-) > > > > Version 2 of this patch series doesn't change too much. At a high level: > > * I added all the forgotten "static" to functions. > > * I've hopefully made the bindings better. > > * I've integrated into fw_devlink. > > * I cleaned up a few descriptions / comments. > > > > This still needs someone to say that the idea looks OK or to suggest > > an alternative that solves the problems. ;-) > > Thanks for working on this. > > I haven't seen in any of your commit messages how the panels were > actually "packaged" together? > > Do a panel model typically come together with the i2c-hid support, or is > it added at manufacture time? > > If it's the latter, it's indeed a fairly loose connection and we need > your work. > > If it's the former though and we don't expect a given panel reference to > always (or never) come with a touchscreen attached, Thanks for your reply. Let me see what I can do to bring clarity. In at least some of the cases, I believe that the panel and the touchscreen _are_ logically distinct components, even if they've been glued together at some stage in manufacturing. Even on one of the "poster child" boards that I talk about in patch #3, the early versions of "homestar", I believe this to be the case. However, even if the panel and touchscreen are separate components then they still could be connected to the main board in a way that they share power and/or reset signals. In my experience, in every case where they do the EEs expect that the panel is power sequenced first and then the touchscreen is power sequenced second. The EEs look at the power sequencing requirements of the panel and touchscreen, see that there is a valid power sequence protocol where they can share rails, and design the board that way. Even if the touchscreen and panel are logically separate, the moment the board designers hook them up to the same power rails and/or reset signals they become tied. This is well supported by my patch series. The case that really motivated my patch series, though, is the case that Cong Yang recently has been working on. I think most of the discussion is in his original patch series [1]. Cong Yang's patch series is largely focused on supporting the "ILI9882T" chip and some panels that it's used with. I found a datasheet for that, and the title from the first page is illustrative: "In-cell IC Integrates TFT LCD Driver and Capacitive Touch Controller into a Two Chip Cascade". This is an integrated solution that's designed to handle both the LCD and the touchscreen. [1] https://lore.kernel.org/lkml/20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com/ > I guess we can have > something much simpler with a bunch of helpers that would register a > i2c-hid device and would be called by the panel driver itself. > > And then, since everything is self-contained managing the power state > becomes easier as well. Can you give me more details about how you think this would work? When you say that the panel would register an i2c-hid device itself, do you mean that we'd do something like give a phandle to the i2c bus to the panel and then the panel would manually instantiate the i2c-hid device on it? ...and I guess it would need to be a "subclass" of i2c-hid that knew about the connection to the panel code? This subclass and the panel code would communicate with each other about power sequencing needs through some private API (like MFD devices usually do?). Assuming I'm understanding correctly, I think that could work. Is it cleaner than my current approach, though? I guess, alternatively, we could put the "panel" directly under the i2c bus in this case. That would probably work for Cong Yang's current needs, but we'd end up in trouble if we ever had a similar situation with an eDP panel since eDP panels need to be under the DP-AUX bus. I guess overall, though, while I think this approach could solve Cong Yang's needs, I still feel like it's worth solving the case where board designers have made panel and touchscreens "coupled" by having them rely on the same power rails and/or reset signals. -Doug
On 07/06/2023 23:49, Douglas Anderson wrote: > As talked about in the patch ("drm/panel: Add a way for other devices > to follow panel state"), touchscreens that are connected to panels are > generally expected to be power sequenced together with the panel > they're attached to. Today, nothing provides information allowing you > to find out that a touchscreen is connected to a panel. Let's add a > phandle for this. > > The proerty is added to the generic touchscreen bindings and then > enabled in the bindings for the i2c-hid backed devices. This can and > should be added for other touchscreens in the future, but for now > let's start small. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v2: > - Move the description to the generic touchscreen.yaml. > - Update the desc to make it clearer it's only for integrated devices. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hi Doug, On Thu, Jun 08, 2023 at 07:38:58AM -0700, Doug Anderson wrote: > On Thu, Jun 8, 2023 at 12:17 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > Hi Douglas, > > > > On Wed, Jun 07, 2023 at 02:49:22PM -0700, Douglas Anderson wrote: > > > > > > The big motivation for this patch series is mostly described in the patch > > > ("drm/panel: Add a way for other devices to follow panel state"), but to > > > quickly summarize here: for touchscreens that are connected to a panel we > > > need the ability to power sequence the two device together. This is not a > > > new need, but so far we've managed to get by through a combination of > > > inefficiency, added costs, or perhaps just a little bit of brokenness. > > > It's time to do better. This patch series allows us to do better. > > > > > > Assuming that people think this patch series looks OK, we'll have to > > > figure out the right way to land it. The panel patches and i2c-hid > > > patches will go through very different trees and so either we'll need > > > an Ack from one side or the other or someone to create a tag for the > > > other tree to pull in. This will _probably_ require the true drm-misc > > > maintainers to get involved, not a lowly committer. ;-) > > > > > > Version 2 of this patch series doesn't change too much. At a high level: > > > * I added all the forgotten "static" to functions. > > > * I've hopefully made the bindings better. > > > * I've integrated into fw_devlink. > > > * I cleaned up a few descriptions / comments. > > > > > > This still needs someone to say that the idea looks OK or to suggest > > > an alternative that solves the problems. ;-) > > > > Thanks for working on this. > > > > I haven't seen in any of your commit messages how the panels were > > actually "packaged" together? > > > > Do a panel model typically come together with the i2c-hid support, or is > > it added at manufacture time? > > > > If it's the latter, it's indeed a fairly loose connection and we need > > your work. > > > > If it's the former though and we don't expect a given panel reference to > > always (or never) come with a touchscreen attached, > > Thanks for your reply. Let me see what I can do to bring clarity. > > In at least some of the cases, I believe that the panel and the > touchscreen _are_ logically distinct components, even if they've been > glued together at some stage in manufacturing. Even on one of the > "poster child" boards that I talk about in patch #3, the early > versions of "homestar", I believe this to be the case. However, even > if the panel and touchscreen are separate components then they still > could be connected to the main board in a way that they share power > and/or reset signals. In my experience, in every case where they do > the EEs expect that the panel is power sequenced first and then the > touchscreen is power sequenced second. The EEs look at the power > sequencing requirements of the panel and touchscreen, see that there > is a valid power sequence protocol where they can share rails, and > design the board that way. Even if the touchscreen and panel are > logically separate, the moment the board designers hook them up to the > same power rails and/or reset signals they become tied. This is well > supported by my patch series. > > The case that really motivated my patch series, though, is the case > that Cong Yang recently has been working on. I think most of the > discussion is in his original patch series [1]. Cong Yang's patch > series is largely focused on supporting the "ILI9882T" chip and some > panels that it's used with. I found a datasheet for that, and the > title from the first page is illustrative: "In-cell IC Integrates TFT > LCD Driver and Capacitive Touch Controller into a Two Chip Cascade". > This is an integrated solution that's designed to handle both the LCD > and the touchscreen. > > [1] https://lore.kernel.org/lkml/20230519032316.3464732-1-yangcong5@huaqin.corp-partner.google.com/ Ok, I think we're on the same page at the hardware level then :) > > I guess we can have > > something much simpler with a bunch of helpers that would register a > > i2c-hid device and would be called by the panel driver itself. > > > > And then, since everything is self-contained managing the power state > > becomes easier as well. > > Can you give me more details about how you think this would work? > > When you say that the panel would register an i2c-hid device itself, > do you mean that we'd do something like give a phandle to the i2c bus > to the panel and then the panel would manually instantiate the i2c-hid > device on it? ...and I guess it would need to be a "subclass" of > i2c-hid that knew about the connection to the panel code? This > subclass and the panel code would communicate with each other about > power sequencing needs through some private API (like MFD devices > usually do?). Assuming I'm understanding correctly, I think that could > work. I guess what I had in mind is to do something similar to what we're doing with hdmi-codec already for example. We have several logical components already, in separate drivers, that still need some cooperation. If the panel and touchscreen are on the same i2c bus, I think we could even just get a reference to the panel i2c adapter, get a reference, and pass that to i2c-hid (with a nice layer of helpers). What I'm trying to say is: could we just make it work by passing a bunch of platform_data, 2-3 callbacks and a device registration from the panel driver directly? > Is it cleaner than my current approach, though? "cleaner" is subjective, really, but it's a more "mainstream" approach that one can follow more easily through function calls. > I guess, alternatively, we could put the "panel" directly under the > i2c bus in this case. That would probably work for Cong Yang's current > needs, but we'd end up in trouble if we ever had a similar situation > with an eDP panel since eDP panels need to be under the DP-AUX bus. I don't know DP-AUX very well, what is the issue that you're mentioning? > I guess overall, though, while I think this approach could solve Cong > Yang's needs, I still feel like it's worth solving the case where > board designers have made panel and touchscreens "coupled" by having > them rely on the same power rails and/or reset signals. Sure, I definitely want that too :) Maxime
On Tue, Jun 13, 2023 at 08:56:39AM -0700, Doug Anderson wrote: > Hi, > > On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > What I'm trying to say is: could we just make it work by passing a bunch > > > > of platform_data, 2-3 callbacks and a device registration from the panel > > > > driver directly? > > > > > > I think I'm still confused about what you're proposing. Sorry! :( Let > > > me try rephrasing why I'm confused and perhaps we can get on the same > > > page. :-) > > > > > > First, I guess I'm confused about how you have one of these devices > > > "register" the other device. > > > > > > I can understand how one device might "register" its sub-devices in > > > the MFD case. To make it concrete, we can look at a PMIC like > > > max77686.c. The parent MFD device gets probed and then it's in charge > > > of creating all of its sub-devices. These sub-devices are intimately > > > tied to one another. They have shared data structures and can > > > coordinate power sequencing and whatnot. All good. > > > > We don't necessarily need to use MFD, but yeah, we could just register a > > device for the i2c-hid driver to probe from (using > > i2c_new_client_device?) > > I think this can work for devices where the panel and touchscreen are > truly integrated where the panel driver knows enough about the related > touchscreen to fully describe and instantiate it. It doesn't work > quite as well for cases where the power and reset lines are shared > just because of what a given board designer did. To handle that, each > panel driver would need to get enough DT properties added to it so > that it could fully describe any arbitrary touchscreen, right? > > Let's think about the generic panel-edp driver. This driver runs the > panel on many sc7180-trogdor laptops, including coachz, lazor, and > pompom. All three of those boards have a shared power rail for the > touchscreen and panel. If you look at "sc7180-trogdor-coachz.dtsi", > you can see the touchscreen currently looks like this: > > ap_ts: touchscreen@5d { > compatible = "goodix,gt7375p"; > reg = <0x5d>; > pinctrl-names = "default"; > pinctrl-0 = <&ts_int_l>, <&ts_reset_l>; > > interrupt-parent = <&tlmm>; > interrupts = <9 IRQ_TYPE_LEVEL_LOW>; > > reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>; > > vdd-supply = <&pp3300_ts>; > }; > > In "sc7180-trogdor-lazor.dtsi" we have: > > ap_ts: touchscreen@10 { > compatible = "hid-over-i2c"; > reg = <0x10>; > pinctrl-names = "default"; > pinctrl-0 = <&ts_int_l>, <&ts_reset_l>; > > interrupt-parent = <&tlmm>; > interrupts = <9 IRQ_TYPE_LEVEL_LOW>; > > post-power-on-delay-ms = <20>; > hid-descr-addr = <0x0001>; > > vdd-supply = <&pp3300_ts>; > }; > > In both cases "pp3300_ts" is simply another name for "pp3300_dx_edp" > > So I think to do what you propose, we need to add this information to > the panel-edp DT node so that it could dynamically construct the i2c > device for the touchscreen: > > a) Which touchscreen is actually connected (generic hid-over-i2c, > goodix, ...). I guess this would be a "compatible" string? > > b) Which i2c bus that device is hooked up to. > > c) Which i2c address that device is hooked up to. > > d) What the touchscreen interrupt GPIO is. > > e) Possibly what the "hid-descr-addr" for the touchscreen is. > > f) Any extra timing information needed to be passed to the touchscreen > driver, like "post-power-on-delay-ms" > > The "pinctrl" stuff would be easy to subsume into the panel's DT node, > at least. ...and, in this case, we could skip the "vdd-supply" since > the panel and eDP are sharing power rails (which is what got us into > this situation). ...but, the above is still a lot. At this point, it > would make sense to have a sub-node under the panel to describe it, > which we could do but it starts to feel weird. We'd essentially be > describing an i2c device but not under the i2c controller it belongs > to. > > I guess I'd also say that the above design also need additional code > if/when someone had a touchscreen that used a different communication > method, like SPI. > > So I guess the tl;dr of all the above is that I think it could all work if: > > 1. We described the touchscreen in a sub-node of the panel. > > 2. We added a property to the panel saying what the true parent of the > touchscreen was (an I2C controller, a SPI controller, ...) and what > type of controller it was ("SPI" vs "I2C"). > > 3. We added some generic helpers that panels could call that would > understand how to instantiate the touchscreen under the appropriate > controller. > > 4. From there, we added a new private / generic API between panels and > touchscreens letting them know that the panel was turning on/off. > > That seems much more complex to me, though. It also seems like an > awkward way to describe it in DT. Yeah, I guess you're right. I wish we had something simpler, but I can't think of any better way. Sorry for the distraction. > > > In any case, is there any chance that we're in violent agreement > > > > Is it even violent? Sorry if it came across that way, it's really isn't > > on my end. > > Sorry, maybe a poor choice of words on my end. I've heard that term > thrown about when two people spend a lot of time discussing something > / trying to persuade the other person only to find out in the end that > they were both on the same side of the issue. ;-) > > > > and that if you dig into my design more you might like it? Other than > > > the fact that the panel doesn't "register" the touchscreen device, it > > > kinda sounds as if what my patches are already doing is roughly what > > > you're describing. The touchscreen and panel driver are really just > > > coordinating with each other through a shared data structure (struct > > > drm_panel_follower) that has a few callback functions. Just like with > > > "hdmi-codec", the devices probe separately but find each other through > > > a phandle. The coordination between the two happens through a few > > > simple helper functions. > > > > I guess we very much agree on the end-goal, and I'd really like to get > > this addressed somehow. There's a couple of things I'm not really > > sold on with your proposal though: > > > > - It creates a ad-hoc KMS API for some problem that looks fairly > > generic. It's also redundant with the notifier mechanism without > > using it (probably for the best though). > > > > - MIPI-DSI panel probe sequence is already fairly complex and fragile > > (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges). > > I'd rather avoid creating a new dependency in that graph. > > > > - And yeah, to some extent it's inconsistent with how we dealt with > > secondary devices in KMS so far. > > Hmmmm. To a large extent, my current implementation actually has no > impact on the DRM probe sequence. The panel itself never looks for the > touchscreen code and everything DRM-related can register without a > care in the world. From reading your bullet points, I guess that's > both a strength and a weakness of my current proposal. It's really > outside the world of bridge chains and DRM components which makes it a > special snowflake that people need to understand on its own. ...but, > at the same time, the fact that it is outside all the rest of that > stuff means it doesn't add complexity to an already complex system. > > I guess I'd point to the panel backlight as a preexisting design > that's not totally unlike what I'm doing. The backlight is not part of > the DRM bridge chain and doesn't fit in like other components. This > actually makes sense since the backlight doesn't take in or put out > video data and it's simply something associated with the panel. The > backlight also has a loose connection to the panel driver and a given > panel could be associated with any number of different backlight > drivers depending on the board design. I guess one difference between > the backlight and what I'm doing with "panel follower" is that we > typically don't let the panel probe until after the backlight has > probed. In the case of my "panel follower" proposal it's the opposite. > As per above, from a DRM probe point of view this actually makes my > proposal less intrusive. I guess also a difference between backlight > and "panel follower" is that I allow an arbitrary number of followers > but there's only one backlight. > > One additional note: if I actually make the panel probe function start > registering the touchscreen, that actually _does_ add more complexity > to the already complex DRM probe ordering. It's yet another thing that > could fail and/or defer... > > Also, I'm curious: would my proposal be more or less palatable if I > made it less generic? Instead of "panel follower", I could hardcode it > to "touchscreen" and then remove all the list management. From a DRM > point of view this would make it even more like the preexisting > "backlight" except for the ordering difference. No, that's fine. I guess I don't have any objections to your work, so feel free to send a v2 :) Maxime
The big motivation for this patch series is mostly described in the patch ("drm/panel: Add a way for other devices to follow panel state"), but to quickly summarize here: for touchscreens that are connected to a panel we need the ability to power sequence the two device together. This is not a new need, but so far we've managed to get by through a combination of inefficiency, added costs, or perhaps just a little bit of brokenness. It's time to do better. This patch series allows us to do better. Assuming that people think this patch series looks OK, we'll have to figure out the right way to land it. The panel patches and i2c-hid patches will go through very different trees and so either we'll need an Ack from one side or the other or someone to create a tag for the other tree to pull in. This will _probably_ require the true drm-misc maintainers to get involved, not a lowly committer. ;-) Version 2 of this patch series doesn't change too much. At a high level: * I added all the forgotten "static" to functions. * I've hopefully made the bindings better. * I've integrated into fw_devlink. * I cleaned up a few descriptions / comments. This still needs someone to say that the idea looks OK or to suggest an alternative that solves the problems. ;-) Changes in v2: - Move the description to the generic touchscreen.yaml. - Update the desc to make it clearer it's only for integrated devices. - Add even more text to the commit message. - A few comment cleanups. - ("Add a devlink for panel followers") new for v2. - i2c_hid_core_initial_power_up() is now static. - i2c_hid_core_panel_prepared() and ..._unpreparing() are now static. - ihid_core_panel_prepare_work() is now static. - Improve documentation for smp_wmb(). Douglas Anderson (10): dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens drm/panel: Check for already prepared/enabled in drm_panel drm/panel: Add a way for other devices to follow panel state of: property: fw_devlink: Add a devlink for panel followers HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() HID: i2c-hid: Rearrange probe() to power things up later HID: i2c-hid: Make suspend and resume into helper functions HID: i2c-hid: Support being a panel follower HID: i2c-hid: Do panel follower work on the system_wq arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels .../bindings/input/elan,ekth6915.yaml | 5 + .../bindings/input/goodix,gt7375p.yaml | 5 + .../bindings/input/hid-over-i2c.yaml | 2 + .../input/touchscreen/touchscreen.yaml | 7 + .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi | 1 + .../dts/qcom/sc7180-trogdor-homestar.dtsi | 1 + .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi | 1 + .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi | 1 + .../qcom/sc7180-trogdor-quackingstick.dtsi | 1 + .../dts/qcom/sc7180-trogdor-wormdingler.dtsi | 1 + drivers/gpu/drm/drm_panel.c | 196 +++++++++- drivers/hid/i2c-hid/i2c-hid-core.c | 338 +++++++++++++----- drivers/of/property.c | 2 + include/drm/drm_panel.h | 89 +++++ 14 files changed, 555 insertions(+), 95 deletions(-)