diff mbox series

[resend,1/3] Input: touchscreen - Extend touchscreen_parse_properties() to allow overriding settings with a module option

Message ID 20230125105416.17406-2-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show
Series Input: touchscreen - settings module-param support | expand

Commit Message

Hans de Goede Jan. 25, 2023, 10:54 a.m. UTC 
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.

Reviewed-by: Bastien Nocera <hadess@hadess.net>
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(-)

Comments

Jeff LaBundy Jan. 26, 2023, 4:41 a.m. UTC | #1 
Hi Hans,

On Wed, Jan 25, 2023 at 11:54:14AM +0100, 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.
> 
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Thank you for resurrecting this series. Perhaps I can give my own $.02 as
a fellow customer of input.

I can appreciate the hesitancy that was expressed in the past, as this is
not a generic solution and is specific to touch devices. However, I also
agree with your point that extending dts overrides to all properties opens
a can of worms which should not necessarily gate this benign series (i.e.,
"good is not the enemy of great").

Personally I am highly in favor of this series because I myself have had
to support this very situation where a panel arrives 180 degrees from the
expected orientation, and fellow teammates who are not in a position to
quickly spin a build need a means to quickly unblock themselves through a
serial console or other means.

The code itself also LGTM and I verified there were no regressions on one
of my own drivers that expects these properties to come from dts, and so:

Reviewed-by: Jeff LaBundy <jeff@labundy.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.39.0
> 

Kind regards,
Jeff LaBundy
diff mbox series

Patch

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,