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@xxxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>
> This LGTM, with one comment that I do not feel strongly about.
>
> Reviewed-by: Jeff LaBundy <jeff@xxxxxxxxxxx>
>
>> ---
>> 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
>
