Hi, On 6/14/21 11:16 AM, Gregor Riepl wrote: >>> 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. There are some functions which can parse a key=value string, but they mostly iterate over the string, they don't really build a dictionary struct. So you are expected to have this while processing the key-s one at a time, where as the drivers/input/touchscreen.c code is using device_property_read_foo calls which would require using a dictionary in between. Also these helpers are hard-coded to use comma as separator, which does not work for driver specific properties like "silead,home-button" and also they all expect there to always be a =value which is not how boolean properties work in e.g. devicetree. > 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 If there was common, proven, code to do 1. and 2. already then this would be a good approach, but given that there is no such code code this seems like it adds a lot of code, which will likely introduce quite a few bugs since it will all be untested new code. > This is probably less efficient, but looks more robust to me. How would this be more robust, this would require adding a whole bunch of mallocs + frees, which introduces a lot of chances to get things wrong where as the current approach is pretty KISS. Given the way how device-properties in the kernel are already using a module where the consumer queries them one at the time, extending this to also query an extra string provided through a module parameter is a natural way to extend this. Also I notice that both you and Bastien are weary of the robustness of my parsing code, but it is pretty simple and straight forward code, which does not make any changes to the input string at all. It could definitely do with a second pair of eyes taking a goo critical look at it. But AFAICT it actually is pretty robust and since the KISS way how it integrates with the existing parsing code, the overall solution, even though not very elegant does seem pretty 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. Doing this requires the following 5 lines of code: int/char* property_foo; module_param(property_foo, type, 0444); MODULE_PARM_DESC(property_foo, "description ..... .... .... .... ...."); if (property_foo_is_set()) ts_data->prop.property_foo = property_foo; For each property for each driver where we want to allow overrides. touchscreen_parse_properties() atm makes 11 device_property_read calls, so this would require the above code 11 times *per driver, which IMHO is not a good idea. Regards, Hans
