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