Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
On 11/19/20 4:54 PM, Barnabás Pőcze wrote:
Hi
[...]
+enum ssam_ssh_tc {
+ /* Known SSH/EC target categories. */
+ // category 0x00 is invalid for EC use
+ SSAM_SSH_TC_SAM = 0x01, // generic system functionality, real-time clock
+ SSAM_SSH_TC_BAT = 0x02, // battery/power subsystem
+ SSAM_SSH_TC_TMP = 0x03, // thermal subsystem
+ SSAM_SSH_TC_PMC = 0x04,
+ SSAM_SSH_TC_FAN = 0x05,
+ SSAM_SSH_TC_PoM = 0x06,
+ SSAM_SSH_TC_DBG = 0x07,
+ SSAM_SSH_TC_KBD = 0x08, // legacy keyboard (Laptop 1/2)
+ SSAM_SSH_TC_FWU = 0x09,
+ SSAM_SSH_TC_UNI = 0x0a,
+ SSAM_SSH_TC_LPC = 0x0b,
+ SSAM_SSH_TC_TCL = 0x0c,
+ SSAM_SSH_TC_SFL = 0x0d,
+ SSAM_SSH_TC_KIP = 0x0e,
+ SSAM_SSH_TC_EXT = 0x0f,
+ SSAM_SSH_TC_BLD = 0x10,
+ SSAM_SSH_TC_BAS = 0x11, // detachment system (Surface Book 2/3)
+ SSAM_SSH_TC_SEN = 0x12,
+ SSAM_SSH_TC_SRQ = 0x13,
+ SSAM_SSH_TC_MCU = 0x14,
+ SSAM_SSH_TC_HID = 0x15, // generic HID input subsystem
+ SSAM_SSH_TC_TCH = 0x16,
+ SSAM_SSH_TC_BKL = 0x17,
+ SSAM_SSH_TC_TAM = 0x18,
+ SSAM_SSH_TC_ACC = 0x19,
+ SSAM_SSH_TC_UFI = 0x1a,
+ SSAM_SSH_TC_USC = 0x1b,
+ SSAM_SSH_TC_PEN = 0x1c,
+ SSAM_SSH_TC_VID = 0x1d,
+ SSAM_SSH_TC_AUD = 0x1e,
+ SSAM_SSH_TC_SMC = 0x1f,
+ SSAM_SSH_TC_KPD = 0x20,
+ SSAM_SSH_TC_REG = 0x21,
+};
Is it known what these abbreviations stand for? Maybe I missed them?
The comments state all we really know about these (through observation
and experimentation). The table itself has been extracted from the
Windows driver, but the abbreviations and values are all we're getting
from it.
I see, thanks for the clarification. For some reason, I believed the
"Known SSH/EC target categories" comment means that those are known, as in,
it is known what they are for, etc., not just the abbreviation.
Right, that can be misunderstood, sorry. It's probably best if I add a
comment explaining that in a bit more detail and noting where those
values and abbreviations come from.
Thanks,
Max
Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
Hi
2020. november 19., csütörtök 0:06 keltezéssel, Maximilian Luz
írta:
> [...]
> >> +/**
> >> + * ssam_nf_head_destroy() - Deinitialize the given notifier head.
> >> + * @nh: The notifier head to deinitialize.
> >> + */
> >> +static void ssam_nf_head_destroy(struct ssam_nf_head *nh)
> >> +{
> >> + cleanup_srcu_struct(>srcu);
> >> +}
> >
> > I'm also wondering if there's any reason why these static one-liner
> > functions are
> > not explicitly marked inline.
>
> Isn't inline more of a linkage keyword at this point? I fully expect any
> compiler to inline things like this automatically at the first hint of
> an optimization flag.
>
I also expect the compiler to inline such functions even without the `inline`
specifier. In retrospect I'm not sure why I actually made this comment, possibly
because of my preference, but I believe it's fine either way, so my comment is
moot, sorry.
> [...]
> >> + * error values via ssam_notifier_from_errno() to notifier values.
> >> + *
> >> + * Also note that any callback that could handle an event should return a
> >> value
> >> + * with bit %SSAM_NOTIF_HANDLED set, indicating that the event does not go
> >> + * unhandled/ignored. In case no registered callback could handle an
> >> event,
> >> + * this function will emit a warning.
> >> + *
> >> + * In case a callback failed, this function will emit an error message.
> >> + */
> >> +static void ssam_nf_call(struct ssam_nf *nf, struct device *dev, u16 rqid,
> >> + struct ssam_event *event)
> >> +{
> >> + struct ssam_nf_head *nf_head;
> >> + int status, nf_ret;
> >> +
> >> + if (!ssh_rqid_is_event(rqid)) {
> >> + dev_warn(dev, "event: unsupported rqid: 0x%04x\n", rqid);
> >
> > A small note, "%#04x" would insert the "0x" prefix.
>
> Oh neat, didn't know that. Should be "%#06x" though, right?
>
Yes, indeed, sorry, it should be "%#06x".
> [...]
> >> +static int ssam_controller_caps_load_from_acpi(
> >> + acpi_handle handle, struct ssam_controller_caps *caps)
> >> +{
> >> + u32 d3_closes_handle = false;
> >
> > Assinging a boolean like this to a `u32` looks very odd to me.
>
> The value returned by the corresponding DSM call is an integer, but
> essentially only contains a bool value (zero vs. one). I thought using
> false here explicitly for initialization helps clarify that. That also
> makes it consistent with the "caps->d3_closes_handle = false" line
> below.
>
Although I still find it pretty odd, your explanation makes sense to me.
> >> +int ssam_request_sync_alloc(size_t payload_len, gfp_t flags,
> >> + struct ssam_request_sync **rqst,
> >> + struct ssam_span *buffer)
> >> +{
> >> + size_t msglen = SSH_COMMAND_MESSAGE_LENGTH(payload_len);
> >> +
> >> + *rqst = kzalloc(sizeof(**rqst) + msglen, flags);
> >> + if (!*rqst)
> >> + return -ENOMEM;
> >> +
> >> + buffer->ptr = (u8 *)(*rqst + 1);
> >> + buffer->len = msglen;
> >> +
> >> + return 0;
> >> +}
> >
> > I think there is a bit of incosistency: sometimes you use ** pointer +
> > return int,
> > sometimes you return a pointer with potentially embedded errno. I think it
> > would
> > be better if you stuck with one or the other.
>
> My rule of thumb is: If it's one output parameter, use a pointer as
> return value. If it's two or more output parameters (as in this case,
> "buffer" is written to), use two pointer arguments and an int as return.
>
> I noticed that ssam_client_bind doesn't follow that scheme so I'll
> change that.
>
I see, thanks for the explanation, what prompted me to make this comment
was that, as you noted, there were functions with one output parameter that
were not consistent.
> [...]
> >> + * Note that the packet completion callback is, in case of success and
> >> for a
> >> + * sequenced packet, guaranteed to run on the receiver thread, thus
> >> providing
> >> + * a way to reliably identify responses to the packet. The packet
> >> completion
> >> + * callback is only run once and it does not indicate that the packet has
> >> + * fully left the system (for this, one should rely on the release method,
> >> + * triggered when the reference count of the packet reaches zero). In
> >> case of
> >> + * re-submission (and with somewhat unlikely timing), it may be possible
> >> that
> >> + * the packet is being re-transmitted while the completion callback runs.
> >> + * Completion will occur both on success and internal error, as well as
> >> when
> >> + * the packet is canceled.
> >
> > If I understand it correctly, it is possible that submission of a packet
> > fails
> > for the first time, but it's scheduled for resubmission, and this
> > retransmission
> > happens at the same time when the complete() callback is called. If that's
> > the
> > case, then the callback is called with an error condition, no? Thus it is
> > possible
> > that a packet is successfully submitted (for the second, third, etc. time),
> > but the
> >
Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
On 11/18/20 1:28 AM, Barnabás Pőcze wrote:
Hi
I have attached some thoughts and comments inline.
Thanks!
2020. november 15., vasárnap 20:21 keltezéssel, Maximilian Luz írta:
[...]
+/* -- Event notifier/callbacks. -
*/
+/*
+ * The notifier system is based on linux/notifier.h, specifically the SRCU
+ * implementation. The difference to that is, that some bits of the notifier
+ * call return value can be tracked across multiple calls. This is done so that
+ * handling of events can be tracked and a warning can be issued in case an
+ * event goes unhandled. The idea of that waring is that it should help
discover
^
"warning"
Thanks, I'll run a spell-checker over the comments and (hopefully) fix all
typos.
+ * and identify new/currently unimplemented features.
+ */
+
+
+/**
+ * ssam_event_matches_notifier() - Test if an event matches a notifier;
^
Shouldn't it be a period?
Yes, it should.
+ * @notif: The event notifier to test against.
+ * @event: The event to test.
+ *
+ * Return: Returns %true iff the given event matches the given notifier
+ * according to the rules set in the notifier's event mask, %false otherwise.
+ */
[...]
+static int __ssam_nfblk_remove(struct ssam_nf_head *nh,
+ struct ssam_notifier_block *nb)
+{
+ struct ssam_notifier_block **link;
+
+ link = __ssam_nfblk_find_link(nh, nb);
+ if (!link)
+ return -ENOENT;
I find it odd that here you return ENOENT, but in `__ssam_nfblk_insert()`
EINVAL is returned instead of EEXIST. I believe either both should be EINVAL,
or EEXIST+ENOENT.
Yes, EEXIST is better for insert.
+
+ __ssam_nfblk_erase(link);
I'm wondering if it's necessary to create a new function which contains just
a single line.
I prefer to create these sorts of abstractions as this, in my opinion,
keeps things contained and makes things easier to reason about. In this
case, it also allows for documentation.
+ return 0;
+}
[...]
+/**
+ * ssam_nf_head_destroy() - Deinitialize the given notifier head.
+ * @nh: The notifier head to deinitialize.
+ */
+static void ssam_nf_head_destroy(struct ssam_nf_head *nh)
+{
+ cleanup_srcu_struct(>srcu);
+}
I'm also wondering if there's any reason why these static one-liner functions
are
not explicitly marked inline.
Isn't inline more of a linkage keyword at this point? I fully expect any
compiler to inline things like this automatically at the first hint of
an optimization flag.
[...]
+/**
+ * struct ssam_nf_refcount_entry - RB-tree entry for referecnce counting event
+ * activations.
+ * @node: The node of this entry in the rb-tree.
+ * @key: The key of the event.
+ * @refcount: The reference-count of the event.
+ * @flags:The flags used when enabling the event.
+ */
+struct ssam_nf_refcount_entry {
+ struct rb_node node;
+ struct ssam_nf_refcount_key key;
+ int refcount;
Is there any reason why a signed type is used for reference counting?
Can help to spot overflows when debugging, not that that should happen.
We also don't need values larger than INT_MAX. We shouldn't even get
remotely close to it.
+ u8 flags;
+};
+
+
+/**
+ * ssam_nf_refcount_inc() - Increment reference-/activation-count of the given
+ * event.
+ * @nf: The notifier system reference.
+ * @reg: The registry used to enable/disable the event.
+ * @id: The event ID.
+ *
+ * Increments the reference-/activation-count associated with the specified
+ * event type/ID, allocating a new entry for this event ID if necessary. A
+ * newly allocated entry will have a refcount of one.
Shouldn't it be noted that nf->lock(?) must(?) be held when calling?
All functions taking 'struct ssam_nf *' as first parameter must run
synchronized with regards to each other, except for ssam_nf_call().
Same for all access to struct ssam_nf directly. Whether that is
guaranteed by nf->lock or other means (de-/initialization) doesn't
really matter. Only higher-level functions may run concurrently.
On the higher level, the lock must also be held to ensure that the EC
requests are sent at the right time (which is its main purpose).
Arguably that's not well documented here, I'll try to fix that.
[...]
+ * callback is left or a callback returned a value with the %SSAM_NOTIF_STOP
+ * bit set. Note that this bit is set automatically when converting non.zero
^
maybe "non-zero"?
Right.
+ * error values via ssam_notifier_from_errno() to notifier values.
+ *
+ * Also note that any callback that could handle an event should return a value
+ * with bit %SSAM_NOTIF_HANDLED set, indicating that the event does not go
+ * unhandled/ignored. In case no registered callback could handle an event,
+ * this
Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
Hi
I have attached some thoughts and comments inline.
2020. november 15., vasárnap 20:21 keltezéssel, Maximilian Luz írta:
> [...]
> +/* -- Event notifier/callbacks.
> - */
> +/*
> + * The notifier system is based on linux/notifier.h, specifically the SRCU
> + * implementation. The difference to that is, that some bits of the notifier
> + * call return value can be tracked across multiple calls. This is done so
> that
> + * handling of events can be tracked and a warning can be issued in case an
> + * event goes unhandled. The idea of that waring is that it should help
> discover
^
"warning"
> + * and identify new/currently unimplemented features.
> + */
> +
> +
> +/**
> + * ssam_event_matches_notifier() - Test if an event matches a notifier;
^
Shouldn't it be a period?
> + * @notif: The event notifier to test against.
> + * @event: The event to test.
> + *
> + * Return: Returns %true iff the given event matches the given notifier
> + * according to the rules set in the notifier's event mask, %false otherwise.
> + */
> [...]
> +static int __ssam_nfblk_remove(struct ssam_nf_head *nh,
> +struct ssam_notifier_block *nb)
> +{
> + struct ssam_notifier_block **link;
> +
> + link = __ssam_nfblk_find_link(nh, nb);
> + if (!link)
> + return -ENOENT;
I find it odd that here you return ENOENT, but in `__ssam_nfblk_insert()`
EINVAL is returned instead of EEXIST. I believe either both should be EINVAL,
or EEXIST+ENOENT.
> +
> + __ssam_nfblk_erase(link);
I'm wondering if it's necessary to create a new function which contains just
a single line.
> + return 0;
> +}
> [...]
> +/**
> + * ssam_nf_head_destroy() - Deinitialize the given notifier head.
> + * @nh: The notifier head to deinitialize.
> + */
> +static void ssam_nf_head_destroy(struct ssam_nf_head *nh)
> +{
> + cleanup_srcu_struct(>srcu);
> +}
I'm also wondering if there's any reason why these static one-liner functions
are
not explicitly marked inline.
> [...]
> +/**
> + * struct ssam_nf_refcount_entry - RB-tree entry for referecnce counting
> event
> + * activations.
> + * @node: The node of this entry in the rb-tree.
> + * @key: The key of the event.
> + * @refcount: The reference-count of the event.
> + * @flags:The flags used when enabling the event.
> + */
> +struct ssam_nf_refcount_entry {
> + struct rb_node node;
> + struct ssam_nf_refcount_key key;
> + int refcount;
Is there any reason why a signed type is used for reference counting?
> + u8 flags;
> +};
> +
> +
> +/**
> + * ssam_nf_refcount_inc() - Increment reference-/activation-count of the
> given
> + * event.
> + * @nf: The notifier system reference.
> + * @reg: The registry used to enable/disable the event.
> + * @id: The event ID.
> + *
> + * Increments the reference-/activation-count associated with the specified
> + * event type/ID, allocating a new entry for this event ID if necessary. A
> + * newly allocated entry will have a refcount of one.
Shouldn't it be noted that nf->lock(?) must(?) be held when calling?
> [...]
> +/**
> + * ssam_nf_call() - Call notification callbacks for the provided event.
> + * @nf:The notifier system
> + * @dev: The associated device, only used for logging.
> + * @rqid: The request ID of the event.
> + * @event: The event provided to the callbacks.
> + *
> + * Executa registered callbacks in order of their priority until either no
^
"execute"
> + * callback is left or a callback returned a value with the %SSAM_NOTIF_STOP
> + * bit set. Note that this bit is set automatically when converting non.zero
^
maybe "non-zero"?
> + * error values via ssam_notifier_from_errno() to notifier values.
> + *
> + * Also note that any callback that could handle an event should return a
> value
> + * with bit %SSAM_NOTIF_HANDLED set, indicating that the event does not go
> + * unhandled/ignored. In case no registered callback could handle an event,
> + * this function will emit a warning.
> + *
> + * In case a callback failed, this function will emit an error message.
> + */
> +static void ssam_nf_call(struct ssam_nf *nf, struct device *dev, u16 rqid,
> + struct ssam_event *event)
> +{
> + struct ssam_nf_head *nf_head;
> + int status, nf_ret;
> +
> + if (!ssh_rqid_is_event(rqid)) {
> + dev_warn(dev, "event: unsupported rqid: 0x%04x\n", rqid);
A small note, "%#04x" would insert the "0x" prefix.
> + return;
> + }
> [...]
> +/**
> + * ssam_event_item_alloc() - Allocate an event item with the given payload
> size.
> + * @len: The event payload length.
> + * @flags: The flags used for allocation.
> + *
> + * Allocate an event item with
Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
On 11/16/20 6:03 PM, Maximilian Luz wrote: On 11/16/20 2:33 PM, Andy Shevchenko wrote: On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz wrote: [...] READ_ONCE and WRITE_ONCE are used to ensure proper access to state that can be changed outside of the queue/pending locks (or under any one of them). In general, I have tried to document all critical access of such state with an explanation of why it is safe to do so. I've looked at this again and noticed that I can guard the packet timestamp by the pending lock and the packet priority by the queue lock (after first submission). This makes reasoning about access to them significantly easier and removes the need for WRITE_ONCE / READ_ONCE. After that, READ_ONCE is used - to access the controller state for smoke-testing to (hopefully) detect invalid request function usage (note that all other access to this state happens under the controller state lock) - for the "safe counters", where access to the shared value is, after initialization, restricted to the increment function - to update the timeout reaper, where access to the shared value (rtx_timeout.expires) is, after initialization, restricted to its modification function (ssh_ptl_timeout_reaper_mod() / ssh_rtl_timeout_reaper_mod()) and the timer function - to access the request timestamp, which is, after initialization, only set once in the lifetime of a request (all other access is read-only) - to access the 'ptl' reference of the packet, which, after initialization, is only set once, either at packet or request submission (all other access is read-only). Note due to this, READ_ONCE is only required for functions that can run concurrently with ssh_ptl_submit() and ssh_rtl_submit(), i.e. ssh_ptl_cancel() and ssh_rtl_cancel(). - to access request state outside of bit-ops when canceling I'd argue that all of these cases can be checked and verified with a reasonable amount of effort. Cancellation (last two points) is probably the most complex one. Unfortunately, I don't see any way to simplify that part without disallowing cancellation to run concurrently to submission, which is something I'd like to support as this makes implementing asynchronous requests in the future easier. Regards, Max
Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
On 11/16/20 2:33 PM, Andy Shevchenko wrote: On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz wrote: Add Surface System Aggregator Module core and Surface Serial Hub driver, required for the embedded controller found on Microsoft Surface devices. The Surface System Aggregator Module (SSAM, SAM or Surface Aggregator) is an embedded controller (EC) found on 4th and later generation Microsoft Surface devices, with the exception of the Surface Go series. This EC provides various functionality, depending on the device in question. This can include battery status and thermal reporting (5th and later generations), but also HID keyboard (6th+) and touchpad input (7th+) on Surface Laptop and Surface Book 3 series devices. This patch provides the basic necessities for communication with the SAM EC on 5th and later generation devices. On these devices, the EC provides an interface that acts as serial device, called the Surface Serial Hub (SSH). 4th generation devices, on which the EC interface is provided via an HID-over-I2C device, are not supported by this patch. Specifically, this patch adds a driver for the SSH device (device HID MSHW0084 in ACPI), as well as a controller structure and associated API. This represents the functional core of the Surface Aggregator kernel subsystem, introduced with this patch, and will be expanded upon in subsequent commits. The SSH driver acts as the main attachment point for this subsystem and sets-up and manages the controller structure. The controller in turn provides a basic communication interface, allowing to send requests from host to EC and receiving the corresponding responses, as well as managing and receiving events, sent from EC to host. It is structured into multiple layers, with the top layer presenting the API used by other kernel drivers and the lower layers modeled after the serial protocol used for communication. Said other drivers are then responsible for providing the (Surface model specific) functionality accessible through the EC (e.g. battery status reporting, thermal information, ...) via said controller structure and API, and will be added in future commits. ... +menuconfig SURFACE_AGGREGATOR + tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers" + depends on SERIAL_DEV_BUS + depends on ACPI + select CRC_CCITT + help + The Surface System Aggregator Module (Surface SAM or SSAM) is an + embedded controller (EC) found on 5th- and later-generation Microsoft + Surface devices (i.e. Surface Pro 5, Surface Book 2, Surface Laptop, + and newer, with exception of Surface Go series devices). + + Depending on the device in question, this EC provides varying + functionality, including: + - EC access from ACPI via Surface ACPI Notify (5th- and 6th-generation) + - battery status information (all devices) + - thermal sensor access (all devices) + - performance mode / cooling mode control (all devices) + - clipboard detachment system control (Surface Book 2 and 3) + - HID / keyboard input (Surface Laptops, Surface Book 3) + + This option controls whether the Surface SAM subsystem core will be + built. This includes a driver for the Surface Serial Hub (SSH), which + is the device responsible for the communication with the EC, and a + basic kernel interface exposing the EC functionality to other client + drivers, i.e. allowing them to make requests to the EC and receive + events from it. Selecting this option alone will not provide any + client drivers and therefore no functionality beyond the in-kernel + interface. Said functionality is the responsibility of the respective + client drivers. + + Note: While 4th-generation Surface devices also make use of a SAM EC, + due to a difference in the communication interface of the controller, + only 5th and later generations are currently supported. Specifically, + devices using SAM-over-SSH are supported, whereas devices using + SAM-over-HID, which is used on the 4th generation, are currently not + supported. From this help text I didn't get if it will be a module or what if I chose the above? I thought this would be implied by the tristate options. So should I simply add something like Choose m if you want to build the SAM subsystem core and SSH driver as module, y if you want to build it into the kernel and n if you don't want it at all. ... +/* -- Safe counters. */ Why can't XArray be used here? These are basically packet and request IDs (that are sent to the EC and received from it). XArray is a type of collection. I'm not sure how this would work. What happens is: For each new packet/request we get a new ID by incrementing the counter (with wrap-around). This ID is
Re: [PATCH 1/9] platform/surface: Add Surface Aggregator subsystem
On Sun, Nov 15, 2020 at 9:25 PM Maximilian Luz wrote: > > Add Surface System Aggregator Module core and Surface Serial Hub driver, > required for the embedded controller found on Microsoft Surface devices. > > The Surface System Aggregator Module (SSAM, SAM or Surface Aggregator) > is an embedded controller (EC) found on 4th and later generation > Microsoft Surface devices, with the exception of the Surface Go series. > This EC provides various functionality, depending on the device in > question. This can include battery status and thermal reporting (5th and > later generations), but also HID keyboard (6th+) and touchpad input > (7th+) on Surface Laptop and Surface Book 3 series devices. > > This patch provides the basic necessities for communication with the SAM > EC on 5th and later generation devices. On these devices, the EC > provides an interface that acts as serial device, called the Surface > Serial Hub (SSH). 4th generation devices, on which the EC interface is > provided via an HID-over-I2C device, are not supported by this patch. > > Specifically, this patch adds a driver for the SSH device (device HID > MSHW0084 in ACPI), as well as a controller structure and associated API. > This represents the functional core of the Surface Aggregator kernel > subsystem, introduced with this patch, and will be expanded upon in > subsequent commits. > > The SSH driver acts as the main attachment point for this subsystem and > sets-up and manages the controller structure. The controller in turn > provides a basic communication interface, allowing to send requests from > host to EC and receiving the corresponding responses, as well as > managing and receiving events, sent from EC to host. It is structured > into multiple layers, with the top layer presenting the API used by > other kernel drivers and the lower layers modeled after the serial > protocol used for communication. > > Said other drivers are then responsible for providing the (Surface model > specific) functionality accessible through the EC (e.g. battery status > reporting, thermal information, ...) via said controller structure and > API, and will be added in future commits. ... > +menuconfig SURFACE_AGGREGATOR > + tristate "Microsoft Surface System Aggregator Module Subsystem and > Drivers" > + depends on SERIAL_DEV_BUS > + depends on ACPI > + select CRC_CCITT > + help > + The Surface System Aggregator Module (Surface SAM or SSAM) is an > + embedded controller (EC) found on 5th- and later-generation > Microsoft > + Surface devices (i.e. Surface Pro 5, Surface Book 2, Surface Laptop, > + and newer, with exception of Surface Go series devices). > + > + Depending on the device in question, this EC provides varying > + functionality, including: > + - EC access from ACPI via Surface ACPI Notify (5th- and > 6th-generation) > + - battery status information (all devices) > + - thermal sensor access (all devices) > + - performance mode / cooling mode control (all devices) > + - clipboard detachment system control (Surface Book 2 and 3) > + - HID / keyboard input (Surface Laptops, Surface Book 3) > + > + This option controls whether the Surface SAM subsystem core will be > + built. This includes a driver for the Surface Serial Hub (SSH), > which > + is the device responsible for the communication with the EC, and a > + basic kernel interface exposing the EC functionality to other client > + drivers, i.e. allowing them to make requests to the EC and receive > + events from it. Selecting this option alone will not provide any > + client drivers and therefore no functionality beyond the in-kernel > + interface. Said functionality is the responsibility of the > respective > + client drivers. > + > + Note: While 4th-generation Surface devices also make use of a SAM > EC, > + due to a difference in the communication interface of the > controller, > + only 5th and later generations are currently supported. > Specifically, > + devices using SAM-over-SSH are supported, whereas devices using > + SAM-over-HID, which is used on the 4th generation, are currently not > + supported. >From this help text I didn't get if it will be a module or what if I chose the above? ... > +/* -- Safe counters. > */ Why can't XArray be used here? ... > + > + One blank line is enough ... > +static bool ssam_event_matches_notifier( > + const struct ssam_event_notifier *notif, > + const struct ssam_event *event) Perhaps static bool ssam_event_matches_notifier(const struct ssam_event_notifier *n, const struct ssam_event *event) (or even switch to 100 limit, also note notif ->n — no need to repeat same word) ... > + nb
