Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together

[Doug Anderson]

Hi,

On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@xxxxxxxxxx> 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