Hi,
Agreed - all option removed. Removed appset alteration in pulse store.
Thanks
Mike
On Mon, 30 Sep 2019 at 23:12, Mathieu Poirier mathieu.poirier@linaro.org wrote:
On Fri, 27 Sep 2019 at 07:49, Suzuki K Poulose suzuki.poulose@arm.com wrote:
Hi Mike,
Looks good overall. I have some comments about how we mix integer and string output for write/read for sysfs handles.
On 19/08/2019 22:13, Mike Leach wrote:
Adds a user API to allow programming of CTI by trigger ID and channel number. This will take the channel and trigger ID supplied by the user and program the appropriate register values.
Signed-off-by: Mike Leach mike.leach@linaro.org
.../hwtracing/coresight/coresight-cti-sysfs.c | 355 ++++++++++++++++++ drivers/hwtracing/coresight/coresight-cti.c | 157 +++++++- drivers/hwtracing/coresight/coresight-cti.h | 34 ++ 3 files changed, 545 insertions(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 4055e392530d..dbbfd1de58b3 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -480,6 +480,355 @@ static struct attribute *coresight_cti_regs_attrs[] = { NULL, };
+/* CTI channel x-trigger programming */ +static int +cti_trig_op_parse(struct device *dev, enum cti_chan_op op,
enum cti_trig_dir dir, const char *buf, size_t size)+{
u32 chan_idx;u32 trig_idx;int items, err = size;nit: If we set this to -EINVAL, you could remove the else case below. We can overwrite the err with size only when we have parsed properly.
/* extract chan idx and trigger idx */items = sscanf(buf, "%d %d", &chan_idx, &trig_idx);if (items) {To make sure we have got two values parsed, the above check must be :
if (items == 2) {
err = cti_channel_trig_op(dev, op, dir, chan_idx, trig_idx);if (!err)err = size;} elseerr = -EINVAL;return err;+}
+static ssize_t gate_enable_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
int err = 0, channel = 0;if ((size >= 3) && (strncmp(buf, "all", 3) == 0)) {Could we make it strict by making this (size == 4) ? But more on that below.
err = cti_channel_gate_op(dev, CTI_GATE_CHAN_ENABLE_ALL, 0);} else {if (kstrtoint(buf, 0, &channel))return -EINVAL;err = cti_channel_gate_op(dev, CTI_GATE_CHAN_ENABLE, channel);}In general I am a bit concerned over mixing string and integer as input for an ABI. I don't have a perfect solution for "all" option, other than using -1 ! What do others think ?
There is only 4 channel so it would not be overly cumbersome to simply iterate over them. That would fix the mix between integer and strings along with removing the CTI_GATE_CHAN_ENABLE_ALL option.
return err ? err : size;+} +static DEVICE_ATTR_WO(gate_enable);
+static ssize_t gate_disable_store(struct device *dev,
struct device_attribute *attr,const char *buf, size_t size)+{
int err = 0, channel = 0;if ((size >= 3) && (strncmp(buf, "all", 3) == 0)) {err = cti_channel_gate_op(dev, CTI_GATE_CHAN_DISABLE_ALL, 0);} else {if (kstrtoint(buf, 0, &channel))return -EINVAL;err = cti_channel_gate_op(dev, CTI_GATE_CHAN_DISABLE, channel);}return err ? err : size;+} +static DEVICE_ATTR_WO(gate_disable);
+static ssize_t trig_filter_enable_show(struct device *dev,
struct device_attribute *attr,char *buf)+{
unsigned long val;struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);spin_lock(&drvdata->spinlock);val = drvdata->config.trig_filter_enable;spin_unlock(&drvdata->spinlock);return scnprintf(buf, PAGE_SIZE, "%s\n", val ? "enabled" : "disabled");nit: I am not sure if "enabled"/"disabled" is preferred over 1/0. We follow the latter for enable_sink/source etc. Also for the "store" below for this field. So it may be a good idea to keep this consistent with the rest. e.g this might be more helpful for scripting.
I agree with Suzuki.
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK