On Wed, May 22, 2024 at 06:48:07PM +0200, 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.
>
> touchscreen_dmi is a special place for DMI quirks for this, but it can be
> challeging for users to figure out the right property values, especially
> for Silead touchscreens where non of these can be read back from the ctrl.
>
> ATM users can only test touchscreen properties by editing touchscreen_dmi.c
> and then building a completely new kernel which makes it unnecessary
> difficult for users to test and submit properties when necessary for their
> laptop / tablet model.
>
> Add support for specifying properties on the kernel commandline to allow
> users to easily figure out the right settings. See the added documentation
> in kernel-parameters.txt for the commandline syntax.
...
> + /*
> + * str is part of the static_command_line from init/main.c and poking
> + * holes in that by writing 0 to it is allowed, as is taking long
> + * lasting references to it.
> + */
> + ts_cmdline_data.acpi_name = strsep(&str, ",");
> +
> + for (i = 0; i < MAX_CMDLINE_PROPS; i++) {
> + name = strsep(&str, ",");
> + if (!name)
> + break;
> +
> + /* Replace '=' with 0 and make value point past '=' or NULL */
> + value = name;
> + strsep(&value, "=");
> + if (!value) {
> + ts_cmdline_props[i] = PROPERTY_ENTRY_BOOL(name);
> + } else if (isdigit(value[0])) {
> + ret = kstrtou32(value, 10, &u32val);
> + if (ret)
> + return ret;
> +
> + ts_cmdline_props[i] = PROPERTY_ENTRY_U32(name, u32val);
> + } else {
> + ts_cmdline_props[i] = PROPERTY_ENTRY_STRING(name, value);
> + }
> + }
This reminds me a lot from the next_arg(), can we not reinvent a wheel?
> +
> + if (!i)
> + return -EINVAL; /* No properties specified */
> +
> + if (str)
> + return -ENOSPC; /* More then MAX_CMDLINE_PROPS properties specified */
> +
> + ts_data = &ts_cmdline_data;
> + return 0;
> +}
...
> + struct ts_dmi_data *ts_data_dmi = NULL;
I prefer to see 'else' branch closer to the 'if' which will avoid a need to
look somewhere else in the code to answer the question "what if condition is
false".
...
> + if (!ts_data && !ts_data_dmi)
> return 0; /* Not an error */
This is basically a part of the below now:
> - ts_data = dmi_id->driver_data;
> + if (ts_data) {
> + /*
> + * Kernel cmdline provided data takes precedence, copy over
> + * DMI efi_embedded_fw info if available.
> + */
> + if (ts_data_dmi)
> + ts_data->embedded_fw = ts_data_dmi->embedded_fw;
> + } else {
> + ts_data = ts_data_dmi;
> + }
} else if (ts_data_dmi) {
ts_data = ts_data_dmi;
} else {
return 0; /* Not an error */
}
--
With Best Regards,
Andy Shevchenko
