Messages in this thread | | | From | Mathieu Poirier <> | Date | Wed, 18 Mar 2020 12:15:04 -0600 | Subject | Re: [PATCH 01/13] coresight: cti: Initial CoreSight CTI Driver |
| |
On Wed, 18 Mar 2020 at 12:12, Mathieu Poirier <mathieu.poirier@linaro.org> wrote: > > 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
Correction - this link should be:
https://lkml.org/lkml/2020/3/9/642
> > > > > > +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
| |