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

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

[Hans de Goede]

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.

Cc: Gregor Riepl <onitake@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note assuming this gets favourable review(s) in a reasonable timeframe
I'm thinking about maybe even adding this to 6.10 as a fix since users
not being able to easily test Silead touchscreen settings has been an
issue for quite a while now. Without the cmdline option being used this
is a no-op so the chance of this causing regressions is close to 0.
---
 .../admin-guide/kernel-parameters.txt         | 22 ++++++
 drivers/platform/x86/touchscreen_dmi.c        | 76 ++++++++++++++++++-
 2 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 396137ee018d..9d04fc8d191f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1899,6 +1899,28 @@
 				Format:
 				<bus_id>,<clkrate>
 
+	i2c_touchscreen_props= [HW,ACPI,X86]
+			Set device-properties for ACPI enumerated I2C attached
+			touchscreen, to e.g. fix coordinates of upside-down
+			mounted touchscreens. If you need this option please
+			submit a drivers/platform/x86/touchscreen_dmi.c patch
+			adding a DMI quirk for this.
+
+			Format:
+			<ACPI_HW_ID>,<prop_name>=<val>[,prop_name=val][,...]
+			Where <val> is one of:
+			Omit "=<val>" entirely	Set a boolean device-property
+			Unsigned number		Set a u32 device-property
+			Anything else		Set a string device-property
+
+			Examples (split over multiple lines):
+			i2c_touchscreen_props=GDIX1001,touchscreen-inverted-x,
+			touchscreen-inverted-y
+
+			i2c_touchscreen_props=MSSL1680,touchscreen-size-x=1920,
+			touchscreen-size-y=1080,touchscreen-inverted-y,
+			firmware-name=gsl1680-vendor-model.fw
+
 	i8042.debug	[HW] Toggle i8042 debug mode
 	i8042.unmask_kbd_data
 			[HW] Enable printing of interrupt data from the KBD port
diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index c6a10ec2c83f..63400ef6d90d 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -9,10 +9,13 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/ctype.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
 #include <linux/efi_embedded_fw.h>
 #include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kstrtox.h>
 #include <linux/notifier.h>
 #include <linux/property.h>
 #include <linux/string.h>
@@ -1817,7 +1820,7 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
 	{ }
 };
 
-static const struct ts_dmi_data *ts_data;
+static struct ts_dmi_data *ts_data;
 
 static void ts_dmi_add_props(struct i2c_client *client)
 {
@@ -1852,20 +1855,87 @@ static int ts_dmi_notifier_call(struct notifier_block *nb,
 	return 0;
 }
 
+#define MAX_CMDLINE_PROPS 16
+
+static struct property_entry ts_cmdline_props[MAX_CMDLINE_PROPS + 1];
+
+static struct ts_dmi_data ts_cmdline_data = {
+	.properties = ts_cmdline_props,
+};
+
+static int __init ts_parse_props(char *str)
+{
+	char *name, *value;
+	u32 u32val;
+	int i, ret;
+
+	/*
+	 * 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);
+		}
+	}
+
+	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;
+}
+__setup("i2c_touchscreen_props=", ts_parse_props);
+
 static struct notifier_block ts_dmi_notifier = {
 	.notifier_call = ts_dmi_notifier_call,
 };
 
 static int __init ts_dmi_init(void)
 {
+	struct ts_dmi_data *ts_data_dmi = NULL;
 	const struct dmi_system_id *dmi_id;
 	int error;
 
 	dmi_id = dmi_first_match(touchscreen_dmi_table);
-	if (!dmi_id)
+	if (dmi_id)
+		ts_data_dmi = dmi_id->driver_data;
+
+	if (!ts_data && !ts_data_dmi)
 		return 0; /* Not an error */
 
-	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;
+	}
+
 	/* Some dmi table entries only provide an efi_embedded_fw_desc */
 	if (!ts_data->properties)
 		return 0;
-- 
2.45.1

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

On Wed, May 22, 2024 at 08:19:07PM +0300, Andy Shevchenko wrote:
> On Wed, May 22, 2024 at 06:48:07PM +0200, Hans de Goede wrote:

...

> > +			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.

> > +			if (ret)
> > +				return ret;

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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

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

[Andy Shevchenko]

On Wed, May 22, 2024 at 9:40 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/22/24 7:19 PM, Andy Shevchenko wrote:
> > On Wed, May 22, 2024 at 06:48:07PM +0200, Hans de Goede wrote:

...

> >> +    /*
> >> +     * 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.

I believe it's not the first time I hear such an excuse for
duplicating Yet Another (Same) Parser.
If you think you really need another separator, we may patch
next_arg() or add next_arg_any(is_separator_fn *fn) and make
next_arg() to be a wrapper of the other one.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, May 22, 2024 at 10:32 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, May 22, 2024 at 9:40 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 5/22/24 7:19 PM, Andy Shevchenko wrote:
> > > On Wed, May 22, 2024 at 06:48:07PM +0200, Hans de Goede wrote:

...

> > 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.
>
> I believe it's not the first time I hear such an excuse for
> duplicating Yet Another (Same) Parser.
> If you think you really need another separator, we may patch
> next_arg() or add next_arg_any(is_separator_fn *fn) and make
> next_arg() to be a wrapper of the other one.

Also note, that it will allow (AFAIU) to have something like
xy="xy,inverted" to be passed if needed in the future.


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi Andy,

On 5/22/24 9:32 PM, Andy Shevchenko wrote:
> On Wed, May 22, 2024 at 9:40 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 5/22/24 7:19 PM, Andy Shevchenko wrote:
>>> On Wed, May 22, 2024 at 06:48:07PM +0200, Hans de Goede wrote:
> 
> ...
> 
>>>> +    /*
>>>> +     * 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.
> 
> I believe it's not the first time I hear such an excuse for
> duplicating Yet Another (Same) Parser.
> If you think you really need another separator, we may patch
> next_arg() or add next_arg_any(is_separator_fn *fn) and make
> next_arg() to be a wrapper of the other one.

The kernel already has a generic parser for most things in the form of
include/linux/parser.h but that will not work here since that assumes
a list of fixed keywords while in this case I want to allow any keyword
and change it into a device-property with that name.

Also the actual splitting into key=value code here is maybe 5 lines,
the whole patch itself is not that big and most of the parsing is
figuring out if value represents a bool, uint or string.

And the kind of refactoring of next_arg() you are asking for here
is way out of scope, so sorry but I don't plan to change this part
of the patch.

Regards,

Hans

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

[Andy Shevchenko]

On Thu, May 23, 2024 at 11:47 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/22/24 9:32 PM, Andy Shevchenko wrote:
> > On Wed, May 22, 2024 at 9:40 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 5/22/24 7:19 PM, Andy Shevchenko wrote:
> >>> On Wed, May 22, 2024 at 06:48:07PM +0200, Hans de Goede wrote:

...

> >>>> +    /*
> >>>> +     * 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.
> >
> > I believe it's not the first time I hear such an excuse for
> > duplicating Yet Another (Same) Parser.
> > If you think you really need another separator, we may patch
> > next_arg() or add next_arg_any(is_separator_fn *fn) and make
> > next_arg() to be a wrapper of the other one.
>
> The kernel already has a generic parser for most things in the form of
> include/linux/parser.h but that will not work here since that assumes
> a list of fixed keywords while in this case I want to allow any keyword
> and change it into a device-property with that name.

I know about that and that's why I haven't suggested it.

> Also the actual splitting into key=value code here is maybe 5 lines,
> the whole patch itself is not that big and most of the parsing is
> figuring out if value represents a bool, uint or string.

Yeah, 5 strings here, 5 bugs there and duplicating all over in the
kernel in zillions copies. This is not good. We should not duplicate
things, we should deduplicate them. And as I said, your excuse is
being repeated not the first time. This is also not good.

> And the kind of refactoring of next_arg() you are asking for here
> is way out of scope,

Of course, of course, but why introduce Yet Another Parser to begin with?

> so sorry but I don't plan to change this part
> of the patch.

This is not good.
But I have no power to stop it, while being very sad about this attitude.


-- 
With Best Regards,
Andy Shevchenko

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

[Gregor Riepl]

>> And the kind of refactoring of next_arg() you are asking for here
>> is way out of scope,
> 
> Of course, of course, but why introduce Yet Another Parser to begin with?
> 
>> so sorry but I don't plan to change this part
>> of the patch.
> 
> This is not good.
> But I have no power to stop it, while being very sad about this attitude.

Andy, I do agree with you, but what would you suggest we can do about the situation?

Write a generic parser that everyone should use from now on, and then migrate all usage to this parser?

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

[Andy Shevchenko]

On Sat, May 25, 2024 at 8:41 PM Gregor Riepl <onitake@gmail.com> wrote:
>
> >> And the kind of refactoring of next_arg() you are asking for here
> >> is way out of scope,
> >
> > Of course, of course, but why introduce Yet Another Parser to begin with?
> >
> >> so sorry but I don't plan to change this part
> >> of the patch.
> >
> > This is not good.
> > But I have no power to stop it, while being very sad about this attitude.
>
> Andy, I do agree with you, but what would you suggest we can do about the situation?

Ask for more help. See below as well.

> Write a generic parser that everyone should use from now on, and then migrate all usage to this parser?

Yeah, that's, if you noticed, what I'm doing (see Git history of
lib/*cmdline*, for example and gpio-aggregator.c) and _that's_ why I'm
not happy about the situation. What I'm trying to say is that we need
to change people's mind about double efforts and duplication.

-- 
With Best Regards,
Andy Shevchenko

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

[Randy Dunlap]

Hi,

On 5/22/24 9:48 AM, Hans de Goede wrote:
> +static int __init ts_parse_props(char *str)
> +{
> +	char *name, *value;
> +	u32 u32val;
> +	int i, ret;
> +
> +	/*
> +	 * 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);
> +		}
> +	}
> +
> +	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;
> +}
> +__setup("i2c_touchscreen_props=", ts_parse_props);

__setup() is different from early_param() & its family.
__setup() functions return 1 for "handled" and 0 for "not handled".

See include/linux/init.h.

-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html

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

[Hans de Goede]

Hi Randy,

On 5/22/24 9:01 PM, Randy Dunlap wrote:
> Hi,
> 
> On 5/22/24 9:48 AM, Hans de Goede wrote:
>> +static int __init ts_parse_props(char *str)
>> +{
>> +	char *name, *value;
>> +	u32 u32val;
>> +	int i, ret;
>> +
>> +	/*
>> +	 * 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);
>> +		}
>> +	}
>> +
>> +	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;
>> +}
>> +__setup("i2c_touchscreen_props=", ts_parse_props);
> 
> __setup() is different from early_param() & its family.
> __setup() functions return 1 for "handled" and 0 for "not handled".
> 
> See include/linux/init.h.

Thank you for pointing this out, I looked at parse_args() which
expects the parse_unknown_fn handler to return 0 on success or
negative errno on error. But I see know there is a layer in between
which indeed works as you point out. I'll fix this for v2 and add
a pr_warn() about invalid syntax to the parser since the core does
not do this.

Regards,

Hans