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@xxxxxxxxxx> 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
