Messages in this thread | | | Date | Wed, 18 Mar 2020 12:12:26 -0600 | From | Mathieu Poirier <> | Subject | Re: [PATCH 01/13] coresight: cti: Initial CoreSight CTI Driver |
| |
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@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...
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
| |