[PATCH v2 0/2] Input: hideep - 2 HiDeep touchscreen patches

[PATCH v2 0/2] Input: hideep - 2 HiDeep touchscreen patches

[Hans de Goede]

Hi Dmitry,

Here are 2 patches for the HiDeep touchscreen driver,
new in v2 is the addition of the second patch, patch 1/2
really is just a resend.

Regards,

Hans


Hans de Goede (2):
  Input: hideep - Silence error in SW_RESET()
  Input: hideep - Optionally reset controller work mode to native HiDeep
    protocol

 .../bindings/input/touchscreen/hideep.txt     |  1 +
 drivers/input/touchscreen/hideep.c            | 23 ++++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.39.1

[PATCH v2 1/2] Input: hideep - Silence error in SW_RESET()

[Hans de Goede]

On some models the first HIDEEP_SYSCON_WDT_CON write alone is enough to
cause the controller to reset, causing the second write to fail:

i2c-hideep_ts: write to register 0x52000014 (0x000001) failed: -121

Switch this write to a raw hideep_pgm_w_mem() to avoid an error getting
logged in this case.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/hideep.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/hideep.c b/drivers/input/touchscreen/hideep.c
index ff4bb4c14898..373c1269485f 100644
--- a/drivers/input/touchscreen/hideep.c
+++ b/drivers/input/touchscreen/hideep.c
@@ -271,9 +271,14 @@ static int hideep_pgm_w_reg(struct hideep_ts *ts, u32 addr, u32 val)
 
 #define SW_RESET_IN_PGM(clk)					\
 {								\
+	__be32 data = cpu_to_be32(0x01);			\
 	hideep_pgm_w_reg(ts, HIDEEP_SYSCON_WDT_CNT, (clk));	\
 	hideep_pgm_w_reg(ts, HIDEEP_SYSCON_WDT_CON, 0x03);	\
-	hideep_pgm_w_reg(ts, HIDEEP_SYSCON_WDT_CON, 0x01);	\
+	/*							\
+	 * The first write may already cause a reset, use a raw	\
+	 * write for the second write to avoid error logging.	\
+	 */							\
+	hideep_pgm_w_mem(ts, HIDEEP_SYSCON_WDT_CON, &data, 1);	\
 }
 
 #define SET_FLASH_PIO(ce)					\
-- 
2.39.1

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

[Hans de Goede]

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>
---
 .../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
 
 Example:
 
diff --git a/drivers/input/touchscreen/hideep.c b/drivers/input/touchscreen/hideep.c
index 373c1269485f..452914b78bf9 100644
--- a/drivers/input/touchscreen/hideep.c
+++ b/drivers/input/touchscreen/hideep.c
@@ -35,6 +35,7 @@
 #define HIDEEP_EVENT_ADDR		0x240
 
 /* command list */
+#define HIDEEP_WORK_MODE		0x081e
 #define HIDEEP_RESET_CMD		0x9800
 
 /* event bit */
@@ -964,6 +965,17 @@ static const struct attribute_group hideep_ts_attr_group = {
 	.attrs = hideep_ts_sysfs_entries,
 };
 
+static void hideep_set_work_mode(struct hideep_ts *ts)
+{
+	/*
+	 * Reset touch report format to the native HiDeep 20 protocol if requested.
+	 * This is necessary to make touchscreens which come up in I2C-HID mode
+	 * work with this driver.
+	 */
+	if (device_property_read_bool(&ts->client->dev, "hideep,reset-work-mode"))
+		regmap_write(ts->reg, HIDEEP_WORK_MODE, 0x00);
+}
+
 static int __maybe_unused hideep_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -987,6 +999,8 @@ static int __maybe_unused hideep_resume(struct device *dev)
 		return error;
 	}
 
+	hideep_set_work_mode(ts);
+
 	enable_irq(client->irq);
 
 	return 0;
@@ -1063,6 +1077,8 @@ static int hideep_probe(struct i2c_client *client)
 		return error;
 	}
 
+	hideep_set_work_mode(ts);
+
 	error = hideep_init_input(ts);
 	if (error)
 		return error;
-- 
2.39.1

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

[Krzysztof Kozlowski]

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)?



Best regards,
Krzysztof

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

[Hans de Goede]

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.

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.

And Rob has repeatedly requested to not add properties not
actually used in devicetree files (1) to the bindings.

So I'll just drop the bindings in the next version of this
patch-set.

Regards,

Hans


1) Like these x86 kinda kernel internal (it is also the kernel
setting the property) properties to devicetree bindings.

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

[Dmitry Torokhov]

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

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

[Hans de Goede]

Hi,

On 3/11/23 05:48, Dmitry Torokhov wrote:
> 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.

"hideep,force-native-protocol" is a good suggestion I'll prepare a new
version with that.

>> 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.

Right, but I have beene explicitly told multiple times (1) to not document
device-properties when they are only used between x86 platform code and
drivers consuming them (and thus not actually used in any DT files
at that point in time).

Regards,

Hans


1) By the DT bindings maintainers

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

[Krzysztof Kozlowski]

On 11/03/2023 12:16, Hans de Goede wrote:
> 
> "hideep,force-native-protocol" is a good suggestion I'll prepare a new
> version with that.
> 
>>> 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.
> 
> Right, but I have beene explicitly told multiple times (1) to not document
> device-properties when they are only used between x86 platform code and
> drivers consuming them (and thus not actually used in any DT files
> at that point in time).

Comment in the code that this is not a DT property, is the most we can
do now (so people won't use it later as argument to add to the binding)...

Best regards,
Krzysztof