Re: [PATCH] platform/x86: touchscreen_dmi: Add support for setting touchscreen properties from cmdline

[Andy Shevchenko <andy@kernel.org>]

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