Messages in this thread | | | Subject | Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module | From | Maximilian Luz <> | Date | Sun, 6 Dec 2020 11:56:26 +0100 |
| |
On 12/6/20 11:43 AM, Hans de Goede wrote: > Hi, > > On 12/6/20 11:33 AM, Maximilian Luz wrote: >> On 12/6/20 10:06 AM, Leon Romanovsky wrote:> On Sun, Dec 06, 2020 at 05:58:32PM +0900, Blaž Hrastnik wrote: >>>>> >>>>>> More on that, the whole purpose of proposed interface is to debug and >>>>>> not intended to be used by any user space code. >>>>> >>>>> The purpose is to provide raw access to the Surface Serial Hub protocol, >>>>> just like we provide raw access to USB devices and have hidraw devices. >>>>> >>>>> So this goes a litle beyond just debugging; and eventually the choice >>>>> may be made to implement some functionality with userspace drivers, >>>>> just like we do for some HID and USB devices. >>>>> >>>>> Still I agree with you that adding new userspace API is something which >>>>> needs to be considered carefully. So I will look at this closely when >>>>> reviewing this set. >>>> >>>> To add to that: this was previously a debugfs interface but was moved to misc after review on the initial RFC: >>>> https://lkml.org/lkml/2020/9/24/96 >>> >>> There is a huge difference between the suggestion and final implementation. >>> >>> Greg suggested to add new debug module to the drivers/misc that will >>> open char device explicitly after user loaded that module to debug this >>> hub. However, the author added full blown char device as a first citizen >>> that has all not-break-user constrains. >> >> This module still needs to be loaded explicitly. > > Good then I really do not see a problem with this. > >> And (I might be wrong >> about this) the "not-break-user constraints" hold as soon as I register >> a misc device at all, no? > > Correct. > >> So I don't see how this is a) any different >> than previously discussed with Greg and b) how the uapi header now >> introduces any not-break-user constraints that would not be there >> without it. >> >> This interface is intended as a stable interface. That's something that >> I committed to as soon as I decided to implement this via a misc-device. >> >> Sure, I can move the definitions in the uapi header to the module >> itself, but I don't see any benefit in that. > > Right, if we are going to use a misc chardev for this, then the > correct thing to do is to put the API bits for that chardev under > include/uapi. > > It would still be good if you can provide a pointer to some userspace > tools using this new API; and for the next version maybe add that > pointer to the commit message
Right, I will add that to the commit message. I just linked you the scripts in my other response, but here again for completeness:
https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam
While I'm not using the header directly (the scripts are written in python) I still think uapi is the right place to put this (please correct me if I'm wrong). Not putting them there seems to be needless obfuscating to me.
Regards, Max
| |