On Wed, Sep 23, 2020 at 05:15:08PM +0200, Maximilian Luz wrote:[...]
Overall, nice work on this patch, the integration to the driver core
looks totally correct. Great job.
A few minor nits below:
--- /dev/null
+++ b/drivers/misc/surface_aggregator/bus.c
@@ -0,0 +1,419 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
No copyright?
+int ssam_device_add(struct ssam_device *sdev)
+{
+ int status;
+
+ /*
+ * Ensure that we can only add new devices to a controller if it has
+ * been started and is not going away soon. This works in combination
+ * with ssam_controller_remove_clients to ensure driver presence for the
+ * controller device, i.e. it ensures that the controller (sdev->ctrl)
+ * is always valid and can be used for requests as long as the client
+ * device we add here is registered as child under it. This essentially
+ * guarantees that the client driver can always expect the preconditions
+ * for functions like ssam_request_sync (controller has to be started
+ * and is not suspended) to hold and thus does not have to check for
+ * them.
+ *
+ * Note that for this to work, the controller has to be a parent device.
+ * If it is not a direct parent, care has to be taken that the device is
+ * removed via ssam_device_remove(), as device_unregister does not
+ * remove child devices recursively.
+ */
+ ssam_controller_statelock(sdev->ctrl);
+
+ if (READ_ONCE(sdev->ctrl->state) != SSAM_CONTROLLER_STARTED) {
You locked the state, why the READ_ONCE()? Is taht needed?
+ ssam_controller_stateunlock(sdev->ctrl);
+ return -ENXIO;
odd error value, why this one?
+/**
+ * struct ssam_device_uid - Unique identifier for SSAM device.
+ * @domain: Domain of the device.
+ * @category: Target category of the device.
+ * @target: Target ID of the device.
+ * @instance: Instance ID of the device.
+ * @function: Sub-function of the device. This field can be used to split a
+ * single SAM device into multiple virtual subdevices to separate
+ * different functionality of that device and allow one driver per
+ * such functionality.
+ */
+struct ssam_device_uid {
+ u8 domain;
+ u8 category;
+ u8 target;
+ u8 instance;
+ u8 function;
+};
+
+/*
+ * Special values for device matching.
+ */
+#define SSAM_ANY_TID 0xffff
+#define SSAM_ANY_IID 0xffff
+#define SSAM_ANY_FUN 0xffff
These are 16 bits, but the uid values above are 8 bits. How does that
match up?
+/**
+ * SSAM_DEVICE() - Initialize a &struct ssam_device_id with the given
+ * parameters.
+ * @d: Domain of the device.
+ * @cat: Target category of the device.
+ * @tid: Target ID of the device.
+ * @iid: Instance ID of the device.
+ * @fun: Sub-function of the device.
+ *
+ * Initializes a &struct ssam_device_id with the given parameters. See &struct
+ * ssam_device_uid for details regarding the parameters. The special values
+ * %SSAM_ANY_TID, %SSAM_ANY_IID, and %SSAM_ANY_FUN can be used to specify that
+ * matching should ignore target ID, instance ID, and/or sub-function,
+ * respectively. This macro initializes the ``match_flags`` field based on the
+ * given parameters.
+ */
+#define SSAM_DEVICE(d, cat, tid, iid, fun) \
+ .match_flags = (((tid) != SSAM_ANY_TID) ? SSAM_MATCH_TARGET : 0) \
+ | (((iid) != SSAM_ANY_IID) ? SSAM_MATCH_INSTANCE : 0) \
+ | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0), \
+ .domain = d, \
+ .category = cat, \
+ .target = ((tid) != SSAM_ANY_TID) ? (tid) : 0, \
+ .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \
+ .function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0 \
Again, the 16 vs 8 bits here feels odd. No casting???
+/**
+ * ssam_device_get() - Increment reference count of SSAM client device.
+ * @sdev: The device to increment the reference count of.
+ *
+ * Increments the reference count of the given SSAM client device by
+ * incrementing the reference count of the enclosed &struct device via
+ * get_device().
+ *
+ * See ssam_device_put() for the counter-part of this function.
+ *
+ * Return: Returns the device provided as input.
+ */
+static inline struct ssam_device *ssam_device_get(struct ssam_device *sdev)
+{
+ get_device(&sdev->dev);
+ return sdev;
Do you want to check if sdev is NULL or not here before referencing
it?
+}
+
+/**
+ * ssam_device_put() - Decrement reference count of SSAM client device.
+ * @sdev: The device to decrement the reference count of.
+ *
+ * Decrements the reference count of the given SSAM client device by
+ * decrementing the reference count of the enclosed &struct device via
+ * put_device().
+ *
+ * See ssam_device_get() for the counter-part of this function.
+ */
+static inline void ssam_device_put(struct ssam_device *sdev)
+{
+ put_device(&sdev->dev);
Same here, do you need to check?
anyway, again, nice work, if only all of my code reviews were this easy
:)