Re: [PATCH v3 0/6] platform/surface: Add Surface Aggregator device registry

[Hans de Goede]

Hi,

On 2/12/21 12:54 PM, Maximilian Luz wrote:
> The Surface System Aggregator Module (SSAM) subsystem provides various
> functionalities, which are separated by spreading them across multiple
> devices and corresponding drivers. Parts of that functionality / some of
> those devices, however, can (as far as we currently know) not be
> auto-detected by conventional means. While older (specifically 5th- and
> 6th-)generation models do advertise most of their functionality via
> standard platform devices in ACPI, newer generations do not.
> 
> As we are currently also not aware of any feasible way to query said
> functionalities dynamically, this poses a problem. There is, however, a
> device in ACPI that seems to be used by Windows for identifying
> different Surface models: The Windows Surface Integration Device (WSID).
> This device seems to have a HID corresponding to the overall set of
> functionalities SSAM provides for the associated model.
> 
> This series introduces a device registry based on software nodes and
> device hubs to solve this problem. The registry is intended to contain
> all required non-detectable information.
> 
> The platform hub driver is loaded against the WSID device and
> instantiates and manages SSAM devices based on the information provided
> by the registry for the given WSID HID of the Surface model. All new
> devices created by this hub added as child devices to this hub.
> 
> In addition, a base hub is introduced to manage devices associated with
> the detachable base part of the Surface Book 3, as this requires special
> handling (i.e. devices need to be removed when the base is removed).
> Again, all devices created by the base hub (i.e. associated with the
> base) are added as child devices to this hub.
> 
> In total, this will yield the following device structure
> 
>   WSID
>    |- SSAM device 1 (physical device)
>    |- SSAM device 2 (physical device)
>    |- SSAM device 3 (physical device)
>    |- ...
>    \- SSAM base hub (virtual device)
>       |- SSAM base device 1 (physical device)
>       |- SSAM base device 2 (physical device)
>       |- ...
> 
> While software nodes seem to be well suited for this approach due to
> extensibility, they still need to be hard-coded, so I'm open for ideas
> on how this could be improved.

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> 
> Changes in v2:
>  - Fix Kconfig dependency
> 
> Changes in v3:
>  - Fix use of lockdep_assert_held()
> 
> Maximilian Luz (6):
>   platform/surface: Set up Surface Aggregator device registry
>   platform/surface: aggregator_registry: Add base device hub
>   platform/surface: aggregator_registry: Add battery subsystem devices
>   platform/surface: aggregator_registry: Add platform profile device
>   platform/surface: aggregator_registry: Add DTX device
>   platform/surface: aggregator_registry: Add HID subsystem devices
> 
>  MAINTAINERS                                   |   1 +
>  drivers/platform/surface/Kconfig              |  27 +
>  drivers/platform/surface/Makefile             |   1 +
>  .../surface/surface_aggregator_registry.c     | 641 ++++++++++++++++++
>  4 files changed, 670 insertions(+)
>  create mode 100644 drivers/platform/surface/surface_aggregator_registry.c
>