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

[Mathieu Poirier]

On Wed, Mar 18, 2020 at 02:22:41PM +0100, Greg KH wrote:
> On Mon, Mar 09, 2020 at 10:17:36AM -0600, Mathieu Poirier wrote:
> > From: Mike Leach <mike.leach@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> 
> You didn't cc: all of them to get review comments?  I've added it
> above...

Thanks

> 
> And signed-off-by implies reviewed-by.

This set has been refined over several iterations.  I added my R-b to patches
that I had reviewed and did not need attentions anymore.  Since this is supposed
to be a chain of custody I decided to keep my R-b and append my S-b before
queueing in my tree.  I have seen this done many times before but will remove if
you think it is better.

> 
> > +/* 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.

I've seen many patches using scnprintf() that were merged.  We can change this
to sprintf().

> 
> 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?

There is no shortage of files under /sys/device/ with output that needs parsing,
but this can be split in two entries.

> 
> Where is the documentation for this new sysfs file?

All the documentation for sysfs files are lumped together in a single patch [1]
that is also part of this set.

[1]. https://lkml.org/lkml/2020/3/9/643

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

As with all the other coresight devices, groups are communicated to
coresight_register() and added to the csdev->dev in that function.

> 
> > +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}

Would you mind indicating where?  builtin_amba_driver() calls
amba_driver_register() and  that doesn't set the owner field.

Thanks,
Mathieu

> 
> greg k-h