[Outreachy kernel] Re: [PATCH v2 1/3] drm: Extract drm_debug.[hc]

[Daniel Vetter]

On Fri, Oct 13, 2017 at 03:21:19PM -0600, Haneen Mohammed wrote:
> On Thu, Oct 12, 2017 at 11:35:12AM +0100, Chris Wilson wrote:
> > Quoting Haneen Mohammed (2017-10-12 03:32:53)
> > > diff --git a/drivers/gpu/drm/drm_debug.c b/drivers/gpu/drm/drm_debug.c
> > > new file mode 100644
> > > index 0000000..a79593f
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_debug.c
> > > @@ -0,0 +1,75 @@
> > > +/*
> > > + * Copyright 2001 VA Linux Systems, Inc., Sunnyvale, California.
> > > + * All Rights Reserved.
> > > + *
> > > + * Author Rickard E. (Rik) Faith <faith at valinux.com>
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining a
> > > + * copy of this software and associated documentation files (the "Software"),
> > > + * to deal in the Software without restriction, including without limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including the next
> > > + * paragraph) shall be included in all copies or substantial portions of the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > > + * PRECISION INSIGHT AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > > + * DEALINGS IN THE SOFTWARE.
> > > + */
> > > +
> > > +#include <drm/drm_debug.h>
> > > +#include <drm/drmP.h>
> > > +
> > > +#define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"
> > > +
> > > +void drm_dev_printk(const struct device *dev, const char *level,
> > > +                   unsigned int category, const char *function_name,
> > > +                   const char *prefix, const char *format, ...)
> > > +{
> > > +       struct va_format vaf;
> > > +       va_list args;
> > > +
> > > +       if (category != DRM_UT_NONE && !(drm_debug & category))
> > > +               return;
> > > +
> > > +       va_start(args, format);
> > > +       vaf.fmt = format;
> > > +       vaf.va = &args;
> > > +
> > > +       if (dev)
> > > +               dev_printk(level, dev, DRM_PRINTK_FMT, function_name, prefix,
> > > +                          &vaf);
> > > +       else
> > > +               printk("%s" DRM_PRINTK_FMT, level, function_name, prefix, &vaf);
> > > +
> > > +       va_end(args);
> > > +}
> > > +EXPORT_SYMBOL(drm_dev_printk);
> > > +
> > > +void drm_printk(const char *level, unsigned int category,
> > > +               const char *format, ...)
> > > +{
> > > +       struct va_format vaf;
> > > +       va_list args;
> > > +
> > > +       if (category != DRM_UT_NONE && !(drm_debug & category))
> > > +               return;
> > > +
> > > +       va_start(args, format);
> > > +       vaf.fmt = format;
> > > +       vaf.va = &args;
> > > +
> > > +       printk("%s" "[" DRM_NAME ":%ps]%s %pV",
> > > +              level, __builtin_return_address(0),
> > > +              strcmp(level, KERN_ERR) == 0 ? " *ERROR*" : "", &vaf);
> > > +
> > > +       va_end(args);
> > > +}
> > > +EXPORT_SYMBOL(drm_printk);
> > 
> > We already have drm_print.c, currently used to house drm_printf and the
> > drm_printer. It might be a bit confusing to have drm_printk and
> > drm_printf next to each other, but less confusing that calling user
> > error messages drm_debug.c.
> > -Chris
> 
> I didn't notice that. 
> Should I move these functions and macros to drm_print.[hc] instead then?

Yeah I think this makes sense, instead of having yet another file with
some big overlap with drm_print.[hc]. That also solves the problem of
where to put the kerneldoc include :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch