Hi Leo,
On Sat, 25 May 2019 at 09:31, Leo Yan leo.yan@linaro.org wrote:
On Wed, May 01, 2019 at 09:49:34AM +0100, Mike Leach wrote:
[...]
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 9b6749621dcb..ff8dad02a779 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
[...]
+/* register based attributes */ +#define coresight_cti_reg(name, offset) \
coresight_simple_reg32(struct cti_drvdata, name, offset)+/*
- Show a simple 32 bit value. If pval is NULL then live read,
- otherwise read from supplied pointer only.
- */
+static ssize_t cti_reg32_show(struct device *dev, char *buf,
u32 *pval, int reg_offset)+{
unsigned long val = 0;Since this function is only used for 32 bit register, so could consider to use u32.
OK.
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;spin_lock(&drvdata->spinlock);if (pval) {val = (unsigned long)*pval;} else if ((reg_offset >= 0) && CTI_PWR_ENA(config)) {CS_UNLOCK(drvdata->base);val = readl_relaxed(drvdata->base + reg_offset);CS_LOCK(drvdata->base);}spin_unlock(&drvdata->spinlock);return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);+}
+/*
- Store a simple 32 bit value.
- If pval not NULL, then copy to here too,
- if reg_offset >= 0 then write through if enabled.
- */
+static ssize_t cti_reg32_store(struct device *dev, const char *buf,
size_t size, u32 *pval,int reg_offset)+{
unsigned long val;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;if (kstrtoul(buf, 16, &val))return -EINVAL;spin_lock(&drvdata->spinlock);/* local store */if (pval)*pval = (u32)val;/* write through of offset and enabled */if ((reg_offset >= 0) && CTI_PWR_ENA(config))If CTI_PWR_ENA() returns false, should return back the failure to use space? This can avoid the confusion that the user will understand the register has been set but actually the kernel doesn't really set the register when the CTI is not powered on.
No - the purpose of the function is to store the value in the driver. If the user is programming when the device is disabled then the value will be successfully stored, and can be read back. The user may have disabled the device prior to programming a series of registers so we cannot error here. Similarly, unlike other CS devices, CTI can be programmed when enabled, so that is the purpose of the check - to see if it is permissible to write the value directly.
For this reason, it's good to check CTI is powered/enabled at the beginning for every xxx_store() functions.
cti_write_single_reg(drvdata, reg_offset, val);spin_unlock(&drvdata->spinlock);return size;+}
[...]
+static ssize_t appclear_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
unsigned long val, mask;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;if (kstrtoul(buf, 16, &val))return -EINVAL;spin_lock(&drvdata->spinlock);/* a 1'b1 in appclr clears down the same bit in appset*/mask = ~val;config->ctiappset &= mask;nitpick: 'mask' variable is not necessary, we can use 'val':
config->ctiappset &= ~val;
/* write through if enabled */if (CTI_PWR_ENA(config))cti_write_single_reg(drvdata, CTIAPPCLEAR, val);spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_WO(appclear);
+static ssize_t apppulse_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
unsigned long val, mask;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *config = &drvdata->config;if (kstrtoul(buf, 16, &val))return -EINVAL;spin_lock(&drvdata->spinlock);/** a 1'b1 in apppulse sets then clears the bit,* effectively clears down the same bit in appset*/mask = ~val;config->ctiappset &= mask;Ditto.
OK.
/* write through if enabled */if (CTI_PWR_ENA(config))cti_write_single_reg(drvdata, CTIAPPPULSE, val);spin_unlock(&drvdata->spinlock);return size;+} +static DEVICE_ATTR_WO(apppulse);
[...]
+/* for the selected channel - show the signals attached. */ +static ssize_t show_chan_xtrigs_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);struct cti_config *cfg = &drvdata->config;int used = 0, reg_idx;int buf_sz = PAGE_SIZE;u32 chan_mask = 0x1 << cfg->xtrig_rchan_sel;used += scnprintf(buf, buf_sz, "IN: ");for (reg_idx = 0;reg_idx < drvdata->config.nr_trig_max;reg_idx++) {if (chan_mask & cfg->ctiinen[reg_idx]) {used += scnprintf(buf+used, buf_sz-used, "%d ",reg_idx);}}used += scnprintf(buf+used, buf_sz-used, " OUT: ");Nitpick: remove the first space in " OUT: ".
OK
for (reg_idx = 0;reg_idx < drvdata->config.nr_trig_max;reg_idx++) {if (chan_mask & cfg->ctiouten[reg_idx]) {used += scnprintf(buf+used, buf_sz-used, "%d ",reg_idx);}}used += scnprintf(buf+used, buf_sz-used, "\n");return used;+} +static DEVICE_ATTR_RO(show_chan_xtrigs);
[...]
Thanks, Leo Yan
Thanks
Mike
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK