From: Maxime Ripard <mripard@kernel.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
Bjorn Andersson <andersson@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
Frank Rowand <frowand.list@gmail.com>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Neil Armstrong <neil.armstrong@linaro.org>,
Sam Ravnborg <sam@ravnborg.org>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Thomas Zimmermann <tzimmermann@suse.de>,
dri-devel@lists.freedesktop.org,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
linux-input@vger.kernel.org, Daniel Vetter <daniel@ffwll.ch>,
linux-kernel@vger.kernel.org, hsinyi@google.com,
cros-qcom-dts-watchers@chromium.org, devicetree@vger.kernel.org,
yangcong5@huaqin.corp-partner.google.com,
linux-arm-msm@vger.kernel.org,
Chris Morgan <macroalpha82@gmail.com>
Subject: Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
Date: Tue, 13 Jun 2023 14:06:20 +0200 [thread overview]
Message-ID: <boqzlmbrp5rvepmckkqht4h5auspjlbt5leam4xivy7a4bqxnj@iuxxhooxcphk> (raw)
In-Reply-To: <CAD=FV=XnANRM=+2D9+DzcXx9Gw6iKKQsgkAiq8=izNEN-91f_Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6926 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2023-06-13 12:06 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-07 21:49 [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 01/10] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed touchscreens Douglas Anderson
2023-06-09 15:53 ` Krzysztof Kozlowski
2023-06-07 21:49 ` [PATCH v2 02/10] drm/panel: Check for already prepared/enabled in drm_panel Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 03/10] drm/panel: Add a way for other devices to follow panel state Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 04/10] of: property: fw_devlink: Add a devlink for panel followers Douglas Anderson
2023-06-09 16:10 ` Rob Herring
2023-06-07 21:49 ` [PATCH v2 05/10] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 06/10] HID: i2c-hid: Rearrange probe() to power things up later Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 07/10] HID: i2c-hid: Make suspend and resume into helper functions Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 08/10] HID: i2c-hid: Support being a panel follower Douglas Anderson
2023-06-08 5:18 ` kernel test robot
2023-06-08 7:14 ` kernel test robot
2023-06-08 15:10 ` Doug Anderson
2023-06-08 15:36 ` Benjamin Tissoires
2023-06-08 16:42 ` Doug Anderson
2023-06-09 9:27 ` Benjamin Tissoires
2023-06-09 15:01 ` Doug Anderson
2023-06-26 22:49 ` Doug Anderson
2023-07-17 18:15 ` Doug Anderson
2023-07-25 20:41 ` Doug Anderson
2023-07-26 8:07 ` Benjamin Tissoires
2023-06-07 21:49 ` [PATCH v2 09/10] HID: i2c-hid: Do panel follower work on the system_wq Douglas Anderson
2023-06-07 21:49 ` [PATCH v2 10/10] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels Douglas Anderson
2023-06-08 7:17 ` [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Maxime Ripard
2023-06-08 14:38 ` Doug Anderson
2023-06-12 16:03 ` Maxime Ripard
2023-06-12 21:13 ` Doug Anderson
2023-06-13 12:06 ` Maxime Ripard [this message]
2023-06-13 15:56 ` Doug Anderson
2023-06-21 16:31 ` Doug Anderson
2023-06-23 9:08 ` Maxime Ripard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=boqzlmbrp5rvepmckkqht4h5auspjlbt5leam4xivy7a4bqxnj@iuxxhooxcphk \
--to=mripard@kernel.org \
--cc=andersson@kernel.org \
--cc=benjamin.tissoires@redhat.com \
--cc=conor+dt@kernel.org \
--cc=cros-qcom-dts-watchers@chromium.org \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=dmitry.torokhov@gmail.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=frowand.list@gmail.com \
--cc=hsinyi@google.com \
--cc=jikos@kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=macroalpha82@gmail.com \
--cc=neil.armstrong@linaro.org \
--cc=robh+dt@kernel.org \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
--cc=yangcong5@huaqin.corp-partner.google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).