| Message ID | 20230607215224.2067679-1-dianders@chromium.org (mailing list archive) |
|---|---|
| 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
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
Hi, On Mon, Jun 12, 2023 at 9:03 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > I guess we can have > > > something much simpler with a bunch of helpers that would register a > > > i2c-hid device and would be called by the panel driver itself. > > > > > > And then, since everything is self-contained managing the power state > > > becomes easier as well. > > > > Can you give me more details about how you think this would work? > > > > When you say that the panel would register an i2c-hid device itself, > > do you mean that we'd do something like give a phandle to the i2c bus > > to the panel and then the panel would manually instantiate the i2c-hid > > device on it? ...and I guess it would need to be a "subclass" of > > i2c-hid that knew about the connection to the panel code? This > > subclass and the panel code would communicate with each other about > > power sequencing needs through some private API (like MFD devices > > usually do?). Assuming I'm understanding correctly, I think that could > > work. > > I guess what I had in mind is to do something similar to what we're > doing with hdmi-codec already for example. By this you mean "rockchip,hdmi-codec" and "mediatek,hdmi-codec", right? > We have several logical components already, in separate drivers, that > still need some cooperation. > > If the panel and touchscreen are on the same i2c bus, I think we could > even just get a reference to the panel i2c adapter, get a reference, and > pass that to i2c-hid (with a nice layer of helpers). Just for reference: the panel and touchscreen aren't on the same i2c bus. In the cases that I've looked at the panel is either controlled entirely by eDP or MIPI signals and isn't on any i2c bus at all. The touchscreen is on the i2c bus in the cases I've looked at, though I suppose I could imagine one that used a different bus. > What I'm trying to say is: could we just make it work by passing a bunch > of platform_data, 2-3 callbacks and a device registration from the panel > driver directly? I think I'm still confused about what you're proposing. Sorry! :( Let me try rephrasing why I'm confused and perhaps we can get on the same page. :-) First, I guess I'm confused about how you have one of these devices "register" the other device. I can understand how one device might "register" its sub-devices in the MFD case. To make it concrete, we can look at a PMIC like max77686.c. The parent MFD device gets probed and then it's in charge of creating all of its sub-devices. These sub-devices are intimately tied to one another. They have shared data structures and can coordinate power sequencing and whatnot. All good. ...but here, we really have something different in two fundamental ways: a) In this case, the two components (panel and touchscreen) both use separate primary communication methods. In DT the primary communication method determines where the device is described in the hierarchy. For eDP, this means that the DT node for the panel should be under the eDP controller. For an i2c touchscreen, this means that the DT node for the touchscreen should be under the i2c controller. Describing things like this causes the eDP controller to "register" the panel and the i2c controller to "register" the touchscreen. If we wanted the panel driver to "register" the touchscreen then it would get really awkward. Do we leave the touchscreen DT node under the i2c controller but somehow tell the i2c subsytem not to register it? Do we try to dynamically construct the touchscreen i2c node? Do we make a fake i2c controller under our panel DT node and somehow tell the i2c core to look at it? b) Things are different because the two devices here are not nearly as intimately tied to one another. At least in the case of "homestar", the only reason that the devices were tied to one another was because the board designers chose to share power rails, but otherwise the drivers were both generic. In any case, is there any chance that we're in violent agreement and that if you dig into my design more you might like it? Other than the fact that the panel doesn't "register" the touchscreen device, it kinda sounds as if what my patches are already doing is roughly what you're describing. The touchscreen and panel driver are really just coordinating with each other through a shared data structure (struct drm_panel_follower) that has a few callback functions. Just like with "hdmi-codec", the devices probe separately but find each other through a phandle. The coordination between the two happens through a few simple helper functions. > > Is it cleaner than my current approach, though? > > "cleaner" is subjective, really, but it's a more "mainstream" approach > that one can follow more easily through function calls. > > > I guess, alternatively, we could put the "panel" directly under the > > i2c bus in this case. That would probably work for Cong Yang's current > > needs, but we'd end up in trouble if we ever had a similar situation > > with an eDP panel since eDP panels need to be under the DP-AUX bus. > > I don't know DP-AUX very well, what is the issue that you're mentioning? Hopefully I've explained what I meant above (see point "a)").
On Mon, Jun 12, 2023 at 02:13:46PM -0700, Doug Anderson wrote: > Hi, > > On Mon, Jun 12, 2023 at 9:03 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > I guess we can have > > > > something much simpler with a bunch of helpers that would register a > > > > i2c-hid device and would be called by the panel driver itself. > > > > > > > > And then, since everything is self-contained managing the power state > > > > becomes easier as well. > > > > > > Can you give me more details about how you think this would work? > > > > > > When you say that the panel would register an i2c-hid device itself, > > > do you mean that we'd do something like give a phandle to the i2c bus > > > to the panel and then the panel would manually instantiate the i2c-hid > > > device on it? ...and I guess it would need to be a "subclass" of > > > i2c-hid that knew about the connection to the panel code? This > > > subclass and the panel code would communicate with each other about > > > power sequencing needs through some private API (like MFD devices > > > usually do?). Assuming I'm understanding correctly, I think that could > > > work. > > > > I guess what I had in mind is to do something similar to what we're > > doing with hdmi-codec already for example. > > By this you mean "rockchip,hdmi-codec" and "mediatek,hdmi-codec", right? No, sorry it was a bit ambiguous. I meant how we instantiate the hdmi-codec driver here for example: https://elixir.bootlin.com/linux/v6.3.7/source/drivers/gpu/drm/exynos/exynos_hdmi.c#L1665 https://elixir.bootlin.com/linux/v6.3.7/source/drivers/gpu/drm/vc4/vc4_hdmi.c#L2539 https://elixir.bootlin.com/linux/v6.3.7/source/drivers/gpu/drm/tegra/hdmi.c#L1525 > > We have several logical components already, in separate drivers, that > > still need some cooperation. > > > > If the panel and touchscreen are on the same i2c bus, I think we could > > even just get a reference to the panel i2c adapter, get a reference, and > > pass that to i2c-hid (with a nice layer of helpers). > > Just for reference: the panel and touchscreen aren't on the same i2c > bus. In the cases that I've looked at the panel is either controlled > entirely by eDP or MIPI signals and isn't on any i2c bus at all. The > touchscreen is on the i2c bus in the cases I've looked at, though I > suppose I could imagine one that used a different bus. Ok, so we would indeed need a phandle to the i2c controller > > What I'm trying to say is: could we just make it work by passing a bunch > > of platform_data, 2-3 callbacks and a device registration from the panel > > driver directly? > > I think I'm still confused about what you're proposing. Sorry! :( Let > me try rephrasing why I'm confused and perhaps we can get on the same > page. :-) > > First, I guess I'm confused about how you have one of these devices > "register" the other device. > > I can understand how one device might "register" its sub-devices in > the MFD case. To make it concrete, we can look at a PMIC like > max77686.c. The parent MFD device gets probed and then it's in charge > of creating all of its sub-devices. These sub-devices are intimately > tied to one another. They have shared data structures and can > coordinate power sequencing and whatnot. All good. We don't necessarily need to use MFD, but yeah, we could just register a device for the i2c-hid driver to probe from (using i2c_new_client_device?) > ...but here, we really have something different in two fundamental ways: > > a) In this case, the two components (panel and touchscreen) both use > separate primary communication methods. In DT the primary > communication method determines where the device is described in the > hierarchy. For eDP, this means that the DT node for the panel should > be under the eDP controller. For an i2c touchscreen, this means that > the DT node for the touchscreen should be under the i2c controller. > Describing things like this causes the eDP controller to "register" > the panel and the i2c controller to "register" the touchscreen. If we > wanted the panel driver to "register" the touchscreen then it would > get really awkward. Do we leave the touchscreen DT node under the i2c > controller but somehow tell the i2c subsytem not to register it? Do we > try to dynamically construct the touchscreen i2c node? Do we make a > fake i2c controller under our panel DT node and somehow tell the i2c > core to look at it? I would expect not to have any DT node for the touchscreen, but we would register a new i2c device on the bus that it's connected to. In essence, it's also fairly similar to what we're doing with i2c_new_ancillary_device() on some bridges. Except the primary device isn't necessarily controlled through the I2C bus (but could be, I'm pretty sure we have that situation for RGB or LVDS panels too). The plus side would also be that we don't really need a DT to make it work either. We just need the panel driver to probe somehow and a pointer to the i2c_adapter. > b) Things are different because the two devices here are not nearly as > intimately tied to one another. At least in the case of "homestar", > the only reason that the devices were tied to one another was because > the board designers chose to share power rails, but otherwise the > drivers were both generic. Yeah, and that's fine I guess? > In any case, is there any chance that we're in violent agreement Is it even violent? Sorry if it came across that way, it's really isn't on my end. > and that if you dig into my design more you might like it? Other than > the fact that the panel doesn't "register" the touchscreen device, it > kinda sounds as if what my patches are already doing is roughly what > you're describing. The touchscreen and panel driver are really just > coordinating with each other through a shared data structure (struct > drm_panel_follower) that has a few callback functions. Just like with > "hdmi-codec", the devices probe separately but find each other through > a phandle. The coordination between the two happens through a few > simple helper functions. I guess we very much agree on the end-goal, and I'd really like to get this addressed somehow. There's a couple of things I'm not really sold on with your proposal though: - It creates a ad-hoc KMS API for some problem that looks fairly generic. It's also redundant with the notifier mechanism without using it (probably for the best though). - MIPI-DSI panel probe sequence is already fairly complex and fragile (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges). I'd rather avoid creating a new dependency in that graph. - And yeah, to some extent it's inconsistent with how we dealt with secondary devices in KMS so far. Maxime
Hi, On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > What I'm trying to say is: could we just make it work by passing a bunch > > > of platform_data, 2-3 callbacks and a device registration from the panel > > > driver directly? > > > > I think I'm still confused about what you're proposing. Sorry! :( Let > > me try rephrasing why I'm confused and perhaps we can get on the same > > page. :-) > > > > First, I guess I'm confused about how you have one of these devices > > "register" the other device. > > > > I can understand how one device might "register" its sub-devices in > > the MFD case. To make it concrete, we can look at a PMIC like > > max77686.c. The parent MFD device gets probed and then it's in charge > > of creating all of its sub-devices. These sub-devices are intimately > > tied to one another. They have shared data structures and can > > coordinate power sequencing and whatnot. All good. > > We don't necessarily need to use MFD, but yeah, we could just register a > device for the i2c-hid driver to probe from (using > i2c_new_client_device?) I think this can work for devices where the panel and touchscreen are truly integrated where the panel driver knows enough about the related touchscreen to fully describe and instantiate it. It doesn't work quite as well for cases where the power and reset lines are shared just because of what a given board designer did. To handle that, each panel driver would need to get enough DT properties added to it so that it could fully describe any arbitrary touchscreen, right? Let's think about the generic panel-edp driver. This driver runs the panel on many sc7180-trogdor laptops, including coachz, lazor, and pompom. All three of those boards have a shared power rail for the touchscreen and panel. If you look at "sc7180-trogdor-coachz.dtsi", you can see the touchscreen currently looks like this: ap_ts: touchscreen@5d { compatible = "goodix,gt7375p"; reg = <0x5d>; pinctrl-names = "default"; pinctrl-0 = <&ts_int_l>, <&ts_reset_l>; interrupt-parent = <&tlmm>; interrupts = <9 IRQ_TYPE_LEVEL_LOW>; reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>; vdd-supply = <&pp3300_ts>; }; In "sc7180-trogdor-lazor.dtsi" we have: ap_ts: touchscreen@10 { compatible = "hid-over-i2c"; reg = <0x10>; pinctrl-names = "default"; pinctrl-0 = <&ts_int_l>, <&ts_reset_l>; interrupt-parent = <&tlmm>; interrupts = <9 IRQ_TYPE_LEVEL_LOW>; post-power-on-delay-ms = <20>; hid-descr-addr = <0x0001>; vdd-supply = <&pp3300_ts>; }; In both cases "pp3300_ts" is simply another name for "pp3300_dx_edp" So I think to do what you propose, we need to add this information to the panel-edp DT node so that it could dynamically construct the i2c device for the touchscreen: a) Which touchscreen is actually connected (generic hid-over-i2c, goodix, ...). I guess this would be a "compatible" string? b) Which i2c bus that device is hooked up to. c) Which i2c address that device is hooked up to. d) What the touchscreen interrupt GPIO is. e) Possibly what the "hid-descr-addr" for the touchscreen is. f) Any extra timing information needed to be passed to the touchscreen driver, like "post-power-on-delay-ms" The "pinctrl" stuff would be easy to subsume into the panel's DT node, at least. ...and, in this case, we could skip the "vdd-supply" since the panel and eDP are sharing power rails (which is what got us into this situation). ...but, the above is still a lot. At this point, it would make sense to have a sub-node under the panel to describe it, which we could do but it starts to feel weird. We'd essentially be describing an i2c device but not under the i2c controller it belongs to. I guess I'd also say that the above design also need additional code if/when someone had a touchscreen that used a different communication method, like SPI. So I guess the tl;dr of all the above is that I think it could all work if: 1. We described the touchscreen in a sub-node of the panel. 2. We added a property to the panel saying what the true parent of the touchscreen was (an I2C controller, a SPI controller, ...) and what type of controller it was ("SPI" vs "I2C"). 3. We added some generic helpers that panels could call that would understand how to instantiate the touchscreen under the appropriate controller. 4. From there, we added a new private / generic API between panels and touchscreens letting them know that the panel was turning on/off. That seems much more complex to me, though. It also seems like an awkward way to describe it in DT. > > In any case, is there any chance that we're in violent agreement > > Is it even violent? Sorry if it came across that way, it's really isn't > on my end. Sorry, maybe a poor choice of words on my end. I've heard that term thrown about when two people spend a lot of time discussing something / trying to persuade the other person only to find out in the end that they were both on the same side of the issue. ;-) > > and that if you dig into my design more you might like it? Other than > > the fact that the panel doesn't "register" the touchscreen device, it > > kinda sounds as if what my patches are already doing is roughly what > > you're describing. The touchscreen and panel driver are really just > > coordinating with each other through a shared data structure (struct > > drm_panel_follower) that has a few callback functions. Just like with > > "hdmi-codec", the devices probe separately but find each other through > > a phandle. The coordination between the two happens through a few > > simple helper functions. > > I guess we very much agree on the end-goal, and I'd really like to get > this addressed somehow. There's a couple of things I'm not really > sold on with your proposal though: > > - It creates a ad-hoc KMS API for some problem that looks fairly > generic. It's also redundant with the notifier mechanism without > using it (probably for the best though). > > - MIPI-DSI panel probe sequence is already fairly complex and fragile > (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges). > I'd rather avoid creating a new dependency in that graph. > > - And yeah, to some extent it's inconsistent with how we dealt with > secondary devices in KMS so far. Hmmmm. To a large extent, my current implementation actually has no impact on the DRM probe sequence. The panel itself never looks for the touchscreen code and everything DRM-related can register without a care in the world. From reading your bullet points, I guess that's both a strength and a weakness of my current proposal. It's really outside the world of bridge chains and DRM components which makes it a special snowflake that people need to understand on its own. ...but, at the same time, the fact that it is outside all the rest of that stuff means it doesn't add complexity to an already complex system. I guess I'd point to the panel backlight as a preexisting design that's not totally unlike what I'm doing. The backlight is not part of the DRM bridge chain and doesn't fit in like other components. This actually makes sense since the backlight doesn't take in or put out video data and it's simply something associated with the panel. The backlight also has a loose connection to the panel driver and a given panel could be associated with any number of different backlight drivers depending on the board design. I guess one difference between the backlight and what I'm doing with "panel follower" is that we typically don't let the panel probe until after the backlight has probed. In the case of my "panel follower" proposal it's the opposite. As per above, from a DRM probe point of view this actually makes my proposal less intrusive. I guess also a difference between backlight and "panel follower" is that I allow an arbitrary number of followers but there's only one backlight. One additional note: if I actually make the panel probe function start registering the touchscreen, that actually _does_ add more complexity to the already complex DRM probe ordering. It's yet another thing that could fail and/or defer... Also, I'm curious: would my proposal be more or less palatable if I made it less generic? Instead of "panel follower", I could hardcode it to "touchscreen" and then remove all the list management. From a DRM point of view this would make it even more like the preexisting "backlight" except for the ordering difference. -Doug
Maxime, On Tue, Jun 13, 2023 at 8:56 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > What I'm trying to say is: could we just make it work by passing a bunch > > > > of platform_data, 2-3 callbacks and a device registration from the panel > > > > driver directly? > > > > > > I think I'm still confused about what you're proposing. Sorry! :( Let > > > me try rephrasing why I'm confused and perhaps we can get on the same > > > page. :-) > > > > > > First, I guess I'm confused about how you have one of these devices > > > "register" the other device. > > > > > > I can understand how one device might "register" its sub-devices in > > > the MFD case. To make it concrete, we can look at a PMIC like > > > max77686.c. The parent MFD device gets probed and then it's in charge > > > of creating all of its sub-devices. These sub-devices are intimately > > > tied to one another. They have shared data structures and can > > > coordinate power sequencing and whatnot. All good. > > > > We don't necessarily need to use MFD, but yeah, we could just register a > > device for the i2c-hid driver to probe from (using > > i2c_new_client_device?) > > I think this can work for devices where the panel and touchscreen are > truly integrated where the panel driver knows enough about the related > touchscreen to fully describe and instantiate it. It doesn't work > quite as well for cases where the power and reset lines are shared > just because of what a given board designer did. To handle that, each > panel driver would need to get enough DT properties added to it so > that it could fully describe any arbitrary touchscreen, right? > > Let's think about the generic panel-edp driver. This driver runs the > panel on many sc7180-trogdor laptops, including coachz, lazor, and > pompom. All three of those boards have a shared power rail for the > touchscreen and panel. If you look at "sc7180-trogdor-coachz.dtsi", > you can see the touchscreen currently looks like this: > > ap_ts: touchscreen@5d { > compatible = "goodix,gt7375p"; > reg = <0x5d>; > pinctrl-names = "default"; > pinctrl-0 = <&ts_int_l>, <&ts_reset_l>; > > interrupt-parent = <&tlmm>; > interrupts = <9 IRQ_TYPE_LEVEL_LOW>; > > reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>; > > vdd-supply = <&pp3300_ts>; > }; > > In "sc7180-trogdor-lazor.dtsi" we have: > > ap_ts: touchscreen@10 { > compatible = "hid-over-i2c"; > reg = <0x10>; > pinctrl-names = "default"; > pinctrl-0 = <&ts_int_l>, <&ts_reset_l>; > > interrupt-parent = <&tlmm>; > interrupts = <9 IRQ_TYPE_LEVEL_LOW>; > > post-power-on-delay-ms = <20>; > hid-descr-addr = <0x0001>; > > vdd-supply = <&pp3300_ts>; > }; > > In both cases "pp3300_ts" is simply another name for "pp3300_dx_edp" > > So I think to do what you propose, we need to add this information to > the panel-edp DT node so that it could dynamically construct the i2c > device for the touchscreen: > > a) Which touchscreen is actually connected (generic hid-over-i2c, > goodix, ...). I guess this would be a "compatible" string? > > b) Which i2c bus that device is hooked up to. > > c) Which i2c address that device is hooked up to. > > d) What the touchscreen interrupt GPIO is. > > e) Possibly what the "hid-descr-addr" for the touchscreen is. > > f) Any extra timing information needed to be passed to the touchscreen > driver, like "post-power-on-delay-ms" > > The "pinctrl" stuff would be easy to subsume into the panel's DT node, > at least. ...and, in this case, we could skip the "vdd-supply" since > the panel and eDP are sharing power rails (which is what got us into > this situation). ...but, the above is still a lot. At this point, it > would make sense to have a sub-node under the panel to describe it, > which we could do but it starts to feel weird. We'd essentially be > describing an i2c device but not under the i2c controller it belongs > to. > > I guess I'd also say that the above design also need additional code > if/when someone had a touchscreen that used a different communication > method, like SPI. > > > So I guess the tl;dr of all the above is that I think it could all work if: > > 1. We described the touchscreen in a sub-node of the panel. > > 2. We added a property to the panel saying what the true parent of the > touchscreen was (an I2C controller, a SPI controller, ...) and what > type of controller it was ("SPI" vs "I2C"). > > 3. We added some generic helpers that panels could call that would > understand how to instantiate the touchscreen under the appropriate > controller. > > 4. From there, we added a new private / generic API between panels and > touchscreens letting them know that the panel was turning on/off. > > That seems much more complex to me, though. It also seems like an > awkward way to describe it in DT. > > > > > In any case, is there any chance that we're in violent agreement > > > > Is it even violent? Sorry if it came across that way, it's really isn't > > on my end. > > Sorry, maybe a poor choice of words on my end. I've heard that term > thrown about when two people spend a lot of time discussing something > / trying to persuade the other person only to find out in the end that > they were both on the same side of the issue. ;-) > > > > > and that if you dig into my design more you might like it? Other than > > > the fact that the panel doesn't "register" the touchscreen device, it > > > kinda sounds as if what my patches are already doing is roughly what > > > you're describing. The touchscreen and panel driver are really just > > > coordinating with each other through a shared data structure (struct > > > drm_panel_follower) that has a few callback functions. Just like with > > > "hdmi-codec", the devices probe separately but find each other through > > > a phandle. The coordination between the two happens through a few > > > simple helper functions. > > > > I guess we very much agree on the end-goal, and I'd really like to get > > this addressed somehow. There's a couple of things I'm not really > > sold on with your proposal though: > > > > - It creates a ad-hoc KMS API for some problem that looks fairly > > generic. It's also redundant with the notifier mechanism without > > using it (probably for the best though). > > > > - MIPI-DSI panel probe sequence is already fairly complex and fragile > > (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges). > > I'd rather avoid creating a new dependency in that graph. > > > > - And yeah, to some extent it's inconsistent with how we dealt with > > secondary devices in KMS so far. > > Hmmmm. To a large extent, my current implementation actually has no > impact on the DRM probe sequence. The panel itself never looks for the > touchscreen code and everything DRM-related can register without a > care in the world. From reading your bullet points, I guess that's > both a strength and a weakness of my current proposal. It's really > outside the world of bridge chains and DRM components which makes it a > special snowflake that people need to understand on its own. ...but, > at the same time, the fact that it is outside all the rest of that > stuff means it doesn't add complexity to an already complex system. > > I guess I'd point to the panel backlight as a preexisting design > that's not totally unlike what I'm doing. The backlight is not part of > the DRM bridge chain and doesn't fit in like other components. This > actually makes sense since the backlight doesn't take in or put out > video data and it's simply something associated with the panel. The > backlight also has a loose connection to the panel driver and a given > panel could be associated with any number of different backlight > drivers depending on the board design. I guess one difference between > the backlight and what I'm doing with "panel follower" is that we > typically don't let the panel probe until after the backlight has > probed. In the case of my "panel follower" proposal it's the opposite. > As per above, from a DRM probe point of view this actually makes my > proposal less intrusive. I guess also a difference between backlight > and "panel follower" is that I allow an arbitrary number of followers > but there's only one backlight. > > One additional note: if I actually make the panel probe function start > registering the touchscreen, that actually _does_ add more complexity > to the already complex DRM probe ordering. It's yet another thing that > could fail and/or defer... > > Also, I'm curious: would my proposal be more or less palatable if I > made it less generic? Instead of "panel follower", I could hardcode it > to "touchscreen" and then remove all the list management. From a DRM > point of view this would make it even more like the preexisting > "backlight" except for the ordering difference. I'm trying to figure out what the next steps are here. I can send a v3 and address Benjamin's comments on the i2c-hid side, but I'd like to get some resolution on our conversation here, too. Did my thoughts above make sense? I know that "panel follower" isn't a beautiful/elegant framework, but IMO it isn't not too bad. It accomplishes the goal and mostly stays out of the way. If you don't have time to dig into all of this now, would you object if I can find someone else willing to review my series from a drm perspective? Thanks! -Doug
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(-)