On Sun, Dec 06, 2020 at 09:41:21AM +0100, Hans de Goede wrote: > Hi Leon, > > On 12/6/20 8:07 AM, Leon Romanovsky wrote: > > On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote: > >> Hello, > >> > >> Here is version two of the Surface System Aggregator Module (SAM/SSAM) > >> driver series, adding initial support for the embedded controller on 5th > >> and later generation Microsoft Surface devices. Initial support includes > >> the ACPI interface to the controller, via which battery and thermal > >> information is provided on some of these devices. > >> > >> The previous version and cover letter detailing what this series is > >> about can be found at > >> > >> https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@xxxxxxxxx/ > >> > >> This patch-set can also be found at the following repository and > >> reference, if you prefer to look at a kernel tree instead of these > >> emails: > >> > >> https://github.com/linux-surface/kernel tags/s/surface-aggregator/v2 > >> > >> Thank you all for the feedback to v1, I hope I have addressed all > >> comments. > > > > > > I think that it is too far fetched to attempt and expose UAPI headers > > for some obscure char device that we are all know won't be around in > > a couple of years from now due to the nature of how this embedded world > > works. > > This is not for an embedded device, but for the popular line of > Microsoft Surface laptops / 2-in-1s... It is the naming, we don't have char device for every "laptop" vendor. Why is Microsoft different here? > > > 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. USB devices implement standard protocol, this surface hub is nothing even close to that. > > 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. I don't know how it goes in device/platform area, but in other large subsystems, UAPI should be presented with working user-space part. > > 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. > > > Also the idea that you are creating new bus just for this device doesn't > > really sound right. I recommend you to take a look on auxiliary bus and > > use it or come with very strong justifications why it is not fit yet. > > AFAIK the auxiliary bus is for sharing a single device between multiple > drivers, while the main device-driver also still offers functionality > (beyond the providing of access) itself. The idea behind auxiliary bus is to slice various functionalities into different sub-drivers, see it as a way to create subsystem inside one driver. > > This is more akin to how the WMI driver also models different WMI > functions as a bus + devices on the bus. > > Or how the SDIO driver multiplex a single SDIO device into its > functions by again using a bus + devices on the bus model. > > Also this has been in the works for quite a while now, the Linux on > Microsoft Surface devices community has been working on this out of > tree for a long time, see: > https://github.com/linux-surface/ It is not relevant, the code is merged than it is ready. > > And an RFC and a v1 have been posted a while ago, while auxiliary > bus support is not even in the mainline kernel yet. I would agree > with you that this should switch to auxiliary bus, despite the timing, > if that would lead to much better code. But ATM I don't really see > switching to auxiliary bus offering much benefits here. The auxiliary bus is merged and part of linux-next. https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/tag/?h=auxbus-5.11-rc1 > > > I'm sorry to say, but this series is not ready to be merged yet. > > > > NAK: Leon Romanovsky <leon@xxxxxxxxxx> > > See above, I believe that this all is a bit harsh and I have not > really heard convincing arguments for not merging this. > > Moreover such a quick nack does not really promote working upstream, > where as we actually want people to work upstream as much as possible. > I know this is not a reason for taking bad code, but I'm not > convinced that this is bad code. I naked explicitly for two reasons: UAPI(chardev) and lack of rationale for the custom bus, never said "bad code". > > I have not reviewed this myself yet, but once I have reviewed > this and any review remarks have been addressed I do expect to > merge this series through the platform-drivers-x86 tree. > > Regards, > > Hans de Goede > (drivers/platform/x86 and drivers/platform/surface subsys maintainer) >
