On Thu, Jan 12, 2023 at 10:50:40AM +0200, Jani Nikula wrote:
> On Wed, 11 Jan 2023, Maíra Canal <mcanal@xxxxxxxxxx> wrote:
> > Create a helper to encapsulate the code that adds a new debugfs file to
> > a linked list related to a object. Moreover, the helper also provides
> > more flexibily on the type of the object, allowing to use the helper for
> > other types of drm_debugfs_entry.
> >
> > Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_debugfs.c | 20 ++++++++++++--------
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > index 4f643a490dc3..255d2068ac16 100644
> > --- a/drivers/gpu/drm/drm_debugfs.c
> > +++ b/drivers/gpu/drm/drm_debugfs.c
> > @@ -316,6 +316,17 @@ void drm_debugfs_cleanup(struct drm_minor *minor)
> > minor->debugfs_root = NULL;
> > }
> >
> > +#define drm_debugfs_add_file_to_list(component) do { \
> > + entry->file.name = name; \
> > + entry->file.show = show; \
> > + entry->file.data = data; \
> > + entry->component = (component); \
> > + \
> > + mutex_lock(&(component)->debugfs_mutex); \
> > + list_add(&entry->list, &(component)->debugfs_list); \
> > + mutex_unlock(&(component)->debugfs_mutex); \
> > + } while (0)
>
> In general, please don't add macros that implicitly depend on certain
> local variable names. In this case, "entry".
>
> But I'm also not convinced about the usefulness of adding this kind of
> "generics". Sure, it'll save you a few lines here and there, but I think
> overall it's just confusing more than it's useful.
So the non-generics way I guess would be to
- pass the right pointer to the functions as an explicit parameter (struct
drm_device|crtc|connector *, )
- make drm_debugfs_entry and implementation detail
- switch the pointer in there to void *, have glue show functions for each
case which do nothing else than cast from void * to the right type
(both for the parameter and the function pointer)
- have a single function which takes that void *entry list and a pointer
to the debugfs director to add them all for code sharing
I think this should work for ->show, but for ->fops it becomes a rather
big mess I fear. Maybe for ->fops (and also for ->show for now) we leave
the explicit parameter out and just rely on seq_file->private or whatever
it was.
Or just copypaste, it's not that much code really :-)
-Daniel
>
>
> BR,
> Jani.
>
> > +
> > /**
> > * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list
> > * @dev: drm device for the ioctl
> > @@ -334,14 +345,7 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
> > if (!entry)
> > return;
> >
> > - entry->file.name = name;
> > - entry->file.show = show;
> > - entry->file.data = data;
> > - entry->dev = dev;
> > -
> > - mutex_lock(&dev->debugfs_mutex);
> > - list_add(&entry->list, &dev->debugfs_list);
> > - mutex_unlock(&dev->debugfs_mutex);
> > + drm_debugfs_add_file_to_list(dev);
> > }
> > EXPORT_SYMBOL(drm_debugfs_add_file);
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
