diff mbox series

[resend,2/3] Input: silead - Add a settings module-parameter

Message ID 20230125105416.17406-3-hdegoede@redhat.com
State New
Headers show
Series Input: touchscreen - settings module-param support | expand

Commit Message

Hans de Goede Jan. 25, 2023, 10:54 a.m. UTC
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 <hdegoede@redhat.com>
---
 drivers/input/touchscreen/silead.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

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

On Wed, Jan 25, 2023 at 11:54:15AM +0100, Hans de Goede wrote:
> 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 <hdegoede@redhat.com>

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

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

>  		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
Hans de Goede Jan. 26, 2023, 9:45 a.m. UTC | #2
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:
>> 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 <hdegoede@redhat.com>
> 
> This LGTM, with one comment that I do not feel strongly about.
> 
> Reviewed-by: Jeff LaBundy <jeff@labundy.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 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


> 
>>  		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
>
Gregor Riepl Jan. 26, 2023, 5:53 p.m. UTC | #3
> 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.

I vaguely remember that some users also reported Android-targeted 
tablets where the surface covering the soft-touch buttons was simply an 
extension of the whole touch surface, and not send any explicit key 
press events.

But I may be mistaken, or it was a bug in an older driver version that 
didn't support touch buttons well.
diff mbox series

Patch

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