Re: [PATCH v2 03/27] coresight: Add helper device type

[Suzuki K Poulose]

On 05/03/2018 06:00 PM, Mathieu Poirier wrote:

...

+/*
+ * coresight_release_device - Release this device and any of the helper
+ * devices connected to it for trace operation.
+ */
+static void coresight_release_device(struct coresight_device *csdev)
+{
+       int i;
+
+       for (i = 0; i < csdev->nr_outport; i++) {
+               struct coresight_device *child = csdev->conns[i].child_dev;
+
+               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+                       pm_runtime_put(child->dev.parent);
+       }

There is a newline here in coresight_prepare_device().  Either add one (or not)
in both function but please be consistent.

@@ -480,8 +517,7 @@ static int _coresight_build_path(struct coresight_device 
*csdev,

node->csdev = csdev;

        list_add(&node->link, path);
-       pm_runtime_get_sync(csdev->dev.parent);
-
+       coresight_prepare_device(csdev);

There was a newline between pm_runtime_get_sync() and the return statement in
the original code.

@@ -775,6 +811,10 @@ static struct device_type coresight_dev_type[] = {
                .name = "source",
                .groups = coresight_source_groups,
        },
+       {
+               .name = "helper",
+       },
+

Extra newline.

}; +/**

+ * struct coresight_ops_helper - Operations for a helper device.
+ *
+ * All operations could pass in a device specific data, which could
+ * help the helper device to determine what to do.
+ *
+ * @enable     : Turn the device ON.
+ * @disable    : Turn the device OFF.

There is a discrepancy between the comment and the operations, i.e enabling a
device is not synonymous of turning it on.  Looking at patch 04/27 the ops is
called in tmc_etr_enable/disable_catu() so the comment propably needs to be
changed.

Sure, will fix all of them.

Cheers
Suzuki