[PATCH 01/13] drm/debugfs: Create helper to add debugfs files to device's list

[Daniel Vetter]

On Thu, Jan 12, 2023 at 10:50:40AM +0200, Jani Nikula wrote:
> On Wed, 11 Jan 2023, Maíra Canal <mcanal at igalia.com> 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 at igalia.com>
> > ---
> >  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