[PATCH 0/4] Input: touchscreen - Goodix bugfix + settings mod-param support

[PATCH 0/4] Input: touchscreen - Goodix bugfix + settings mod-param support

[Hans de Goede]

Hi Dmitry, et al,

The first patch in this set is a bugfix for the Goodix touchscreen driver,
it really is independent of the rest, but since the 4th patch will not
apply cleanly without this I have decided to send this as 1 big series.

If there are issues with the rest of the series please apply at least 1/4.

The rest of the series adds support for overriding various silead and
goodix touchscreen related device-properties from the kernel cmdline.
This is a reposting of an earlier version which had the following
cover letter:

On x86/ACPI platforms touchscreens mostly just work without needing any
device/model specific configuration. But in some cases (mostly with Silead
and Goodix touchscreens) it is still necessary to manually specify various
touchscreen-properties on a per model basis.

This is handled by drivers/platform/x86/touchscreen_dmi.c which contains
a large list of per-model touchscreen properties which it attaches to the
(i2c)device before the touchscreen driver's probe() method gets called.
This means that ATM changing these settings requires recompiling the
kernel. This makes figuring out what settings/properties a specific
touchscreen needs very hard for normal users to do.

This new settings parameter support is especially useful for helping
users to get their Silead touchscreens to work. These need to have
all of their settings (min-x, width, min-y, height, swapping, inverting)
hardcoded in touchscreen_dmi.c. The settings mod-param allows users
to find the right setting without them needing to be capable of
building their own kernel (and without a lot of rebooting) after which
they can ask someone with some kernel-dev experience to turn this into
a touchscreen_dmi.c patch for them (and yes the way this hw works sucks,
but we cannot change that).

An example of a user who was stuck getting their touchscreen to work and
who was helped by providing a kernel with the settings mod-param added:
https://github.com/onitake/gsl-firmware/discussions/193

Regards,

Hans


Hans de Goede (4):
  Input: goodix - Try resetting the controller when no config is set
  Input: touchscreen - Extend touchscreen_parse_properties() to allow
    overriding settings with a module option
  Input: silead - Add a settings module-parameter
  Input: goodix - Add a settings module-parameter

 drivers/input/touchscreen.c        | 103 ++++++++++++++++++++++++++---
 drivers/input/touchscreen/goodix.c |  18 ++++-
 drivers/input/touchscreen/silead.c |  26 +++++---
 include/linux/input/touchscreen.h  |  19 +++++-
 4 files changed, 142 insertions(+), 24 deletions(-)

-- 
2.37.3

[PATCH 1/4] Input: goodix - Try resetting the controller when no config is set

[Hans de Goede]

On ACPI systems (irq_pin_access_method == IRQ_PIN_ACCESS_ACPI_*) the driver
does not reset the controller at probe time, because sometimes the system
firmware loads a config and resetting might loose this config.

On the Nanote UMPC-01 device OTOH the config is in flash of the controller,
the controller needs a reset to load this; and the system firmware does not
reset the controller on a cold boot.

To fix the Nanote UMPC-01 touchscreen not working on a cold boot, try
resetting the controller and then re-reading the config when encountering
a config with 0 width/height/max_touch_num value and the controller has
not already been reset by goodix_ts_probe().

This should be safe to do in general because normally we should never
encounter a config with 0 width/height/max_touch_num. Doing this in
general not only avoids the need for a DMI quirk, but also might help
other systems.

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

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index a33cc7950cf5..c281e49826c2 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -1158,6 +1158,7 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
 	input_set_abs_params(ts->input_dev, ABS_MT_WIDTH_MAJOR, 0, 255, 0, 0);
 	input_set_abs_params(ts->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
 
+retry_read_config:
 	/* Read configuration and apply touchscreen parameters */
 	goodix_read_config(ts);
 
@@ -1165,6 +1166,16 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
 	touchscreen_parse_properties(ts->input_dev, true, &ts->prop);
 
 	if (!ts->prop.max_x || !ts->prop.max_y || !ts->max_touch_num) {
+		if (!ts->reset_controller_at_probe &&
+		    ts->irq_pin_access_method != IRQ_PIN_ACCESS_NONE) {
+			dev_info(&ts->client->dev, "Config not set, resetting controller\n");
+			/* Retry after a controller reset */
+			ts->reset_controller_at_probe = true;
+			error = goodix_reset(ts);
+			if (error)
+				return error;
+			goto retry_read_config;
+		}
 		dev_err(&ts->client->dev,
 			"Invalid config (%d, %d, %d), using defaults\n",
 			ts->prop.max_x, ts->prop.max_y, ts->max_touch_num);
-- 
2.37.3

[PATCH 2/4] Input: touchscreen - Extend touchscreen_parse_properties() to allow overriding settings with a module option

[Hans de Goede]

On x86/ACPI platforms touchscreens mostly just work without needing any
device/model specific configuration. But in some cases (mostly with Silead
and Goodix touchscreens) it is still necessary to manually specify various
touchscreen-properties on a per model basis.

This is handled by drivers/platform/x86/touchscreen_dmi.c which contains
a large list of per-model touchscreen properties which it attaches to the
(i2c)device before the touchscreen driver's probe() method gets called.
This means that ATM changing these settings requires recompiling the
kernel. This makes figuring out what settings/properties a specific
touchscreen needs very hard for normal users to do.

Add a new, optional, settings_override string argument to
touchscreen_parse_properties(), which takes a list of ; separated
property-name=value pairs, e.g. :
"touchscreen-size-x=1665;touchscreen-size-y=1140;touchscreen-swapped-x-y".

This new argument can be used by drivers to implement a module option which
allows users to easily specify alternative settings for testing.

The 2 new touchscreen_property_read_u32() and
touchscreen_property_read_bool() helpers are also exported so that
drivers can use these to add settings-override support to the code
for driver-specific properties.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Instead of patching all drivers rename touchscreen_parse_properties()
  to touchscreen_parse_properties_with_override() and add
  a static inline wrapper which passes NULL.
---
 drivers/input/touchscreen.c       | 103 ++++++++++++++++++++++++++----
 include/linux/input/touchscreen.h |  19 +++++-
 2 files changed, 109 insertions(+), 13 deletions(-)

diff --git a/drivers/input/touchscreen.c b/drivers/input/touchscreen.c
index 4620e20d0190..3b9505d5468d 100644
--- a/drivers/input/touchscreen.c
+++ b/drivers/input/touchscreen.c
@@ -12,15 +12,80 @@
 #include <linux/input/touchscreen.h>
 #include <linux/module.h>
 
+static int touchscreen_get_prop_from_settings_string(const char *settings,
+						     const char *propname,
+						     bool is_boolean,
+						     u32 *val_ret)
+{
+	char *begin, *end;
+	u32 val;
+
+	if (!settings)
+		return -ENOENT;
+
+	begin = strstr(settings, propname);
+	if (!begin)
+		return -ENOENT;
+
+	/* begin must be either the begin of settings, or be preceded by a ';' */
+	if (begin != settings && begin[-1] != ';')
+		return -EINVAL;
+
+	end = begin + strlen(propname);
+	if (*end != '=') {
+		if (is_boolean && (*end == '\0' || *end == ';')) {
+			*val_ret = true;
+			return 0;
+		}
+		return -EINVAL;
+	}
+
+	val = simple_strtoul(end + 1, &end, 0);
+	if (*end != '\0' && *end != ';')
+		return -EINVAL;
+
+	*val_ret = val;
+	return 0;
+}
+
+int touchscreen_property_read_u32(struct device *dev, const char *propname,
+				  const char *settings, u32 *val)
+{
+	int error;
+
+	error = device_property_read_u32(dev, propname, val);
+
+	if (touchscreen_get_prop_from_settings_string(settings, propname,
+						      false, val) == 0)
+		error = 0;
+
+	return error;
+}
+EXPORT_SYMBOL(touchscreen_property_read_u32);
+
+bool touchscreen_property_read_bool(struct device *dev, const char *propname,
+				    const char *settings)
+{
+	u32 val;
+
+	val = device_property_read_bool(dev, propname);
+
+	touchscreen_get_prop_from_settings_string(settings, propname, true, &val);
+
+	return val;
+}
+EXPORT_SYMBOL(touchscreen_property_read_bool);
+
 static bool touchscreen_get_prop_u32(struct device *dev,
 				     const char *property,
+				     const char *settings,
 				     unsigned int default_value,
 				     unsigned int *value)
 {
 	u32 val;
 	int error;
 
-	error = device_property_read_u32(dev, property, &val);
+	error = touchscreen_property_read_u32(dev, property, settings, &val);
 	if (error) {
 		*value = default_value;
 		return false;
@@ -50,20 +115,28 @@ static void touchscreen_set_params(struct input_dev *dev,
 }
 
 /**
- * touchscreen_parse_properties - parse common touchscreen properties
+ * touchscreen_parse_properties_with_settings - parse common touchscreen properties
  * @input: input device that should be parsed
  * @multitouch: specifies whether parsed properties should be applied to
  *	single-touch or multi-touch axes
  * @prop: pointer to a struct touchscreen_properties into which to store
  *	axis swap and invert info for use with touchscreen_report_x_y();
  *	or %NULL
+ * @settings: string with ; separated name=value pairs overriding
+ *	the device-properties or %NULL.
  *
  * This function parses common properties for touchscreens and sets up the
  * input device accordingly. The function keeps previously set up default
  * values if no value is specified.
+ *
+ * Callers can optional specify a settings string overriding the
+ * device-properties, this can be used to implement a module option which
+ * allows users to easily specify alternative settings for testing.
  */
-void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
-				  struct touchscreen_properties *prop)
+void touchscreen_parse_properties_with_settings(struct input_dev *input,
+						bool multitouch,
+						struct touchscreen_properties *prop,
+						const char *settings)
 {
 	struct device *dev = input->dev.parent;
 	struct input_absinfo *absinfo;
@@ -79,26 +152,32 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
 	axis_y = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
 
 	data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-x",
+						settings,
 						input_abs_get_min(input, axis_x),
 						&minimum);
 	data_present |= touchscreen_get_prop_u32(dev, "touchscreen-size-x",
+						 settings,
 						 input_abs_get_max(input,
 								   axis_x) + 1,
 						 &maximum);
 	data_present |= touchscreen_get_prop_u32(dev, "touchscreen-fuzz-x",
+						 settings,
 						 input_abs_get_fuzz(input, axis_x),
 						 &fuzz);
 	if (data_present)
 		touchscreen_set_params(input, axis_x, minimum, maximum - 1, fuzz);
 
 	data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-y",
+						settings,
 						input_abs_get_min(input, axis_y),
 						&minimum);
 	data_present |= touchscreen_get_prop_u32(dev, "touchscreen-size-y",
+						 settings,
 						 input_abs_get_max(input,
 								   axis_y) + 1,
 						 &maximum);
 	data_present |= touchscreen_get_prop_u32(dev, "touchscreen-fuzz-y",
+						 settings,
 						 input_abs_get_fuzz(input, axis_y),
 						 &fuzz);
 	if (data_present)
@@ -107,10 +186,12 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
 	axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
 	data_present = touchscreen_get_prop_u32(dev,
 						"touchscreen-max-pressure",
+						settings,
 						input_abs_get_max(input, axis),
 						&maximum);
 	data_present |= touchscreen_get_prop_u32(dev,
 						 "touchscreen-fuzz-pressure",
+						 settings,
 						 input_abs_get_fuzz(input, axis),
 						 &fuzz);
 	if (data_present)
@@ -122,28 +203,28 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
 	prop->max_x = input_abs_get_max(input, axis_x);
 	prop->max_y = input_abs_get_max(input, axis_y);
 
-	prop->invert_x =
-		device_property_read_bool(dev, "touchscreen-inverted-x");
+	prop->invert_x = touchscreen_property_read_bool(dev, "touchscreen-inverted-x",
+							settings);
 	if (prop->invert_x) {
 		absinfo = &input->absinfo[axis_x];
 		absinfo->maximum -= absinfo->minimum;
 		absinfo->minimum = 0;
 	}
 
-	prop->invert_y =
-		device_property_read_bool(dev, "touchscreen-inverted-y");
+	prop->invert_y = touchscreen_property_read_bool(dev, "touchscreen-inverted-y",
+							settings);
 	if (prop->invert_y) {
 		absinfo = &input->absinfo[axis_y];
 		absinfo->maximum -= absinfo->minimum;
 		absinfo->minimum = 0;
 	}
 
-	prop->swap_x_y =
-		device_property_read_bool(dev, "touchscreen-swapped-x-y");
+	prop->swap_x_y = touchscreen_property_read_bool(dev, "touchscreen-swapped-x-y",
+							settings);
 	if (prop->swap_x_y)
 		swap(input->absinfo[axis_x], input->absinfo[axis_y]);
 }
-EXPORT_SYMBOL(touchscreen_parse_properties);
+EXPORT_SYMBOL(touchscreen_parse_properties_with_settings);
 
 static void
 touchscreen_apply_prop_to_x_y(const struct touchscreen_properties *prop,
diff --git a/include/linux/input/touchscreen.h b/include/linux/input/touchscreen.h
index fe66e2b58f62..0023c6e368ba 100644
--- a/include/linux/input/touchscreen.h
+++ b/include/linux/input/touchscreen.h
@@ -17,8 +17,23 @@ struct touchscreen_properties {
 	bool swap_x_y;
 };
 
-void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
-				  struct touchscreen_properties *prop);
+void touchscreen_parse_properties_with_settings(struct input_dev *input,
+						bool multitouch,
+						struct touchscreen_properties *prop,
+						const char *settings);
+
+static inline void touchscreen_parse_properties(struct input_dev *input,
+						bool multitouch,
+						struct touchscreen_properties *prop)
+{
+	touchscreen_parse_properties_with_settings(input, multitouch, prop, NULL);
+}
+
+int touchscreen_property_read_u32(struct device *dev, const char *propname,
+				  const char *settings, u32 *val);
+
+bool touchscreen_property_read_bool(struct device *dev, const char *propname,
+				    const char *settings);
 
 void touchscreen_set_mt_pos(struct input_mt_pos *pos,
 			    const struct touchscreen_properties *prop,
-- 
2.37.3

[PATCH 3/4] Input: silead - Add a settings module-parameter

[Hans de Goede]

Add a settings module-parameter which can be allowed to specify/override
various device-properties.

Unlike most other touchscreen controllers Silead touchscreens don't tell us
the ranges of the reported x and y coordinates and Silead touchscreens also
often need to have their axis inverted and/or swapped to match the display
coordinates.

Being able to specify the necessary properties through a module parameter,
allows users of new device-models to help us with bringing up support for
new devices without them needing to do a local kernel-build just to modify
these settings.

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

diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
index 3eef8c01090f..3a3115702a13 100644
--- a/drivers/input/touchscreen/silead.c
+++ b/drivers/input/touchscreen/silead.c
@@ -58,6 +58,10 @@
 
 #define SILEAD_MAX_FINGERS	10
 
+static char *settings;
+module_param(settings, charp, 0444);
+MODULE_PARM_DESC(settings, "Override touchscreen settings using a ; separated key=value list, e.g. \"touchscreen-size-x=1665;touchscreen-size-y=1140;touchscreen-swapped-x-y\"");
+
 enum silead_ts_power {
 	SILEAD_POWER_ON  = 1,
 	SILEAD_POWER_OFF = 0
@@ -133,14 +137,15 @@ 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);
+	touchscreen_parse_properties_with_settings(data->input, true,
+						   &data->prop, settings);
 	silead_apply_efi_fw_min_max(data);
 
 	input_mt_init_slots(data->input, data->max_fingers,
 			    INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
 			    INPUT_MT_TRACK);
 
-	if (device_property_read_bool(dev, "silead,home-button"))
+	if (touchscreen_property_read_bool(dev, "silead,home-button", settings))
 		input_set_capability(data->input, EV_KEY, KEY_LEFTMETA);
 
 	data->input->name = SILEAD_TS_NAME;
@@ -173,7 +178,8 @@ static int silead_ts_request_pen_input_dev(struct silead_ts_data *data)
 	input_set_capability(data->pen_input, EV_KEY, BTN_TOUCH);
 	input_set_capability(data->pen_input, EV_KEY, BTN_TOOL_PEN);
 	set_bit(INPUT_PROP_DIRECT, data->pen_input->propbit);
-	touchscreen_parse_properties(data->pen_input, false, &data->prop);
+	touchscreen_parse_properties_with_settings(data->pen_input, false,
+						   &data->prop, settings);
 	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);
 
@@ -523,8 +529,8 @@ static int silead_ts_setup(struct i2c_client *client)
 	 * this.
 	 */
 
-	if (device_property_read_bool(&client->dev,
-				      "silead,stuck-controller-bug")) {
+	if (touchscreen_property_read_bool(&client->dev, "silead,stuck-controller-bug",
+					   settings)) {
 		pm_runtime_set_active(&client->dev);
 		pm_runtime_enable(&client->dev);
 		pm_runtime_allow(&client->dev);
@@ -591,8 +597,8 @@ static void silead_ts_read_props(struct i2c_client *client)
 	const char *str;
 	int error;
 
-	error = device_property_read_u32(dev, "silead,max-fingers",
-					 &data->max_fingers);
+	error = touchscreen_property_read_u32(dev, "silead,max-fingers", settings,
+					      &data->max_fingers);
 	if (error) {
 		dev_dbg(dev, "Max fingers read error %d\n", error);
 		data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
@@ -605,9 +611,9 @@ static void silead_ts_read_props(struct i2c_client *client)
 	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);
+	data->pen_supported = touchscreen_property_read_bool(dev, "silead,pen-supported", settings);
+	touchscreen_property_read_u32(dev, "silead,pen-resolution-x", settings, &data->pen_x_res);
+	touchscreen_property_read_u32(dev, "silead,pen-resolution-y", settings, &data->pen_y_res);
 }
 
 #ifdef CONFIG_ACPI
-- 
2.37.3

[PATCH 4/4] Input: goodix - Add a settings module-parameter

[Hans de Goede]

Add a settings module-parameter which can be allowed to specify/override
various device-properties.

Usuually Goodix touchscreens on x86 devices (where there is no devicetree
to specify the properties) just work. But in some cases the touchscreen is
mounted 90 or 180 degrees rotated vs the display, requiring setting the
touchscreen-x/y-inverted or touchscreen-swapped-x-y properties.

Being able to specify the necessary properties through a module parameter,
allows users of new device-models to help us with bringing up support for
new devices without them needing to do a local kernel-build just to modify
these settings.

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

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index c281e49826c2..36873f053f63 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -53,6 +53,10 @@
 #define ACPI_GPIO_SUPPORT
 #endif
 
+static char *settings;
+module_param(settings, charp, 0444);
+MODULE_PARM_DESC(settings, "Override touchscreen settings using a ; separated key=value list, e.g. \"touchscreen-fuzz-x=5;touchscreen-fuzz-y=5;touchscreen-swapped-x-y\"");
+
 struct goodix_chip_id {
 	const char *id;
 	const struct goodix_chip_data *data;
@@ -1163,7 +1167,8 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
 	goodix_read_config(ts);
 
 	/* Try overriding touchscreen parameters via device properties */
-	touchscreen_parse_properties(ts->input_dev, true, &ts->prop);
+	touchscreen_parse_properties_with_settings(ts->input_dev, true,
+						   &ts->prop, settings);
 
 	if (!ts->prop.max_x || !ts->prop.max_y || !ts->max_touch_num) {
 		if (!ts->reset_controller_at_probe &&
-- 
2.37.3

Re: [PATCH 1/4] Input: goodix - Try resetting the controller when no config is set

[Bastien Nocera]

On Tue, 2022-10-25 at 14:29 +0200, Hans de Goede wrote:
> On ACPI systems (irq_pin_access_method == IRQ_PIN_ACCESS_ACPI_*) the
> driver
> does not reset the controller at probe time, because sometimes the
> system
> firmware loads a config and resetting might loose this config.

"lose".

You can add
Reviewed-by: Bastien Nocera <hadess@hadess.net>
to all 4 patches in this patchset that don't already have it.

Cheers

Re: [PATCH 1/4] Input: goodix - Try resetting the controller when no config is set

[Hans de Goede]

Hi,

On 10/31/22 15:42, Bastien Nocera wrote:
> On Tue, 2022-10-25 at 14:29 +0200, Hans de Goede wrote:
>> On ACPI systems (irq_pin_access_method == IRQ_PIN_ACCESS_ACPI_*) the
>> driver
>> does not reset the controller at probe time, because sometimes the
>> system
>> firmware loads a config and resetting might loose this config.
> 
> "lose".
> 
> You can add
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> to all 4 patches in this patchset that don't already have it.

Thank you.

Dmitry, I assume you can fix the small spelling error in the
commit msg while applying this?

Regards,

Hans

Re: [PATCH 1/4] Input: goodix - Try resetting the controller when no config is set

[Dmitry Torokhov]

On Tue, Oct 25, 2022 at 02:29:27PM +0200, Hans de Goede wrote:
> On ACPI systems (irq_pin_access_method == IRQ_PIN_ACCESS_ACPI_*) the driver
> does not reset the controller at probe time, because sometimes the system
> firmware loads a config and resetting might loose this config.
> 
> On the Nanote UMPC-01 device OTOH the config is in flash of the controller,
> the controller needs a reset to load this; and the system firmware does not
> reset the controller on a cold boot.
> 
> To fix the Nanote UMPC-01 touchscreen not working on a cold boot, try
> resetting the controller and then re-reading the config when encountering
> a config with 0 width/height/max_touch_num value and the controller has
> not already been reset by goodix_ts_probe().
> 
> This should be safe to do in general because normally we should never
> encounter a config with 0 width/height/max_touch_num. Doing this in
> general not only avoids the need for a DMI quirk, but also might help
> other systems.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied, thank you.

-- 
Dmitry

Re: [PATCH 2/4] Input: touchscreen - Extend touchscreen_parse_properties() to allow overriding settings with a module option

[Dmitry Torokhov]

Hi Hans,

On Tue, Oct 25, 2022 at 02:29:28PM +0200, Hans de Goede wrote:
> On x86/ACPI platforms touchscreens mostly just work without needing any
> device/model specific configuration. But in some cases (mostly with Silead
> and Goodix touchscreens) it is still necessary to manually specify various
> touchscreen-properties on a per model basis.
> 
> This is handled by drivers/platform/x86/touchscreen_dmi.c which contains
> a large list of per-model touchscreen properties which it attaches to the
> (i2c)device before the touchscreen driver's probe() method gets called.
> This means that ATM changing these settings requires recompiling the
> kernel. This makes figuring out what settings/properties a specific
> touchscreen needs very hard for normal users to do.
> 
> Add a new, optional, settings_override string argument to
> touchscreen_parse_properties(), which takes a list of ; separated
> property-name=value pairs, e.g. :
> "touchscreen-size-x=1665;touchscreen-size-y=1140;touchscreen-swapped-x-y".
> 
> This new argument can be used by drivers to implement a module option which
> allows users to easily specify alternative settings for testing.
> 
> The 2 new touchscreen_property_read_u32() and
> touchscreen_property_read_bool() helpers are also exported so that
> drivers can use these to add settings-override support to the code
> for driver-specific properties.

I totally understand the motivation for this, but I do not think that
having special handling for only touchscreen properties is the right
thing to do. I would very much prefer is we had a more generic approach
of adding/overriding properties (via an swnode?).

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] Input: touchscreen - Extend touchscreen_parse_properties() to allow overriding settings with a module option

[Hans de Goede]

Hi Dmitry,

On 11/3/22 20:28, Dmitry Torokhov wrote:
> Hi Hans,
> 
> On Tue, Oct 25, 2022 at 02:29:28PM +0200, Hans de Goede wrote:
>> On x86/ACPI platforms touchscreens mostly just work without needing any
>> device/model specific configuration. But in some cases (mostly with Silead
>> and Goodix touchscreens) it is still necessary to manually specify various
>> touchscreen-properties on a per model basis.
>>
>> This is handled by drivers/platform/x86/touchscreen_dmi.c which contains
>> a large list of per-model touchscreen properties which it attaches to the
>> (i2c)device before the touchscreen driver's probe() method gets called.
>> This means that ATM changing these settings requires recompiling the
>> kernel. This makes figuring out what settings/properties a specific
>> touchscreen needs very hard for normal users to do.
>>
>> Add a new, optional, settings_override string argument to
>> touchscreen_parse_properties(), which takes a list of ; separated
>> property-name=value pairs, e.g. :
>> "touchscreen-size-x=1665;touchscreen-size-y=1140;touchscreen-swapped-x-y".
>>
>> This new argument can be used by drivers to implement a module option which
>> allows users to easily specify alternative settings for testing.
>>
>> The 2 new touchscreen_property_read_u32() and
>> touchscreen_property_read_bool() helpers are also exported so that
>> drivers can use these to add settings-override support to the code
>> for driver-specific properties.
> 
> I totally understand the motivation for this, but I do not think that
> having special handling for only touchscreen properties is the right
> thing to do. I would very much prefer is we had a more generic approach
> of adding/overriding properties (via an swnode?).

I understand where you are coming from, but I suspect the devicetree
folks are going to not like any generic solution for 2 reasons:

1. Allowing overriding devicetree properties like regulator voltage is a bad idea,
granted users can already do this with a custom DTB, but that is a higher
threshold to pass for a user then just adding something on the kernel cmdline

2. Devicetree supports devicetree-overlays and I expect the devicetree folks
to steer people who want to override random devicetree  properties in that
direction (or in the direction of using a custom DTB)

So the ACPI/x86 case really is somewhat special here and especially the
silead touchscreens are special here. Normally all the settings we are
talking here come from ACPI tables (or can directly be read from the
touchscreen controller) and then messing with these settings would be
a case of using an initrd with a custom ACPI DSDT, just like how
on devicetree I think we would expect people to use a custom DTB and
or a devicetree overlay.

but because of this info lacking from the ACPI tables we have it
hardcoded per 2-in-1/tablet model in:
drivers/platform/x86/touchscreen_dmi.c

The downside of this hardcoding is that testing new settings requires
building a custom kernel, which is both not helpful for having
a quick change settings -> test -> adjust settings cycle when trying
to find the right settings for a new model as well as quite a steep
hill to climb for novice users who want to get things to work on
a new model.

So I do believe that because of this the touchscreen properties
or special in this case and a somewhat custom solution to allow
setting just the touchscreen properties from the cmdline thus
is justified.

Also:

1. Having a mechanism specific to touchscreen properties is
simpler (more KISS) then having to come up with some more
complicated generic property override mechanism.

2. A touchscreen property specific mechanism is much less
susceptible to being misused. Setting the touchscreen properties
wrong cannot really result in any harm. OTOH setting the
max / end-of charging voltage of a lipo cell to 4.6 volt
(this is a real world example) is very much harmful.

The lipo-cell max charge voltage is something which we
in the sysfs interface deliberately disallow to be set any higher
then the boot-time value (lower is allowed). Adding a generic
cmdline mechanism for setting properties would allow
overriding this.

Regards,

Hans