Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Arnd Bergmann]

On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> On 9/23/20 9:43 PM, Arnd Bergmann wrote:
> > On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> >>
> >> On 9/23/20 5:30 PM, Arnd Bergmann wrote:
> >>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> >>>>
> >>>> Hello,
> >>>>
> >>>> The Surface System Aggregator Module (we'll refer to it as Surface
> >>>> Aggregator or SAM below) is an embedded controller (EC) found on various
> >>>> Microsoft Surface devices. Specifically, all 4th and later generation
> >>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
> >>>> exception of the Surface Go series and the Surface Duo. Notably, it
> >>>> seems like this EC can also be found on the ARM-based Surface Pro X [1].
> >>>
> >>> I think this should go to drivers/platform/x86 or drivers/platform/surface/
> >>> along with other laptop vendor specific code rather than drivers/misc/.
> >>
> >> I initially had this under drivers/platform/x86. There are two main
> >> reasons I changed that: First, I think it's a bit too big for
> >> platform/x86 given that it basically introduces a new subsystem. At this
> >> point it's really less of "a couple of odd devices here and there" and
> >> more of a bus-type thing. Second, with the possibility of future support
> >> for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I
> >> thought that platform/x86 would not be a good fit.
> >
> > I don't see that as a strong reason against it. As you write yourself, the
> > driver won't work on the arm machines without major changes anyway,
> > and even if it does, it fits much better with the rest of it.
>
> Sorry, I should have written that a bit more clearly. I don't see any
> reason why these drivers would not work on an ARM device such as the Pro
> X right now, assuming that it boots via ACPI and the serial device it
> loads against is fully functional.

As I understand, the dialect of ACPI used on the snapdragon laptops
is not really compatible with the subset expected by the kernel, so
you'd be more likely to run those laptops with a device tree description
of the hardware instead (if at all).

Making the driver talk to the hardware directly instead of going through
AML likely requires more refactoring.

> >> I'd be happy to move this to platform/surface though, if that's
> >> considered a better fit and you're okay with me adding that. Would make
> >> sense given that there's already a platform/chrome, which, as far as I
> >> can tell, also seems to be mainly focused on EC support.
> >
> > Yes, I think the main question is how much overlap you see functionally
> > between this driver and the others in drivers/platform/x86.
>
> I think that the Pro X likely won't be the last ARM Surface device with
> a SAM EC. Further, the subsystem is going to grow, and platform/x86
> seems more like a collection of, if at all, loosely connected drivers,
> which might give off the wrong impression. In my mind, this is just a
> bit more comparable to platform/chrome than the rest of platform/x86. I
> don't think I'm really qualified to make the decision on that though,
> that's just my opinion.

I would ask the drivers/platform/x86 maintainers for an opinion here,
they are probably best qualified to make that decision.

I don't really mind either way, for me this is more about who is
responsible as a subsystem maintainer than whether these are
technically x86 or not.

> Here's an overview of other drivers that I hopefully at some point get
> in good enough shape, which are part of this subsystem/dependent on the
> EC API introduced here:
>
> - A device registry / device hub for devices that are connected to the
>    EC but can't be detected via ACPI.
>
> - A dedicated battery driver for 7th generation devices (where the
>    battery isn't hanled via the ACPI shim).
>
> - A driver properly handling clipboard detachment on the Surface Books.
>
> - A driver for HID input/transport on the Surface Laptops and Surface
>    Book 3.
>
> - A driver for allowing users to set the performance/cooling mode via
>    sysfs.
>
> - Possibly a driver improving hot-plug handling of the discrete GPU in
>    the Surface Book base.

Note that drivers that connect to the bus typically don't live in the
same subdirectory as the driver that operates the bus. E.g. the
battery driver would go into drivers/power/supply and the input
would go into drivers/input/ or drivers/hid.

    Arnd

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/24/20 10:26 AM, Arnd Bergmann wrote:
> On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>> On 9/23/20 9:43 PM, Arnd Bergmann wrote:
>>> On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>>>
>>>> On 9/23/20 5:30 PM, Arnd Bergmann wrote:
>>>>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> The Surface System Aggregator Module (we'll refer to it as Surface
>>>>>> Aggregator or SAM below) is an embedded controller (EC) found on various
>>>>>> Microsoft Surface devices. Specifically, all 4th and later generation
>>>>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
>>>>>> exception of the Surface Go series and the Surface Duo. Notably, it
>>>>>> seems like this EC can also be found on the ARM-based Surface Pro X [1].
>>>>>
>>>>> I think this should go to drivers/platform/x86 or drivers/platform/surface/
>>>>> along with other laptop vendor specific code rather than drivers/misc/.
>>>>
>>>> I initially had this under drivers/platform/x86. There are two main
>>>> reasons I changed that: First, I think it's a bit too big for
>>>> platform/x86 given that it basically introduces a new subsystem. At this
>>>> point it's really less of "a couple of odd devices here and there" and
>>>> more of a bus-type thing. Second, with the possibility of future support
>>>> for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I
>>>> thought that platform/x86 would not be a good fit.
>>>
>>> I don't see that as a strong reason against it. As you write yourself, the
>>> driver won't work on the arm machines without major changes anyway,
>>> and even if it does, it fits much better with the rest of it.
>>
>> Sorry, I should have written that a bit more clearly. I don't see any
>> reason why these drivers would not work on an ARM device such as the Pro
>> X right now, assuming that it boots via ACPI and the serial device it
>> loads against is fully functional.
> 
> As I understand, the dialect of ACPI used on the snapdragon laptops
> is not really compatible with the subset expected by the kernel, so
> you'd be more likely to run those laptops with a device tree description
> of the hardware instead (if at all).
> 
> Making the driver talk to the hardware directly instead of going through
> AML likely requires more refactoring.

Oh, I did not know that! Thanks!

>>>> I'd be happy to move this to platform/surface though, if that's
>>>> considered a better fit and you're okay with me adding that. Would make
>>>> sense given that there's already a platform/chrome, which, as far as I
>>>> can tell, also seems to be mainly focused on EC support.
>>>
>>> Yes, I think the main question is how much overlap you see functionally
>>> between this driver and the others in drivers/platform/x86.
>>
>> I think that the Pro X likely won't be the last ARM Surface device with
>> a SAM EC. Further, the subsystem is going to grow, and platform/x86
>> seems more like a collection of, if at all, loosely connected drivers,
>> which might give off the wrong impression. In my mind, this is just a
>> bit more comparable to platform/chrome than the rest of platform/x86. I
>> don't think I'm really qualified to make the decision on that though,
>> that's just my opinion.
> 
> I would ask the drivers/platform/x86 maintainers for an opinion here,
> they are probably best qualified to make that decision.
> 
> I don't really mind either way, for me this is more about who is
> responsible as a subsystem maintainer than whether these are
> technically x86 or not.

I see, okay. I'll ask them and CC them on the next submission.

>> Here's an overview of other drivers that I hopefully at some point get
>> in good enough shape, which are part of this subsystem/dependent on the
>> EC API introduced here:
>>
>> - A device registry / device hub for devices that are connected to the
>>     EC but can't be detected via ACPI.
>>
>> - A dedicated battery driver for 7th generation devices (where the
>>     battery isn't hanled via the ACPI shim).
>>
>> - A driver properly handling clipboard detachment on the Surface Books.
>>
>> - A driver for HID input/transport on the Surface Laptops and Surface
>>     Book 3.
>>
>> - A driver for allowing users to set the performance/cooling mode via
>>     sysfs.
>>
>> - Possibly a driver improving hot-plug handling of the discrete GPU in
>>     the Surface Book base.
> 
> Note that drivers that connect to the bus typically don't live in the
> same subdirectory as the driver that operates the bus. E.g. the
> battery driver would go into drivers/power/supply and the input
> would go into drivers/input/ or drivers/hid.

Right. I wonder if this also holds for devices that are directly
dependent on a special platform though? It could make sense to have them
under plaform/surface rather than in the individual subsystems as they
are only ever going to be used on this platform. On the other hand, one
could argue that having them in the subsystem directories is better for
maintainability.

Thanks,
Max

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Arnd Bergmann]

On Thu, Sep 24, 2020 at 8:59 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> On 9/24/20 10:26 AM, Arnd Bergmann wrote:
> > On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:

> > Note that drivers that connect to the bus typically don't live in the
> > same subdirectory as the driver that operates the bus. E.g. the
> > battery driver would go into drivers/power/supply and the input
> > would go into drivers/input/ or drivers/hid.
>
> Right. I wonder if this also holds for devices that are directly
> dependent on a special platform though? It could make sense to have them
> under plaform/surface rather than in the individual subsystems as they
> are only ever going to be used on this platform. On the other hand, one
> could argue that having them in the subsystem directories is better for
> maintainability.

Yes, absolutely. The subsystem maintainers are the ones that are
most qualified of reviewing code that uses their subsystem, regardless
of which bus is used underneath the device, and having all drivers
for a subsystem in one place makes it much easier to refactor them
all at once in case the internal interfaces are changed or common bugs
are found in multiple drivers.

       Arnd

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/24/20 9:38 PM, Arnd Bergmann wrote:
> On Thu, Sep 24, 2020 at 8:59 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>> On 9/24/20 10:26 AM, Arnd Bergmann wrote:
>>> On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> 
>>> Note that drivers that connect to the bus typically don't live in the
>>> same subdirectory as the driver that operates the bus. E.g. the
>>> battery driver would go into drivers/power/supply and the input
>>> would go into drivers/input/ or drivers/hid.
>>
>> Right. I wonder if this also holds for devices that are directly
>> dependent on a special platform though? It could make sense to have them
>> under plaform/surface rather than in the individual subsystems as they
>> are only ever going to be used on this platform. On the other hand, one
>> could argue that having them in the subsystem directories is better for
>> maintainability.
> 
> Yes, absolutely. The subsystem maintainers are the ones that are
> most qualified of reviewing code that uses their subsystem, regardless
> of which bus is used underneath the device, and having all drivers
> for a subsystem in one place makes it much easier to refactor them
> all at once in case the internal interfaces are changed or common bugs
> are found in multiple drivers.

Got it.

Thank you for bearing with me and answering all my (probably a bit
silly) questions! I really appreciate it!

Regards,
Max

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Andy Shevchenko]

On Thu, Sep 24, 2020 at 10:38 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 24, 2020 at 8:59 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> > On 9/24/20 10:26 AM, Arnd Bergmann wrote:
> > > On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> > > Note that drivers that connect to the bus typically don't live in the
> > > same subdirectory as the driver that operates the bus. E.g. the
> > > battery driver would go into drivers/power/supply and the input
> > > would go into drivers/input/ or drivers/hid.
> >
> > Right. I wonder if this also holds for devices that are directly
> > dependent on a special platform though? It could make sense to have them
> > under plaform/surface rather than in the individual subsystems as they
> > are only ever going to be used on this platform. On the other hand, one
> > could argue that having them in the subsystem directories is better for
> > maintainability.
>
> Yes, absolutely. The subsystem maintainers are the ones that are
> most qualified of reviewing code that uses their subsystem, regardless
> of which bus is used underneath the device, and having all drivers
> for a subsystem in one place makes it much easier to refactor them
> all at once in case the internal interfaces are changed or common bugs
> are found in multiple drivers.

The problem is that some of the drivers are mostly reincarnation of
board files due to the platform being Windows-oriented with badly
written ACPI tables / firmware as a whole (which means a lot of quirks
are required).

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/24/20 10:30 AM, Andy Shevchenko wrote:
> On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>>
>>> Hello,
>>>
>>> The Surface System Aggregator Module (we'll refer to it as Surface
>>> Aggregator or SAM below) is an embedded controller (EC) found on various
>>> Microsoft Surface devices. Specifically, all 4th and later generation
>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
>>> exception of the Surface Go series and the Surface Duo. Notably, it
>>> seems like this EC can also be found on the ARM-based Surface Pro X [1].
>>
>> I think this should go to drivers/platform/x86 or drivers/platform/surface/
>> along with other laptop vendor specific code rather than drivers/misc/.
> 
> +1 here. drivers/platform/surface is a good place to start.
> And you may begin with moving a few Surface drivers out of PDx86 to
> the new folder.

Perfect, thanks! I'll draft up a patch series over the weekend.

A couple questions regarding structure and maintenance:

  - Should I CC the platform-driver-x86 list on future submissions to
    drivers/platform/surface? I.e. is this something you would want to
    review if it doesn't touch the drivers/platform/x86 directory?

  - How would you want the layout to be, specifically regarding to the
    surface-aggregator stuff? My suggestion would be simply:

    drivers/platform/surface/
        surface_aggregator/
            Kconfig
            Makefile
            core.c
            controller.c
            ... (all core stuff built into the surface_aggregator module)
        Kconfig
        Makefile
        surface_aggregator_debugfs.c
        surface_acpi_notify.c
        surface_*.c        (any other surface platform driver as well
                            as drivers dependent on surface_aggregator)

  - Regarding future things like HID transport driver, battery/AC driver:
    Submit them to drivers/platform/surface or to their respective
    subsystem directories?

Thanks,
Max

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Andy Shevchenko]

On Thu, Sep 24, 2020 at 10:17 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> On 9/24/20 10:30 AM, Andy Shevchenko wrote:
> > On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:

...

> >> I think this should go to drivers/platform/x86 or drivers/platform/surface/
> >> along with other laptop vendor specific code rather than drivers/misc/.
> >
> > +1 here. drivers/platform/surface is a good place to start.
> > And you may begin with moving a few Surface drivers out of PDx86 to
> > the new folder.
>
> Perfect, thanks! I'll draft up a patch series over the weekend.
>
> A couple questions regarding structure and maintenance:
>
>   - Should I CC the platform-driver-x86 list on future submissions to
>     drivers/platform/surface? I.e. is this something you would want to
>     review if it doesn't touch the drivers/platform/x86 directory?

Include PDx86 mailing list to the list of that. Current SURFACE*
drivers have per driver record in MAINTAINERS IIRC. So, update them as
well if needed.

>   - How would you want the layout to be, specifically regarding to the
>     surface-aggregator stuff? My suggestion would be simply:
>
>     drivers/platform/surface/

>         surface_aggregator/

Don't repeat parts of the path, the aggregator is enough as a folder
name, but the driver of course should be in its own namespace
('surface').

>             Kconfig
>             Makefile
>             core.c
>             controller.c
>             ... (all core stuff built into the surface_aggregator module)
>         Kconfig
>         Makefile

>         surface_aggregator_debugfs.c

(Not sure why it's not a part of aggregator folder)

>         surface_acpi_notify.c
>         surface_*.c        (any other surface platform driver as well
>                             as drivers dependent on surface_aggregator)
>
>   - Regarding future things like HID transport driver, battery/AC driver:
>     Submit them to drivers/platform/surface or to their respective
>     subsystem directories?

Respective subsystem _if_ it is a subsystem related driver and not
kinda board file. Use common sense and existing examples.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/25/20 4:58 PM, Andy Shevchenko wrote:
> On Thu, Sep 24, 2020 at 10:17 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>> On 9/24/20 10:30 AM, Andy Shevchenko wrote:
>>> On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> 
> ...
> 
>>>> I think this should go to drivers/platform/x86 or drivers/platform/surface/
>>>> along with other laptop vendor specific code rather than drivers/misc/.
>>>
>>> +1 here. drivers/platform/surface is a good place to start.
>>> And you may begin with moving a few Surface drivers out of PDx86 to
>>> the new folder.
>>
>> Perfect, thanks! I'll draft up a patch series over the weekend.
>>
>> A couple questions regarding structure and maintenance:
>>
>>    - Should I CC the platform-driver-x86 list on future submissions to
>>      drivers/platform/surface? I.e. is this something you would want to
>>      review if it doesn't touch the drivers/platform/x86 directory?
> 
> Include PDx86 mailing list to the list of that. Current SURFACE*
> drivers have per driver record in MAINTAINERS IIRC. So, update them as
> well if needed.

Will do.

>>    - How would you want the layout to be, specifically regarding to the
>>      surface-aggregator stuff? My suggestion would be simply:
>>
>>      drivers/platform/surface/
> 
>>          surface_aggregator/
> 
> Don't repeat parts of the path, the aggregator is enough as a folder
> name, but the driver of course should be in its own namespace
> ('surface').

Okay.

>>              Kconfig
>>              Makefile
>>              core.c
>>              controller.c
>>              ... (all core stuff built into the surface_aggregator module)
>>          Kconfig
>>          Makefile
> 
>>          surface_aggregator_debugfs.c
> 
> (Not sure why it's not a part of aggregator folder)

I kind of thought of the aggregator folder to contain only files that
build the core module. surface_aggregator_debugfs is intended as
separate module, to be loaded when needed. So I'd consider it a client
driver to the aggregator in the same way that surface_acpi_notify is.

Let me know if you still want me to move this into the aggregator folder
though. Personally, I just feel that that might lead to a bit of
confusion, specifically the idea that it's built into the core when it's
not.

>>          surface_acpi_notify.c
>>          surface_*.c        (any other surface platform driver as well
>>                              as drivers dependent on surface_aggregator)
>>
>>    - Regarding future things like HID transport driver, battery/AC driver:
>>      Submit them to drivers/platform/surface or to their respective
>>      subsystem directories?
> 
> Respective subsystem _if_ it is a subsystem related driver and not
> kinda board file. Use common sense and existing examples.

Right, thank you!

Regards,
Max