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

[Greg KH]

On Wed, Mar 18, 2020 at 12:12:26PM -0600, Mathieu Poirier wrote:
> > 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,

Then they need to be fixed.  Seriously, we've been doing this and
fighting this for 15 years, not giving up yet! :)


> but this can be split in two entries.

Please do.

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

As I reported in the other email, that is not the correct format to use.

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

Ah, ok, missed that, sorry.

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

Yes, it doesn't, I'm saying that the amba bus code should be fixed, not
that this is a bug here, just complaining in general :)

thanks,

greg k-h