Re: [PATCH 03/51] drm: add managed resources tied to drm_device

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Sam Ravnborg <sam@ravnborg.org>,
	"Nikula, Jani" <jani.nikula@linux.intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Marco Felsch <m.felsch@pengutronix.de>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 03/51] drm: add managed resources tied to drm_device
Date: Sat, 29 Feb 2020 12:28:42 +0100	[thread overview]
Message-ID: <CAKMK7uEYxM8BAsp+DHUxw+qdE_B3J+ePAxC-j0V+v+J6trffgw@mail.gmail.com> (raw)
In-Reply-To: <20200229111710.GB3674@ravnborg.org>

On Sat, Feb 29, 2020 at 12:17 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> > > > + *
> > > > + * Based on drivers/base/devres.c
> > > > + */
> > > > +
> > > > +#include <drm/drm_managed.h>
> > > > +
> > > > +#include <linux/list.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/spinlock.h>
> > > > +
> > > > +#include <drm/drm_device.h>
> > > > +#include <drm/drm_print.h>
> > >
> > > It is good practice to group the include files.
> > > And drm/ comes after linux/
> >
> > I try to put the main header first to make sure it's stand-alone, but
> > I guess that works with the header check now? Do I need to do anything
> > to get that checked?
>
> The header-check infrastructure was dropped again - see:
> fcbb8461fd2376ba3782b5b8bd440c929b8e4980

Uh I'm disappoint :-/

Adding Jani in case he missed this too. I guess maybe we should
resurrect it for drm again (and with a file pattern starting in a
.dot).

> > > > +     /**
> > > > +      * @managed:
> > > > +      *
> > > > +      * Managed resources linked to the lifetime of this &drm_device as
> > > > +      * tracked by @ref.
> > > > +      */
> > > > +     struct {
> > > > +             struct list_head resources;
> > > > +             void *final_kfree;
> > > > +             spinlock_t lock;
> > > > +     } managed;
> > >
> > > I am missing kernel-doc here.
> > > At least document that lock is used to guard access to resources.
> > > (s/lock/lock_resources/ ?)
> >
> > Dunno why, but the support for name sub-structures seems to have
> > broken in kerneldoc. So I can type it, but it's not showing up, so I
> > didn't bother. Well I had it, but deleted it again. It's still
> > documented to work, but I have no idea what I'm doing wrong.
>
> Most readers prefer the .c files as the source.
> I personally read the generated kernel doc when I google
> and when I check that my own stuff looks good in kernel-doc format.
> So comments are still valueable despite not being picked up by
> kernel-doc.
> You know this - but I just wanted to encourage you to write the few
> lines that may help me and others :-)

Hm I thought way back this actually worked. Again ping for Jani, he's
better on top of what's happening in kernel-doc land.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel