On Thu, Oct 21, 2021 at 03:38:51PM +0800, Tao Zhang wrote:

The BC(Basic Counters) interface has RW, WO and RO fields for controlling BC dataset elements transmitted on ATB flush. The BC data set subunit supports from 1-32 counter instances allowing for collection of BC data sets.

Signed-off-by: Tao Zhang quic_taozha@quicinc.com

drivers/hwtracing/coresight/coresight-tpdm.c | 873 +++++++++++++++++++ 1 file changed, 873 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c index c0a01979e42f..0970c69ac8e2 100644 --- a/drivers/hwtracing/coresight/coresight-tpdm.c +++ b/drivers/hwtracing/coresight/coresight-tpdm.c @@ -668,6 +668,878 @@ static ssize_t gp_regs_store(struct device *dev, } static DEVICE_ATTR_RW(gp_regs); +static ssize_t bc_capture_mode_show(struct device *dev,

• 			 struct device_attribute *attr,
  

• 			 char *buf)
  

Indentation. I won't repeat this comment but please make sure it is fixed for the entire patchset.

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• return scnprintf(buf, PAGE_SIZE, "%s\n",

• 	 drvdata->bc->capture_mode == TPDM_MODE_ATB ?
  

• 	 "ATB" : "APB");
  

+}

+static ssize_t bc_capture_mode_store(struct device *dev,

• 			  struct device_attribute *attr,
  

• 			  const char *buf,
  

• 			  size_t size)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• char str[20] = "";

char str[4];

• uint32_t val;
• if (size >= 20)

• return -EINVAL;
  

• if (sscanf(buf, "%s", str) != 1)

if (sscanf(buf, "%3s", str) != 1)

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

return -EINVAL;

Please make sure this is fixed everywhere, except when -EINVAL is really the right error code.

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

Why does the device need to be enabled for this operation? Again no comments...

• }
• if (!strcmp(str, "ATB")) {

• drvdata->bc->capture_mode = TPDM_MODE_ATB;
  

• } else if (!strcmp(str, "APB") &&

•    drvdata->bc->retrieval_mode == TPDM_MODE_APB) {
  

• TPDM_UNLOCK(drvdata);
  

• val = tpdm_readl(drvdata, TPDM_BC_CR);
  

• val = val | BIT(3);
  

• tpdm_writel(drvdata, val, TPDM_BC_CR);
  

• TPDM_LOCK(drvdata);
  

• drvdata->bc->capture_mode = TPDM_MODE_APB;
  

• } else {

• mutex_unlock(&drvdata->lock);
  

• return -EINVAL;
  

• }
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_capture_mode);

+static ssize_t bc_retrieval_mode_show(struct device *dev,

• 			   struct device_attribute *attr,
  

• 			   char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• return scnprintf(buf, PAGE_SIZE, "%s\n",

• 	 drvdata->bc->retrieval_mode == TPDM_MODE_ATB ?
  

• 	 "ATB" : "APB");
  

+}

+static ssize_t bc_retrieval_mode_store(struct device *dev,

• 			    struct device_attribute *attr,
  

• 			    const char *buf,
  

• 			    size_t size)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• char str[20] = "";
• if (size >= 20)

• return -EINVAL;
  

• if (sscanf(buf, "%s", str) != 1)

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

Same here, I don't know why the device needs to be enabled for this to success. Please fix everywhere.

• }
• if (!strcmp(str, "ATB")) {

• drvdata->bc->retrieval_mode = TPDM_MODE_ATB;
  

• } else if (!strcmp(str, "APB")) {

• drvdata->bc->retrieval_mode = TPDM_MODE_APB;
  

• } else {

• mutex_unlock(&drvdata->lock);
  

• return -EINVAL;
  

• }
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_retrieval_mode);

+static ssize_t bc_reset_counters_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• if (val) {

• TPDM_UNLOCK(drvdata);
  

• val = tpdm_readl(drvdata, TPDM_BC_CR);
  

• val = val | BIT(1);
  

• tpdm_writel(drvdata, val, TPDM_BC_CR);
  

• TPDM_LOCK(drvdata);
  

• }
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_WO(bc_reset_counters);

+static ssize_t bc_sat_mode_show(struct device *dev,

• 		     struct device_attribute *attr,
  

• 		     char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• return scnprintf(buf, PAGE_SIZE, "%lx\n",

• 	 (unsigned long)drvdata->bc->sat_mode);
  

return scnprintf(buf, PAGE_SIZE, "%#x\n", drvdata->bc->sat_mode);

And everywhere casting in used...

+}

+static ssize_t bc_sat_mode_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• drvdata->bc->sat_mode = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_sat_mode);

+static ssize_t bc_enable_counters_show(struct device *dev,

• 			    struct device_attribute *attr,
  

• 			    char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• return scnprintf(buf, PAGE_SIZE, "%lx\n",

• 	 (unsigned long)drvdata->bc->enable_counters);
  

+}

+static ssize_t bc_enable_counters_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• drvdata->bc->enable_counters = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_enable_counters);

+static ssize_t bc_clear_counters_show(struct device *dev,

• 			   struct device_attribute *attr,
  

• 			   char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• return scnprintf(buf, PAGE_SIZE, "%lx\n",

• 	 (unsigned long)drvdata->bc->clear_counters);
  

+}

+static ssize_t bc_clear_counters_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• drvdata->bc->clear_counters = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_clear_counters);

+static ssize_t bc_enable_irq_show(struct device *dev,

• 		       struct device_attribute *attr,
  

• 		       char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• return scnprintf(buf, PAGE_SIZE, "%lx\n",

• 	 (unsigned long)drvdata->bc->enable_irq);
  

+}

+static ssize_t bc_enable_irq_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• drvdata->bc->enable_irq = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_enable_irq);

+static ssize_t bc_clear_irq_show(struct device *dev,

• 		      struct device_attribute *attr,
  

• 		      char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• return scnprintf(buf, PAGE_SIZE, "%lx\n",

• 	 (unsigned long)drvdata->bc->clear_irq);
  

+}

+static ssize_t bc_clear_irq_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• drvdata->bc->clear_irq = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_clear_irq);

+static ssize_t bc_trig_val_lo_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_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• for (i = 0; i < TPDM_BC_MAX_COUNTERS; i++)

• size += scnprintf(buf + size, PAGE_SIZE - size,
  

• 		  "Index: 0x%x Value: 0x%x\n", i,
  

• 		  drvdata->bc->trig_val_lo[i]);
  

As previously stated, the sysfs interface should output single line and single values. I won't comment on this again, please fix everywhere.

• mutex_unlock(&drvdata->lock);
• return size;

+}

+static ssize_t bc_trig_val_lo_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_BC, drvdata->datasets) ||

•    index >= drvdata->bc_counters_avail ||
  

•    drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_NO ||
  

•    (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_PARTIAL && index > 0))
  

• return -EPERM;
  
  

This is hard to read and maintain. Please break it up in multiple if() statements.

• mutex_lock(&drvdata->lock);
• drvdata->bc->trig_val_lo[index] = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_trig_val_lo);

+static ssize_t bc_trig_val_hi_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_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• for (i = 0; i < TPDM_BC_MAX_COUNTERS; i++)

• size += scnprintf(buf + size, PAGE_SIZE - size,
  

• 		  "Index: 0x%x Value: 0x%x\n", i,
  

• 		  drvdata->bc->trig_val_hi[i]);
  

• mutex_unlock(&drvdata->lock);
• return size;

+}

+static ssize_t bc_trig_val_hi_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_BC, drvdata->datasets) ||

•    index >= drvdata->bc_counters_avail ||
  

•    drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_NO ||
  

•    (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_PARTIAL && index > 0))
  

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• drvdata->bc->trig_val_hi[index] = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_trig_val_hi);

+static ssize_t bc_enable_ganging_show(struct device *dev,

• 			   struct device_attribute *attr,
  

• 			   char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• return scnprintf(buf, PAGE_SIZE, "%lx\n",

• 	 (unsigned long)drvdata->bc->enable_ganging);
  

+}

+static ssize_t bc_enable_ganging_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• drvdata->bc->enable_ganging = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_enable_ganging);

+static ssize_t bc_overflow_val_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_BC, drvdata->datasets))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• for (i = 0; i < TPDM_BC_MAX_OVERFLOW; i++)

• size += scnprintf(buf + size, PAGE_SIZE - size,
  

• 		  "Index: 0x%x Value: 0x%x\n", i,
  

• 		  drvdata->bc->overflow_val[i]);
  

• mutex_unlock(&drvdata->lock);
• return size;

+}

+static ssize_t bc_overflow_val_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_BC, drvdata->datasets) ||

•    index >= TPDM_BC_MAX_OVERFLOW)
  

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• drvdata->bc->overflow_val[index] = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_overflow_val);

+static ssize_t bc_ovsr_show(struct device *dev,

• 		 struct device_attribute *attr,
  

• 		 char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• unsigned long val;
• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• TPDM_UNLOCK(drvdata);
• val = tpdm_readl(drvdata, TPDM_BC_OVSR);
• TPDM_LOCK(drvdata);
• mutex_unlock(&drvdata->lock);
• return scnprintf(buf, PAGE_SIZE, "%lx\n", val);

+}

+static ssize_t bc_ovsr_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• if (val) {

• TPDM_UNLOCK(drvdata);
  

• tpdm_writel(drvdata, val, TPDM_BC_OVSR);
  

• TPDM_LOCK(drvdata);
  

• }
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_ovsr);

+static ssize_t bc_counter_sel_show(struct device *dev,

• 			struct device_attribute *attr,
  

• 			char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• unsigned long val;
• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• TPDM_UNLOCK(drvdata);
• val = tpdm_readl(drvdata, TPDM_BC_SELR);
• TPDM_LOCK(drvdata);
• mutex_unlock(&drvdata->lock);
• return scnprintf(buf, PAGE_SIZE, "%lx\n", val);

+}

+static ssize_t bc_counter_sel_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable || val >= drvdata->bc_counters_avail) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• TPDM_UNLOCK(drvdata);
• tpdm_writel(drvdata, val, TPDM_BC_SELR);
• TPDM_LOCK(drvdata);
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_counter_sel);

+static ssize_t bc_count_val_lo_show(struct device *dev,

• 			 struct device_attribute *attr,
  

• 			 char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• unsigned long val;
• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• TPDM_UNLOCK(drvdata);
• val = tpdm_readl(drvdata, TPDM_BC_CNTR_LO);
• TPDM_LOCK(drvdata);
• mutex_unlock(&drvdata->lock);
• return scnprintf(buf, PAGE_SIZE, "%lx\n", val);

+}

+static ssize_t bc_count_val_lo_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 val, select;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• if (val) {

if (!val) { mutex_unlock(&drvdata->lock); return -EINVAL; }

• TPDM_UNLOCK(drvdata);
  

• select = tpdm_readl(drvdata, TPDM_BC_SELR);
  

• /* Check if selected counter is disabled */
  

• if (BMVAL(tpdm_readl(drvdata, TPDM_BC_CNTENSET), select, select)) {
  

• 	mutex_unlock(&drvdata->lock);
  

• 	return -EPERM;
  

• }
  

• tpdm_writel(drvdata, val, TPDM_BC_CNTR_LO);
  

• TPDM_LOCK(drvdata);
  

• }
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_count_val_lo);

+static ssize_t bc_count_val_hi_show(struct device *dev,

• 			 struct device_attribute *attr,
  

• 			 char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• unsigned long val;
• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• TPDM_UNLOCK(drvdata);
• val = tpdm_readl(drvdata, TPDM_BC_CNTR_HI);
• TPDM_LOCK(drvdata);
• mutex_unlock(&drvdata->lock);
• return scnprintf(buf, PAGE_SIZE, "%lx\n", val);

+}

+static ssize_t bc_count_val_hi_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 val, select;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• if (val) {

Same

• TPDM_UNLOCK(drvdata);
  

• select = tpdm_readl(drvdata, TPDM_BC_SELR);
  

• /* Check if selected counter is disabled */
  

• if (BMVAL(tpdm_readl(drvdata, TPDM_BC_CNTENSET), select, select)) {
  

• 	mutex_unlock(&drvdata->lock);
  

• 	return -EPERM;
  

• }
  

• tpdm_writel(drvdata, val, TPDM_BC_CNTR_HI);
  

• TPDM_LOCK(drvdata);
  

• }
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_count_val_hi);

+static ssize_t bc_shadow_val_lo_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_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• TPDM_UNLOCK(drvdata);
• for (i = 0; i < drvdata->bc_counters_avail; i++) {

• size += scnprintf(buf + size, PAGE_SIZE - size,
  

• 		  "Index: 0x%x Value: 0x%x\n", i,
  

• 		  tpdm_readl(drvdata, TPDM_BC_SHADOW_LO(i)));
  

• }
• TPDM_LOCK(drvdata);
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RO(bc_shadow_val_lo);

+static ssize_t bc_shadow_val_hi_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_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• TPDM_UNLOCK(drvdata);
• for (i = 0; i < drvdata->bc_counters_avail; i++)

• size += scnprintf(buf + size, PAGE_SIZE - size,
  

• 		  "Index: 0x%x Value: 0x%x\n", i,
  

• 		  tpdm_readl(drvdata, TPDM_BC_SHADOW_HI(i)));
  

• TPDM_LOCK(drvdata);
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RO(bc_shadow_val_hi);

+static ssize_t bc_sw_inc_show(struct device *dev,

• 		   struct device_attribute *attr,
  

• 		   char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• unsigned long val;
• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• TPDM_UNLOCK(drvdata);
• val = tpdm_readl(drvdata, TPDM_BC_SWINC);
• TPDM_LOCK(drvdata);
• mutex_unlock(&drvdata->lock);
• return scnprintf(buf, PAGE_SIZE, "%lx\n", val);

+}

+static ssize_t bc_sw_inc_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 val;
• if (kstrtoul(buf, 16, &val))

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->enable_ds))

• return -EPERM;
  

• mutex_lock(&drvdata->lock);
• if (!drvdata->enable) {

• mutex_unlock(&drvdata->lock);
  

• return -EPERM;
  

• }
• if (val) {

• TPDM_UNLOCK(drvdata);
  

• tpdm_writel(drvdata, val, TPDM_BC_SWINC);
  

• TPDM_LOCK(drvdata);
  

• }
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_sw_inc);

+static ssize_t bc_msr_show(struct device *dev,

• 		struct device_attribute *attr,
  

• 		char *buf)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• unsigned int i;
• ssize_t len = 0;
• if (!drvdata->msr_support)

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• for (i = 0; i < TPDM_BC_MAX_MSR; i++)

• len += scnprintf(buf + len, PAGE_SIZE - len, "%u 0x%x\n",
  

• 		 i, drvdata->bc->msr[i]);
  

• return len;

+}

+static ssize_t bc_msr_store(struct device *dev,

• 		 struct device_attribute *attr,
  

• 		 const char *buf,
  

• 		 size_t size)
  

+{

• struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
• unsigned int num, val;
• int nval;
• if (!drvdata->msr_support)

• return -EINVAL;
  

• if (!test_bit(TPDM_DS_BC, drvdata->datasets))

• return -EPERM;
  

• nval = sscanf(buf, "%u %x", &num, &val);
• if (nval != 2)

• return -EINVAL;
  

• if (num >= TPDM_BC_MAX_MSR)

• return -EINVAL;
  

• mutex_lock(&drvdata->lock);
• drvdata->bc->msr[num] = val;
• mutex_unlock(&drvdata->lock);
• return size;

+} +static DEVICE_ATTR_RW(bc_msr);

+static struct attribute *tpdm_bc_attrs[] = {

• &dev_attr_bc_capture_mode.attr,
• &dev_attr_bc_retrieval_mode.attr,
• &dev_attr_bc_reset_counters.attr,
• &dev_attr_bc_sat_mode.attr,
• &dev_attr_bc_enable_counters.attr,
• &dev_attr_bc_clear_counters.attr,
• &dev_attr_bc_enable_irq.attr,
• &dev_attr_bc_clear_irq.attr,
• &dev_attr_bc_trig_val_lo.attr,
• &dev_attr_bc_trig_val_hi.attr,
• &dev_attr_bc_enable_ganging.attr,
• &dev_attr_bc_overflow_val.attr,
• &dev_attr_bc_ovsr.attr,
• &dev_attr_bc_counter_sel.attr,
• &dev_attr_bc_count_val_lo.attr,
• &dev_attr_bc_count_val_hi.attr,
• &dev_attr_bc_shadow_val_lo.attr,
• &dev_attr_bc_shadow_val_hi.attr,
• &dev_attr_bc_sw_inc.attr,
• &dev_attr_bc_msr.attr,
• NULL,

This will result in a very crowded directory. Please move under a "bc" subdirectory. And as I commented before, all sysfs entries need to be documented under Documentation/ABI/testing.

+};

+static struct attribute_group tpdm_bc_attr_grp = {

• .attrs = tpdm_bc_attrs,

+};

static struct attribute *tpdm_attrs[] = { &dev_attr_available_datasets.attr, &dev_attr_enable_datasets.attr, @@ -682,6 +1554,7 @@ static struct attribute_group tpdm_attr_grp = { }; static const struct attribute_group *tpdm_attr_grps[] = { &tpdm_attr_grp,

• &tpdm_bc_attr_grp,

It is quite tedious to review all these options at the same time as the core drivers. I suggest to concentrate on the base functionality for now. When that is merged we can add configuration options such as these.

I am out of time for this patchset and as such will not review the remaining patches - those will have to wait for another iteration.

Thanks, Mathieu

NULL, }; -- 2.17.1