Hi,

On 1/26/23 05:51, Jeff LaBundy wrote:

Hi Hans,

On Wed, Jan 25, 2023 at 11:54:15AM +0100, Hans de Goede wrote:

quoted

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.

Reviewed-by: Bastien Nocera <hadess@hadess.net>
Signed-off-by: Hans de Goede <redacted>

This LGTM, with one comment that I do not feel strongly about.

Reviewed-by: Jeff LaBundy <redacted>

quoted

---
 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 8a7351c4414c..22cee8d1bb22 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))

While harmless, this seems like a bit of an overuse of the proposed
functionality. Either the platform has a home button or it does not.
Would users really need to experiment with this property enabled or
disabled?

Thank you for the review.

Having this is actually useful, because users do need to check if
they actually have a home-button:

1. Always setting this may lead to false-positive home button presses
   on some models (IIRC, this has been around for a long time)

2. The home button typical is a windows logo printed on the front of
   cheap windows tablets below the screen. Recently I was adding info
   for yet another such cheap tablet and I asked the user to test
   the home-button since the windows logo was clearly there visually.
   But on this specific model touching the windows logo does not do
   anything.

Combined these 2 make it useful for users to be able to explicitly
test the home-button functionality.

Regards,

Hans

quoted

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

Kind regards,
Jeff LaBundy