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

[Hans de Goede <hdegoede@redhat.com>]

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