Re: [PATCH 1/3] Input: touchscreen - Extend touchscreen_parse_properties() to allow overriding settings with a module option

From: Gregor Riepl <onitake@gmail.com>
To: Bastien Nocera <hadess@hadess.net>,
	Hans de Goede <hdegoede@redhat.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org
Subject: Re: [PATCH 1/3] Input: touchscreen - Extend touchscreen_parse_properties() to allow overriding settings with a module option
Date: Mon, 14 Jun 2021 11:16:17 +0200	[thread overview]
Message-ID: <278c2a63-2f7a-3fc4-24cb-fea8cb0d7e21@gmail.com> (raw)
In-Reply-To: <be664f7551029705030188f446799e1ff9ad9e03.camel@hadess.net>

>> Add a new, optional, settings_override string argument to
>> touchscreen_parse_properties(), which takes a list of ; separated
>> property-name=value pairs, e.g. :
>> "touchscreen-size-x=1665;touchscreen-size-y=1140;touchscreen-swapped-
>> x-y".

> I haven't reviewed the argument parsing code, but eep. If this were
> user-space code, we'd have exported it and tried to feed it all kind of
> garbage to see whether it parsed things properly, even if it's only run
> on the author's machine. Can't say that I like it.

I'm slightly surprised there isn't already something in the kernel that
can be used for such module argument parsing.

Would it be possible to decouple the settings parsing code from applying
them to the device completely? I.e.:

1. parse the settings string
2. store all detected settings in a static (struct) or dynamic
(dictionary) data strucure
3. apply device settings from DSDT/DT/etc., overriding the values that
were passed through module options

This is probably less efficient, but looks more robust to me.

Or how about simply adding all supported overrides as regular,
individual module options, maybe with a prefix? That way, there doesn't
need to be any additional parsing code.

The downside is that only a fixed set of options will be supported, but
I don't think this is a huge disadvantage. The main purpose of this
patch is to allow users to dynamically test touchscreen hardware that is
not properly factory-calibrated, so only a limited set of
hardware/drivers will be affected (I hope).