[...]
+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.
- 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
- 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.
- 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.
+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;
- 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.
- }
- 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.
Thanks, Mathieu
+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
-
Mathieu Poirier