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

[Mathieu Poirier]

On Wed, 18 Mar 2020 at 12:12, Mathieu Poirier
<mathieu.poirier@xxxxxxxxxx> 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@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

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