[PATCH resend 0/2] Input: silead - Pen support

[PATCH resend 0/2] Input: silead - Pen support

[Hans de Goede]

Hi Dmitry,

This series seems to have fallen through the cracks, so here
is a resend of this series.

Regards,

Hans


Hans de Goede (2):
  Input: silead - Add support for EFI-embedded fw using different
    min/max coordinates
  Input: silead - Add pen support

 drivers/input/touchscreen/silead.c | 177 ++++++++++++++++++++++++++++-
 1 file changed, 172 insertions(+), 5 deletions(-)

-- 
2.33.1

[PATCH resend 1/2] Input: silead - Add support for EFI-embedded fw using different min/max coordinates

[Hans de Goede]

Unfortunately, at the time of writing this commit message, we have been
unable to get permission from Silead, or from device OEMs, to distribute
the necessary Silead firmware files in linux-firmware.

On a whole bunch of devices the UEFI BIOS code contains a touchscreen
driver, which contains an embedded copy of the firmware. The fw-loader
code has a "platform" fallback mechanism, which together with info on the
firmware from drivers/platform/x86/touchscreen_dmi.c will use the firmware
from the UEFI driver when the firmware is missing from /lib/firmware. This
makes the touchscreen work OOTB without users needing to manually download
the firmware.

The firmware bundled with the original Windows/Android is usually newer
then the firmware in the UEFI driver and it is better calibrated. This
better calibration can lead to significant differences in the reported
min/max coordinates.

Add support for a new (optional) "silead,efi-fw-min-max" property which
provides a set of alternative min/max values to use for the x/y axis when
the EFI embedded firmware is used.

The new property is only used on (x86) devices which do not use devicetree,
IOW it is not used in actual devicetree files. The devicetree-bindings
maintainers have requested properties like these to not be added to the
devicetree-bindings, so the new property is deliberately not added to the
existing silead devicetree-bindings documentation.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/touchscreen/silead.c | 73 ++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index 1ee760bac0cf..caa25af53e6e 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -75,6 +75,8 @@ struct silead_ts_data {
 	struct input_mt_pos pos[SILEAD_MAX_FINGERS];
 	int slots[SILEAD_MAX_FINGERS];
 	int id[SILEAD_MAX_FINGERS];
+	u32 efi_fw_min_max[4];
+	bool efi_fw_min_max_set;
 };
 
 struct silead_fw_data {
@@ -82,6 +84,35 @@ struct silead_fw_data {
 	u32 val;
 };
 
+static void silead_apply_efi_fw_min_max(struct silead_ts_data *data)
+{
+	struct input_absinfo *absinfo_x = &data->input->absinfo[ABS_MT_POSITION_X];
+	struct input_absinfo *absinfo_y = &data->input->absinfo[ABS_MT_POSITION_Y];
+
+	if (!data->efi_fw_min_max_set)
+		return;
+
+	absinfo_x->minimum = data->efi_fw_min_max[0];
+	absinfo_x->maximum = data->efi_fw_min_max[1];
+	absinfo_y->minimum = data->efi_fw_min_max[2];
+	absinfo_y->maximum = data->efi_fw_min_max[3];
+
+	if (data->prop.invert_x) {
+		absinfo_x->maximum -= absinfo_x->minimum;
+		absinfo_x->minimum = 0;
+	}
+
+	if (data->prop.invert_y) {
+		absinfo_y->maximum -= absinfo_y->minimum;
+		absinfo_y->minimum = 0;
+	}
+
+	if (data->prop.swap_x_y) {
+		swap(absinfo_x->minimum, absinfo_y->minimum);
+		swap(absinfo_x->maximum, absinfo_y->maximum);
+	}
+}
+
 static int silead_ts_request_input_dev(struct silead_ts_data *data)
 {
 	struct device *dev = &data->client->dev;
@@ -97,6 +128,7 @@ static int silead_ts_request_input_dev(struct silead_ts_data *data)
 	input_set_abs_params(data->input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
 	input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
 	touchscreen_parse_properties(data->input, true, &data->prop);
+	silead_apply_efi_fw_min_max(data);
 
 	input_mt_init_slots(data->input, data->max_fingers,
 			    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
@@ -282,17 +314,48 @@ static int silead_ts_load_fw(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct silead_ts_data *data = i2c_get_clientdata(client);
-	unsigned int fw_size, i;
-	const struct firmware *fw;
+	const struct firmware *fw = NULL;
 	struct silead_fw_data *fw_data;
+	unsigned int fw_size, i;
 	int error;
 
 	dev_dbg(dev, "Firmware file name: %s", data->fw_name);
 
-	error = firmware_request_platform(&fw, data->fw_name, dev);
+	/*
+	 * Unfortunately, at the time of writing this comment, we have been unable to
+	 * get permission from Silead, or from device OEMs, to distribute the necessary
+	 * Silead firmware files in linux-firmware.
+	 *
+	 * On a whole bunch of devices the UEFI BIOS code contains a touchscreen driver,
+	 * which contains an embedded copy of the firmware. The fw-loader code has a
+	 * "platform" fallback mechanism, which together with info on the firmware
+	 * from drivers/platform/x86/touchscreen_dmi.c will use the firmware from the
+	 * UEFI driver when the firmware is missing from /lib/firmware. This makes the
+	 * touchscreen work OOTB without users needing to manually download the firmware.
+	 *
+	 * The firmware bundled with the original Windows/Android is usually newer then
+	 * the firmware in the UEFI driver and it is better calibrated. This better
+	 * calibration can lead to significant differences in the reported min/max
+	 * coordinates.
+	 *
+	 * To deal with this we first try to load the firmware without "platform"
+	 * fallback. If that fails we retry with "platform" fallback and if that
+	 * succeeds we apply an (optional) set of alternative min/max values from the
+	 * "silead,efi-fw-min-max" property.
+	 */
+	error = firmware_request_nowarn(&fw, data->fw_name, dev);
 	if (error) {
-		dev_err(dev, "Firmware request error %d\n", error);
-		return error;
+		error = firmware_request_platform(&fw, data->fw_name, dev);
+		if (error) {
+			dev_err(dev, "Firmware request error %d\n", error);
+			return error;
+		}
+
+		error = device_property_read_u32_array(dev, "silead,efi-fw-min-max",
+						       data->efi_fw_min_max,
+						       ARRAY_SIZE(data->efi_fw_min_max));
+		if (!error)
+			data->efi_fw_min_max_set = true;
 	}
 
 	fw_size = fw->size / sizeof(*fw_data);
-- 
2.33.1

Re: [PATCH resend 1/2] Input: silead - Add support for EFI-embedded fw using different min/max coordinates

[Dmitry Torokhov]

On Mon, Nov 22, 2021 at 11:06:36PM +0100, Hans de Goede wrote:
> Unfortunately, at the time of writing this commit message, we have been
> unable to get permission from Silead, or from device OEMs, to distribute
> the necessary Silead firmware files in linux-firmware.
> 
> On a whole bunch of devices the UEFI BIOS code contains a touchscreen
> driver, which contains an embedded copy of the firmware. The fw-loader
> code has a "platform" fallback mechanism, which together with info on the
> firmware from drivers/platform/x86/touchscreen_dmi.c will use the firmware
> from the UEFI driver when the firmware is missing from /lib/firmware. This
> makes the touchscreen work OOTB without users needing to manually download
> the firmware.
> 
> The firmware bundled with the original Windows/Android is usually newer
> then the firmware in the UEFI driver and it is better calibrated. This
> better calibration can lead to significant differences in the reported
> min/max coordinates.
> 
> Add support for a new (optional) "silead,efi-fw-min-max" property which
> provides a set of alternative min/max values to use for the x/y axis when
> the EFI embedded firmware is used.
> 
> The new property is only used on (x86) devices which do not use devicetree,
> IOW it is not used in actual devicetree files. The devicetree-bindings
> maintainers have requested properties like these to not be added to the
> devicetree-bindings, so the new property is deliberately not added to the
> existing silead devicetree-bindings documentation.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied, thank you.

-- 
Dmitry

[PATCH resend 2/2] Input: silead - Add pen support

[Hans de Goede]

Some Silead touchscreens have support for an active (battery powered)
pen, add support for this.

So far pen-support has only been seen on X86/ACPI (non devicetree) devs,
IOW it is not used in actual devicetree files. The devicetree-bindings
maintainers have requested properties like these to not be added to the
devicetree-bindings, so the new properties are deliberately not added
to the existing silead devicetree-bindings documentation.

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

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index caa25af53e6e..8b51ce15632e 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -67,6 +67,7 @@ struct silead_ts_data {
 	struct i2c_client *client;
 	struct gpio_desc *gpio_power;
 	struct input_dev *input;
+	struct input_dev *pen_input;
 	struct regulator_bulk_data regulators[2];
 	char fw_name[64];
 	struct touchscreen_properties prop;
@@ -77,6 +78,11 @@ struct silead_ts_data {
 	int id[SILEAD_MAX_FINGERS];
 	u32 efi_fw_min_max[4];
 	bool efi_fw_min_max_set;
+	bool pen_supported;
+	bool pen_down;
+	u32 pen_x_res;
+	u32 pen_y_res;
+	int pen_up_count;
 };
 
 struct silead_fw_data {
@@ -144,6 +150,45 @@ static int silead_ts_request_input_dev(struct silead_ts_data *data)
 	error = input_register_device(data->input);
 	if (error) {
 		dev_err(dev, "Failed to register input device: %d\n", error);
+			return error;
+	}
+
+	return 0;
+}
+
+static int silead_ts_request_pen_input_dev(struct silead_ts_data *data)
+{
+	struct device *dev = &data->client->dev;
+	int error;
+
+	if (!data->pen_supported)
+		return 0;
+
+	data->pen_input = devm_input_allocate_device(dev);
+	if (!data->pen_input)
+		return -ENOMEM;
+
+	input_set_abs_params(data->pen_input, ABS_X, 0, 4095, 0, 0);
+	input_set_abs_params(data->pen_input, ABS_Y, 0, 4095, 0, 0);
+	input_set_capability(data->pen_input, EV_KEY, BTN_TOUCH);
+	input_set_capability(data->pen_input, EV_KEY, BTN_TOOL_PEN);
+	/*
+	 * We never report BTN_STYLUS but userspace want to see this in order
+	 * for the device to be recognized as a pen / drawing-tablet.
+	 */
+	input_set_capability(data->pen_input, EV_KEY, BTN_STYLUS);
+	set_bit(INPUT_PROP_DIRECT, data->pen_input->propbit);
+	touchscreen_parse_properties(data->pen_input, false, &data->prop);
+	input_abs_set_res(data->pen_input, ABS_X, data->pen_x_res);
+	input_abs_set_res(data->pen_input, ABS_Y, data->pen_y_res);
+
+	data->pen_input->name = SILEAD_TS_NAME " pen";
+	data->pen_input->phys = "input/pen";
+	data->input->id.bustype = BUS_I2C;
+
+	error = input_register_device(data->pen_input);
+	if (error) {
+		dev_err(dev, "Failed to register pen input device: %d\n", error);
 		return error;
 	}
 
@@ -161,6 +206,45 @@ static void silead_ts_set_power(struct i2c_client *client,
 	}
 }
 
+static bool silead_ts_handle_pen_data(struct silead_ts_data *data, u8 *buf)
+{
+	u8 *coord = buf + SILEAD_POINT_DATA_LEN;
+	struct input_mt_pos pos;
+
+	if (!data->pen_supported || buf[2] != 0x00 || buf[3] != 0x00)
+		return false;
+
+	if (buf[0] == 0x00 && buf[1] == 0x00 && data->pen_down) {
+		data->pen_up_count++;
+		if (data->pen_up_count == 6) {
+			data->pen_down = false;
+			goto sync;
+		}
+		return true;
+	}
+
+	if (buf[0] == 0x01 && buf[1] == 0x08) {
+		touchscreen_set_mt_pos(&pos, &data->prop,
+			get_unaligned_le16(&coord[SILEAD_POINT_X_OFF]) & 0xfff,
+			get_unaligned_le16(&coord[SILEAD_POINT_Y_OFF]) & 0xfff);
+
+		input_report_abs(data->pen_input, ABS_X, pos.x);
+		input_report_abs(data->pen_input, ABS_Y, pos.y);
+
+		data->pen_up_count = 0;
+		data->pen_down = true;
+		goto sync;
+	}
+
+	return false;
+
+sync:
+	input_report_key(data->pen_input, BTN_TOOL_PEN, data->pen_down);
+	input_report_key(data->pen_input, BTN_TOUCH, data->pen_down);
+	input_sync(data->pen_input);
+	return true;
+}
+
 static void silead_ts_read_data(struct i2c_client *client)
 {
 	struct silead_ts_data *data = i2c_get_clientdata(client);
@@ -183,6 +267,9 @@ static void silead_ts_read_data(struct i2c_client *client)
 		buf[0] = data->max_fingers;
 	}
 
+	if (silead_ts_handle_pen_data(data, buf))
+		goto sync; /* Pen is down, release all previous touches */
+
 	touch_nr = 0;
 	bufp = buf + SILEAD_POINT_DATA_LEN;
 	for (i = 0; i < buf[0]; i++, bufp += SILEAD_POINT_DATA_LEN) {
@@ -225,6 +312,7 @@ static void silead_ts_read_data(struct i2c_client *client)
 			data->pos[i].y, data->id[i], data->slots[i]);
 	}
 
+sync:
 	input_mt_sync_frame(input);
 	input_report_key(input, KEY_LEFTMETA, softbutton_pressed);
 	input_sync(input);
@@ -356,6 +444,14 @@ static int silead_ts_load_fw(struct i2c_client *client)
 						       ARRAY_SIZE(data->efi_fw_min_max));
 		if (!error)
 			data->efi_fw_min_max_set = true;
+
+		/* The EFI (platform) embedded fw does not have pen support */
+		if (data->pen_supported) {
+			dev_warn(dev, "Warning loading '%s' from filesystem failed, using EFI embedded copy.\n",
+				 data->fw_name);
+			dev_warn(dev, "Warning pen support is known to be broken in the EFI embedded fw version\n");
+			data->pen_supported = false;
+		}
 	}
 
 	fw_size = fw->size / sizeof(*fw_data);
@@ -513,6 +609,10 @@ static void silead_ts_read_props(struct i2c_client *client)
 			 "silead/%s", str);
 	else
 		dev_dbg(dev, "Firmware file name read error. Using default.");
+
+	data->pen_supported = device_property_read_bool(dev, "silead,pen-supported");
+	device_property_read_u32(dev, "silead,pen-resolution-x", &data->pen_x_res);
+	device_property_read_u32(dev, "silead,pen-resolution-y", &data->pen_y_res);
 }
 
 #ifdef CONFIG_ACPI
@@ -625,6 +725,10 @@ static int silead_ts_probe(struct i2c_client *client,
 	if (error)
 		return error;
 
+	error = silead_ts_request_pen_input_dev(data);
+	if (error)
+		return error;
+
 	error = devm_request_threaded_irq(dev, client->irq,
 					  NULL, silead_ts_threaded_irq_handler,
 					  IRQF_ONESHOT, client->name, data);
-- 
2.33.1

Re: [PATCH resend 2/2] Input: silead - Add pen support

[Dmitry Torokhov]

Hi Hans,

On Mon, Nov 22, 2021 at 11:06:37PM +0100, Hans de Goede wrote:
> +	input_set_capability(data->pen_input, EV_KEY, BTN_TOUCH);
> +	input_set_capability(data->pen_input, EV_KEY, BTN_TOOL_PEN);
> +	/*
> +	 * We never report BTN_STYLUS but userspace want to see this in order
> +	 * for the device to be recognized as a pen / drawing-tablet.
> +	 */

What userspace is that? I see that udev recognizes devices with either
stylus or pen  as tablets since at least 2015.

I am really hesitant adding synthetic capabilities that do not have real
events behind them.

Thanks.

-- 
Dmitry

Re: [PATCH resend 2/2] Input: silead - Add pen support

[Hans de Goede]

Hi Dmitry,

On 12/11/21 04:07, Dmitry Torokhov wrote:
> Hi Hans,
> 
> On Mon, Nov 22, 2021 at 11:06:37PM +0100, Hans de Goede wrote:
>> +	input_set_capability(data->pen_input, EV_KEY, BTN_TOUCH);
>> +	input_set_capability(data->pen_input, EV_KEY, BTN_TOOL_PEN);
>> +	/*
>> +	 * We never report BTN_STYLUS but userspace want to see this in order
>> +	 * for the device to be recognized as a pen / drawing-tablet.
>> +	 */
> 
> What userspace is that? I see that udev recognizes devices with either
> stylus or pen  as tablets since at least 2015.
> 
> I am really hesitant adding synthetic capabilities that do not have real
> events behind them.

You are completely right, I added this when GNOME3 / libinput would not
recognize the pen (looking at other stylus/pen drivers) but IIRC things
then still did not work and then I also added code to set the resolution.

I just tested without setting BTN_STYLUS and things still work fine,
so the comment and the line setting BTN_STYLUS can be dropped while
merging this. Let me know if you want me to do a new version with this
dropped instead.

Regards,

Hans

Re: [PATCH resend 2/2] Input: silead - Add pen support

[Dmitry Torokhov]

On Sun, Dec 12, 2021 at 01:36:04PM +0100, Hans de Goede wrote:
> Hi Dmitry,
> 
> On 12/11/21 04:07, Dmitry Torokhov wrote:
> > Hi Hans,
> > 
> > On Mon, Nov 22, 2021 at 11:06:37PM +0100, Hans de Goede wrote:
> >> +	input_set_capability(data->pen_input, EV_KEY, BTN_TOUCH);
> >> +	input_set_capability(data->pen_input, EV_KEY, BTN_TOOL_PEN);
> >> +	/*
> >> +	 * We never report BTN_STYLUS but userspace want to see this in order
> >> +	 * for the device to be recognized as a pen / drawing-tablet.
> >> +	 */
> > 
> > What userspace is that? I see that udev recognizes devices with either
> > stylus or pen  as tablets since at least 2015.
> > 
> > I am really hesitant adding synthetic capabilities that do not have real
> > events behind them.
> 
> You are completely right, I added this when GNOME3 / libinput would not
> recognize the pen (looking at other stylus/pen drivers) but IIRC things
> then still did not work and then I also added code to set the resolution.
> 
> I just tested without setting BTN_STYLUS and things still work fine,
> so the comment and the line setting BTN_STYLUS can be dropped while
> merging this. Let me know if you want me to do a new version with this
> dropped instead.

Adjusted and applied, thank you.

-- 
Dmitry