Re: [PATCH v2 2/2] Input: hideep - Optionally reset controller work mode to native HiDeep protocol

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Sun, Mar 05, 2023 at 04:04:30PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/5/23 14:34, Krzysztof Kozlowski wrote:
> > On 03/03/2023 23:21, Hans de Goede wrote:
> >> The HiDeep IST940E touchscreen controller used on the Lenovo Yoga Book X90F
> >> convertible comes up in HID mode by default.
> >>
> >> This works well on the X91F Windows model where the touchscreen is
> >> correctly described in ACPI and ACPI takes care of controlling
> >> the reset GPIO and regulators.
> >>
> >> But the X90F ships with Android and the ACPI tables on this model don't
> >> describe the touchscreen. Instead this is hardcoded in the vendor kernel.
> >>
> >> The vendor kernel uses the touchscreen in native HiDeep 20 (2.0?) protocol
> >> mode and switches the controller to this mode by writing 0 to reg 0x081e.
> >>
> >> Adjusting the i2c-hid code to deal with the reset-gpio and regulators on
> >> this non devicetree (but rather broken ACPI) convertible is somewhat tricky
> >> and the native protocol reports ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR
> >> which are not reported in HID mode, so it is preferable to use the native
> >> mode.
> >>
> >> Add support to the hideep driver to reset the work-mode to the native
> >> HiDeep protocol to allow using it on the Lenovo Yoga Book X90F.
> >> This is guarded behind a new "hideep,reset-work-mode" boolean property,
> >> to avoid changing behavior on other devices.
> >>
> >> For the record: I did test using the i2c-hid driver with some quick hacks
> >> and it does work. The I2C-HID descriptor is available from address 0x0020,
> >> just like on the X91F Windows model.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Please use scripts/get_maintainers.pl to get a list of necessary people
> > and lists to CC.  It might happen, that command when run on an older
> > kernel, gives you outdated entries.  Therefore please be sure you base
> > your patches on recent Linux kernel.
> > 
> >> ---
> >>  .../bindings/input/touchscreen/hideep.txt        |  1 +
> >>  drivers/input/touchscreen/hideep.c               | 16 ++++++++++++++++
> >>  2 files changed, 17 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/touchscreen/hideep.txt b/Documentation/devicetree/bindings/input/touchscreen/hideep.txt
> >> index a47c36190b01..68bb9f8dcc30 100644
> >> --- a/Documentation/devicetree/bindings/input/touchscreen/hideep.txt
> >> +++ b/Documentation/devicetree/bindings/input/touchscreen/hideep.txt
> >> @@ -17,6 +17,7 @@ Optional properties:
> >>  - linux,keycodes	: Specifies an array of numeric keycode values to
> >>  			be used for reporting button presses. The array can
> >>  			contain up to 3 entries.
> >> +- hideep,reset-work-mode: bool, reset touch report format to the native HiDeep protocol
> > 
> > Bindings must be a separate patch.
> > 
> > Also, would be nice to convert first the TXT to DT schema (YAML).
> > 
> > "-mode" suggests it's some enum, not bool. Otherwise what exactly it is
> > choosing (which mode)?
> 
> As it says it is resetting the mode to the native HiDeep protocol,
> we have no docs on the controller, so I have no idea what other
> values may be written to 0x081e other then 0 and what modes those
> values will enable.

We could either have property specify desired register value, or call
the property something like "hideep,force-native-protocol" if we want to
keep it a bool.

> 
> Anyways I just realized I should have not included this at all,
> since atm this new property is only used on X86/ACPI platforms
> (through platform code setting a device-property), so it is not
> used on devicetree platforms at all.

Even if such properties are not documented I do not see how it will
prevent people from using them... I guess if they validate DT they will
be caught, but I am not sure that we can rely on this happening.


Thanks.

-- 
Dmitry