| Message ID | 20230125105416.17406-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: touchscreen - settings module-param support | expand |
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
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 >
> 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 --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