Re: [PATCH 03/13] coresight: cti: Add sysfs access to program function registers

[Greg KH]

On Mon, Mar 09, 2020 at 10:17:38AM -0600, Mathieu Poirier wrote:
> From: Mike Leach <mike.leach@xxxxxxxxxx>
> 
> Adds in sysfs programming support for the CTI function register sets.
> Allows direct manipulation of channel / trigger association registers.
> 
> Signed-off-by: Mike Leach <mike.leach@xxxxxxxxxx>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> [Fixed abbreviation in title]
> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> ---
>  drivers/hwtracing/coresight/Kconfig           |   9 +
>  .../hwtracing/coresight/coresight-cti-sysfs.c | 361 ++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-cti.c   |  19 +
>  drivers/hwtracing/coresight/coresight-cti.h   |   8 +
>  4 files changed, 397 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index 45d3822c8c8c..83e841be1081 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -122,4 +122,13 @@ config CORESIGHT_CTI
>  	  halt compared to disabling sources and sinks normally in driver
>  	  software.
>  
> +config CORESIGHT_CTI_INTEGRATION_REGS
> +	bool "Access CTI CoreSight Integration Registers"
> +	depends on CORESIGHT_CTI
> +	help
> +	  This option adds support for the CoreSight integration registers on
> +	  this device. The integration registers allow the exploration of the
> +	  CTI trigger connections between this and other devices.These
> +	  registers are not used in normal operation and can leave devices in
> +	  an inconsistent state.
>  endif
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index 507f8eb487fe..f687e07b68b0 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> @@ -109,6 +109,361 @@ static struct attribute *coresight_cti_mgmt_attrs[] = {
>  	NULL,
>  };
>  
> +/* CTI low level programming registers */
> +
> +/*
> + * Show a simple 32 bit value if enabled and powered.
> + * If inaccessible & pcached_val not NULL then show cached value.
> + */
> +static ssize_t cti_reg32_show(struct device *dev, char *buf,
> +			      u32 *pcached_val, int reg_offset)
> +{
> +	u32 val = 0;
> +	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	struct cti_config *config = &drvdata->config;
> +
> +	spin_lock(&drvdata->spinlock);
> +	if ((reg_offset >= 0) && cti_active(config)) {
> +		CS_UNLOCK(drvdata->base);
> +		val = readl_relaxed(drvdata->base + reg_offset);
> +		if (pcached_val)
> +			*pcached_val = val;
> +		CS_LOCK(drvdata->base);
> +	} else if (pcached_val) {
> +		val = *pcached_val;
> +	}
> +	spin_unlock(&drvdata->spinlock);
> +	return scnprintf(buf, PAGE_SIZE, "%#x\n", val);

Fix all of the scnprintf() calls.

And again, no documentation?

I'll stop here on this series, as much the same comments belong on the
other patches in here.

thanks,

greg k-h