Hi Andy,
Thank you for the review.
On 5/22/24 7:19 PM, Andy Shevchenko wrote:
> 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?
next_arg is meant for parsing different arguments on the kernel cmdline
split by spaces. It has space as separator hardcoded so it cannot be
used here.
>
>> +
>> + 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".
Ok.
> ...
>
>> + 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 */
> }
>
Yes that is better I'll fix that for v2.
>From your other reply:
>>> + ret = kstrtou32(value, 10, &u32val);
>
> One more thing, why to limit to the decimal only?
> Should be up to user to choose. And some properties (now or in the future)
> actually might benefit from being able to be entered in hexadecimal form.
That is a good point I'll replace the "10" with "0" for v2.
Regards,
Hans
