Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20231002155857.24584-1-johan+linaro@kernel.org> <cnnjghxj4od6fniotztlgorc7myzya2bsvixkf2cpk4metcipa@r2w2ouob5ths>
In-Reply-To: <cnnjghxj4od6fniotztlgorc7myzya2bsvixkf2cpk4metcipa@r2w2ouob5ths>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 5 Oct 2023 09:31:34 -0700
Message-ID: <CAD=FV=Wx2m2qgQwY7Od4T__SzC4zZ9Txr4D553+XmLGEaT15rA@mail.gmail.com>
Subject: Re: [PATCH v2] HID: i2c-hid: fix handling of unpopulated devices
To:     Benjamin Tissoires <bentiss@kernel.org>
Cc:     Johan Hovold <johan+linaro@kernel.org>,
        Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        Maxime Ripard <mripard@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Thu, Oct 5, 2023 at 12:10=E2=80=AFAM Benjamin Tissoires <bentiss@kernel.=
org> wrote:
>
> On Oct 02 2023, Johan Hovold wrote:
> > A recent commit reordered probe so that the interrupt line is now
> > requested before making sure that the device exists.
> >
> > This breaks machines like the Lenovo ThinkPad X13s which rely on the
> > HID driver to probe second-source devices and only register the variant
> > that is actually populated. Specifically, the interrupt line may now
> > already be (temporarily) claimed when doing asynchronous probing of the
> > touchpad:
> >
> >       genirq: Flags mismatch irq 191. 00082008 (hid-over-i2c) vs. 00082=
008 (hid-over-i2c)
> >       i2c_hid_of 21-0015: Could not register for hid-over-i2c interrupt=
, irq =3D 191, ret =3D -16
> >       i2c_hid_of: probe of 21-0015 failed with error -16
> >
> > Fix this by restoring the old behaviour of first making sure the device
> > exists before requesting the interrupt line.
> >
> > Note that something like this should probably be implemented also for
> > "panel followers", whose actual probe is currently effectively deferred
> > until the DRM panel is probed (e.g. by powering down the device after
> > making sure it exists and only then register it as a follower).
> >
> > Fixes: 675cd877c952 ("HID: i2c-hid: Rearrange probe() to power things u=
p later")
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >
> > Changes in v2
> >  - initialise ihid->is_panel_follower sooner to avoid repeated property
> >    lookups and so that it can be used consistently throughout the drive=
r
> >    for code that differs for "panel followers"
>
> Patches looks good to me, but I can not test it unfortunately.
>
> Doug, would you mind sending us your Ack or tested-by?

Sure. Patches look OK to me, so if you're good with them then I'm good
with them too.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

I tested this on a board by adding a second i2c-hid device in the
device tree and confirming that things appeared to work OK. I also
tried this with a board setup to use panel-follower (but _not_ two
sources of touchscreens) and that also worked OK. Thus:

Tested-by: Douglas Anderson <dianders@chromium.org>

As expected, combining panel-follower with two sources of touchscreen
_didn't_ work because only one of them is able to acquire the
interrupt. This is fine with me as there is nobody currently doing
that. I'm still of the belief that we need a more complete solution
for that and I'll continue to work on it.

Thanks!

-Doug