[...]

quoted

quoted

+
+static ssize_t reset_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf,
+					  size_t size)
+{
+	int ret = 0;
+	unsigned long val;
+	struct mcmb_dataset *mcmb_temp = NULL;
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	ret = kstrtoul(buf, 10, &val);

The coresight subsystem normally uses the hexadecimal base.

We will address you comments.

quoted

quoted

+	if (ret)
+		return ret;

Shouldn't this be "if (!ret)" ? 

When ret is not 0, it need to return.

I would expect something like this:

$ echo 1 > /sys/path/to/tpdm/device/reset

and not

$ echo 0 > /sys/path/to/tpdm/device/reset

The latter is what the code does.

Thanks,
Mathieu

quoted

quoted

+
+	mutex_lock(&drvdata->lock);
+	/* Reset all datasets to ZERO */
+	if (drvdata->gpr != NULL)
+		memset(drvdata->gpr, 0, sizeof(struct gpr_dataset));
+
+	if (drvdata->bc != NULL)
+		memset(drvdata->bc, 0, sizeof(struct bc_dataset));
+
+	if (drvdata->tc != NULL)
+		memset(drvdata->tc, 0, sizeof(struct tc_dataset));
+
+	if (drvdata->dsb != NULL)
+		memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
+
+	if (drvdata->cmb != NULL) {
+		if (drvdata->cmb->mcmb != NULL) {
+			mcmb_temp = drvdata->cmb->mcmb;
+			memset(drvdata->cmb->mcmb, 0,
+				sizeof(struct mcmb_dataset));
+			}
+
+		memset(drvdata->cmb, 0, sizeof(struct cmb_dataset));
+		drvdata->cmb->mcmb = mcmb_temp;
+	}
+	/* Init the default data */
+	tpdm_init_default_data(drvdata);
+
+	mutex_unlock(&drvdata->lock);
+
+	/* Disable tpdm if enabled */
+	if (drvdata->enable)
+		coresight_disable(drvdata->csdev);

Why is this done out of the lock?

When call coresight_disable, tpdm_disable will be called. There is lock in tpdm_disable.
If add it into the lock, there will be dead lock.

quoted

quoted

+
+	return size;
+}
+static DEVICE_ATTR_WO(reset);
+
+static ssize_t integration_test_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf,
+					  size_t size)
+{
+	int i, ret = 0;
+	unsigned long val;
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	if (val != 1 && val != 2)
+		return -EINVAL;
+
+	if (!drvdata->enable)
+		return -EINVAL;
+
+	if (val == 1)
+		val = ATBCNTRL_VAL_64;
+	else
+		val = ATBCNTRL_VAL_32;
+	TPDM_UNLOCK(drvdata);
+	tpdm_writel(drvdata, 0x1, TPDM_ITCNTRL);
+
+	for (i = 1; i < 5; i++)
+		tpdm_writel(drvdata, val, TPDM_ITATBCNTRL);
+
+	tpdm_writel(drvdata, 0, TPDM_ITCNTRL);
+	TPDM_LOCK(drvdata);
+	return size;
+}
+static DEVICE_ATTR_WO(integration_test);

Integration test interface should be conditional to a compile time option.  Have
a look at what was done for CTIs.

We will check and update.

quoted

quoted

+
+static ssize_t gp_regs_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	int i = 0;
+
+	if (!test_bit(TPDM_DS_GPR, drvdata->datasets))
+		return -EPERM;

                return -EINVAL;

quoted

+
+	mutex_lock(&drvdata->lock);
+	for (i = 0; i < TPDM_GPR_REGS_MAX; i++) {
+		if (!test_bit(i, drvdata->gpr->gpr_dirty))
+			continue;
+		size += scnprintf(buf + size, PAGE_SIZE - size,
+				  "Index: 0x%x Value: 0x%x\n", i,
+				  drvdata->gpr->gp_regs[i]);

This should not be - the sysfs interface requires outputs of a single line.

We will check and update.

quoted

quoted

+	}
+	mutex_unlock(&drvdata->lock);
+	return size;
+}
+
+static ssize_t gp_regs_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long index, val;
+
+	if (sscanf(buf, "%lx %lx", &index, &val) != 2)
+		return -EINVAL;
+	if (!test_bit(TPDM_DS_GPR, drvdata->datasets) ||
+	    index >= TPDM_GPR_REGS_MAX)
+		return -EPERM;
+
+	mutex_lock(&drvdata->lock);
+	drvdata->gpr->gp_regs[index] = val;
+	__set_bit(index, drvdata->gpr->gpr_dirty);
+	mutex_unlock(&drvdata->lock);
+	return size;
+}
+static DEVICE_ATTR_RW(gp_regs);
+
+static struct attribute *tpdm_attrs[] = {
+	&dev_attr_available_datasets.attr,
+	&dev_attr_enable_datasets.attr,
+	&dev_attr_reset.attr,
+	&dev_attr_integration_test.attr,
+	&dev_attr_gp_regs.attr,
+	NULL,
+};

All new sysfs interface need to be documented.  See here:

Documentation/ABI/testing/sysfs-bus-coresight-devices-xyz

More comments to come...

We will add the comments. 

quoted

Thanks,
Mathieu

quoted

+
+static struct attribute_group tpdm_attr_grp = {
+	.attrs = tpdm_attrs,
+};
+static const struct attribute_group *tpdm_attr_grps[] = {
+	&tpdm_attr_grp,
+	NULL,
+};
+
 static int tpdm_datasets_alloc(struct tpdm_drvdata *drvdata)
 {
 	if (test_bit(TPDM_DS_GPR, drvdata->datasets)) {

@@ -513,6 +846,7 @@ static int tpdm_probe(struct amba_device *adev, const struct amba_id *id)
 	desc.ops = &tpdm_cs_ops;
 	desc.pdata = adev->dev.platform_data;
 	desc.dev = &adev->dev;
+	desc.groups = tpdm_attr_grps;
 	drvdata->csdev = coresight_register(&desc);
 	if (IS_ERR(drvdata->csdev))
 		return PTR_ERR(drvdata->csdev);

-- 
2.17.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel