Hi Daniele,
> > On Thu, Feb 27, 2020 at 06:28:42PM -0800, Daniele Ceraolo Spurio wrote:
> > > uC is a component of the GT, so it makes sense for the uC debugfs files
> > > to be in the GT folder. A subfolder has been used to keep the same
> > > structure we have for the code.
> >
> > Can we please document the interface changes. I see there are
> > some differences between the original and the new interfaces.
> >
>
> What differences are you referring to? there aren't supposed to be any,
> aside from the path change.
Have I seen it wrong or there are new files in this patch?
In any case, maybe we need to have the new structure.
> > > +#define DEFINE_UC_DEBUGFS_ATTRIBUTE(__name) \
> > > + static int __name ## _open(struct inode *inode, struct file *file) \
> > > +{ \
> > > + return single_open(file, __name ## _show, inode->i_private); \
> > > +} \
> > > +static const struct file_operations __name ## _fops = { \
> > > + .owner = THIS_MODULE, \
> > > + .open = __name ## _open, \
> > > + .read = seq_read, \
> > > + .llseek = seq_lseek, \
> > > + .release = single_release, \
> > > +}
> >
> > Why do we need DEFINE_UC_DEBUGFS_ATTRIBUTE()?
> >
> > DEFINE_GT_DEBUGFS_ATTRIBUTE() was meant to be common to all gt
> > debugfs. I there any reason we need a new one?
> >
>
> Just wanted to avoid including the other header just for this macro.
well that was supposed to be a library for all the gem/debugfs
files and avoid duplicated code, I don't see anything wrong with
including the file.
> > > +struct debugfs_uc_file {
> > > + const char *name;
> > > + const struct file_operations *fops;
> > > +};
> > > +
> > > +#define debugfs_uc_register_files(files__, root__, data__) \
> > > +do { \
> > > + int i__ = 0; \
> > > + for (i__ = 0; i__ < ARRAY_SIZE(files__); i__++) { \
> > > + debugfs_create_file(files__[i__].name, \
> > > + 0444, root__, data__, \
> > > + files__[i__].fops); \
> > > + } \
> > > +} while (0)
> >
> > You want to define your own debugfs_uc_register_files() instead
> > of using debugfs_gt_register_files() because you want "data__"
> > to be void, right?
> >
> > I think we can achieve that by adding a wrapper in debugfs_gt.c,
> > perhaps we can do something like:
> >
> > void __debugfs_gt_register_files(struct intel_gt *gt,
> > struct dentry *root,
> > const struct debugfs_gt_file *files,
> > void *data,
> > unsigned long count)
> > {
> > ......
> > }
> >
> > and
> >
> > #define debugfs_gt_register_files(...) __debugfs_gt_register_files(...)
> > #define debugfs_uc_register_files(...) __debugfs_gt_register_files(...)
> >
> > so that we can keep everything in a library. What do you think.
> >
>
> LGTM. Mind if I rename to:
>
> intel_gt_debugfs_register(...)
> intel_uc_debugfs_register(...)
>
> to avoid the debugfs_* prefix, as pointed out by Jani?
I have a patch for it, can you please hold a little, unless, of
course, yours is already ready.
Obvously, the naming you propose makes sense.
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
