From: Benjamin Tissoires <bentiss@kernel.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>,
Jiri Kosina <jikos@kernel.org>,
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>,
Maxime Ripard <mripard@kernel.org>,
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 08/10] HID: i2c-hid: Support being a panel follower
Date: Wed, 26 Jul 2023 10:07:00 +0200 [thread overview]
Message-ID: <wcilyqbh23xjscsvubxjnkwlctxuvyj5weix2opywkolg7udyb@gad2pmlazxx4> (raw)
In-Reply-To: <CAD=FV=WmpH6cB0oZOxbL+TUxjLRf3PM+kKvuYRAZSiEhS4o37A@mail.gmail.com>
On Jul 25 2023, Doug Anderson wrote:
> Hi,
>
> On Mon, Jul 17, 2023 at 11:15 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Benjamin,
> >
> > On Mon, Jun 26, 2023 at 3:49 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Benjamin,
> > >
> > > On Thu, Jun 8, 2023 at 8:37 AM Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > >
> > > > > +static const struct drm_panel_follower_funcs i2c_hid_core_panel_follower_funcs = {
> > > > > + .panel_prepared = i2c_hid_core_panel_prepared,
> > > > > + .panel_unpreparing = i2c_hid_core_panel_unpreparing,
> > > > > +};
> > > >
> > > > Can we make that above block at least behind a Kconfig?
> > > >
> > > > i2c-hid is often used for touchpads, and the notion of drm panel has
> > > > nothing to do with them. So I'd be more confident if we could disable
> > > > that code if not required.
> > >
> > > Now that other concerns are addressed, I started trying to write up a
> > > v3 and I found myself writing this as the description of the Kconfig
> > > entry:
> > >
> > > --
> > > config I2C_HID_SUPPORT_PANEL_FOLLOWER
> > > bool "Support i2c-hid devices that must be power sequenced with a panel"
> > >
> > > Say Y here if you want support for i2c-hid devices that need to
> > > coordinate power sequencing with a panel. This is typically important
> > > when you have a panel and a touchscreen that share power rails or
> > > reset GPIOs. If you say N here then the kernel will not try to honor
> > > any shared power sequencing for your hardware. In the best case,
> > > ignoring power sequencing when it's needed will draw extra power. In
> > > the worst case this will prevent your hardware from functioning or
> > > could even damage your hardware.
> > >
> > > If unsure, say Y.
> > >
> > > --
> > >
> > > I can certainly go that way, but I just wanted to truly make sure
> > > that's what we want. Specifically:
> > >
> > > 1. If we put the panel follower code behind a Kconfig then we actually
> > > have no idea if a touchscreen was intended to be a panel follower.
> > > Specifically the panel follower API is the one that detects the
> > > connection between the panel and the i2c-hid device, so without being
> > > able to call the panel follower API we have no idea that an i2c-hid
> > > device was supposed to be a panel follower.
> > >
> > > 2. It is conceivable that power sequencing a device incorrectly could
> > > truly cause hardware damage.
> > >
> > > Together, those points mean that if you turn off the Kconfig entry and
> > > then try to boot on a device that needed that Kconfig setting that you
> > > might damage hardware. I can code it up that way if you want, but it
> > > worries me...
> > >
> > >
> > > Alternatives that I can think of:
> > >
> > > a) I could change the panel follower API so that panel followers are
> > > in charge of detecting the panel that they follow. Today, that looks
> > > like:
> > >
> > > panel_np = of_parse_phandle(dev->of_node, "panel", 0);
> > > if (panel_np)
> > > /* It's a panel follower */
> > > of_node_put(panel_np);
> > >
> > > ...so we could put that code in each touchscreen driver and then fail
> > > to probe i2c-hid if we detect that we're supposed to be a panel
> > > follower but the Kconfig is turned off. The above doesn't seem
> > > massively ideal since it duplicates code. Also, one reason why I put
> > > that code in drm_panel_add_follower() is that I think this concept
> > > will eventually be needed even for non-DT cases. I don't know how to
> > > write the non-DT code right now, though...
> > >
> > >
> > > b) I could open-code detect the panel follower case but leave the
> > > actual linking to the panel follower API. AKA add to i2c-hid:
> > >
> > > if (of_property_read_bool(dev->of_node, "panel"))
> > > /* It's a panel follower */
> > >
> > > ...that's a smaller bit of code, but feels like an abstraction
> > > violation. It also would need to be updated if/when we added support
> > > for non-DT panel followers.
> > >
> > >
> > > c) I could add a "static inline" implementation of b) to "drm_panel.h".
> > >
> > > That sounds great and I started doing it. ...but then realized that it
> > > means adding to drm_panel.h:
> > >
> > > #include <linux/device.h>
> > > #include <linux/of.h>
> > >
> > > ...because otherwise of_property_read_bool() isn't defined and "struct
> > > device" can't be dereferenced. That might be OK, but it looks as if
> > > folks have been working hard to avoid things like this in header
> > > files. Presumably it would get uglier if/when we added the non-DT
> > > case, as well. That being said, I can give it a shot...
> > >
> > > --
> > >
> > > At this point, I'm hoping for some advice. How important is it for you
> > > to have a Kconfig for "I2C_HID_SUPPORT_PANEL_FOLLOWER"?
> > >
> > > NOTE: even if I don't add the Kconfig, I could at least create a
> > > function for registering the panel follower that would get most of the
> > > panel follower logic out of the probe function. Would that be enough?
> >
> > I'd love to send a new version of this patch series, but I'm still
> > stuck with the above issue. I'm hoping you might have a minute to
> > provide your thoughts. If I don't hear anything, I'll try a v3 where I
> > don't have the Kconfig for "I2C_HID_SUPPORT_PANEL_FOLLOWER" but just
> > try to pull a little more of the code out of the probe function.
>
> To provide breadcrumbs, I posted the v3 which pulls a bit more code
> out of the probe function but is otherwise largely unchanged. The
> cover letter for v3 can be found at:
Apologies for the delay. Given that you received feedbacks from other
folks I wanted things to settle down a little bit before returning to
this discussion. Sorry.
>
> https://lore.kernel.org/r/20230725203545.2260506-1-dianders@chromium.org/
I like the 8th patch of this series much more. If there is a risk of
damaging the device, then we should not have the Kconfig to disable it.
I have some comments on that particular patch (v3 8/10), but I; ll reply
inline.
Cheers,
Benjamin
next prev parent reply other threads:[~2023-07-26 8:18 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 [this message]
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
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=wcilyqbh23xjscsvubxjnkwlctxuvyj5weix2opywkolg7udyb@gad2pmlazxx4 \
--to=bentiss@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=mripard@kernel.org \
--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).