Re: [PATCH 01/13] coresight: cti: Initial CoreSight CTI Driver

From: Greg KH <gregkh@linuxfoundation.org>
To: Mathieu Poirier <mathieu.poirier@linaro.org>,
	Mike Leach <mike.leach@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 01/13] coresight: cti: Initial CoreSight CTI Driver
Date: Wed, 18 Mar 2020 14:22:41 +0100	[thread overview]
Message-ID: <20200318132241.GB2789508@kroah.com> (raw)
In-Reply-To: <20200309161748.31975-2-mathieu.poirier@linaro.org>

On Mon, Mar 09, 2020 at 10:17:36AM -0600, Mathieu Poirier wrote:
> From: Mike Leach <mike.leach@linaro.org>
> 
> This introduces a baseline CTI driver and associated configuration files.
> 
> Uses the platform agnostic naming standard for CoreSight devices, along
> with a generic platform probing method that currently supports device
> tree descriptions, but allows for the ACPI bindings to be added once these
> have been defined for the CTI devices.
> 
> Driver will probe for the device on the AMBA bus, and load the CTI driver
> on CoreSight ID match to CTI IDs in tables.
> 
> Initial sysfs support for enable / disable provided.
> 
> Default CTI interconnection data is generated based on hardware
> register signal counts, with no additional connection information.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>

You didn't cc: all of them to get review comments?  I've added it
above...

And signed-off-by implies reviewed-by.

> +/* basic attributes */
> +static ssize_t enable_show(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	int enable_req;
> +	bool enabled, powered;
> +	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	ssize_t size = 0;
> +
> +	enable_req = atomic_read(&drvdata->config.enable_req_count);
> +	spin_lock(&drvdata->spinlock);
> +	powered = drvdata->config.hw_powered;
> +	enabled = drvdata->config.hw_enabled;
> +	spin_unlock(&drvdata->spinlock);
> +
> +	if (powered) {
> +		size = scnprintf(buf, PAGE_SIZE, "cti %s; powered;\n",
> +				 enabled ? "enabled" : "disabled");
> +	} else {
> +		size = scnprintf(buf, PAGE_SIZE, "cti %s; unpowered;\n",
> +				 enable_req ? "enable req" : "disabled");

sysfs files should never need scnprintf() as you "know" a single value
will fit into a PAGE_SIZE.

And shouldn't this just be a single value, this looks like it is 2
values in one line, that then needs to be parsed, is that to be
expected?

Where is the documentation for this new sysfs file?

> +const struct attribute_group *coresight_cti_groups[] = {
> +	&coresight_cti_group,
> +	NULL,
> +};

ATTRIBUTE_GROUPS()?

> +static struct amba_driver cti_driver = {
> +	.drv = {
> +		.name	= "coresight-cti",
> +		.owner = THIS_MODULE,

Aren't amba drivers smart enough to set this properly on their own?
{sigh}

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel