Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: BaL9/nUIttOaIv7FOT7OpJSxM2jDEBrk7Qm+TQTycle6UniRyHE5JmPJz9gjEYMKfv0nABD0/Y
 GiUS+YpbIUpg==
IronPort-SDR: R+vTNA8KsI7v/v3QB9ncTDcyBiWLW0KQZIWz8nfziTZynxrchxmy56k75BSox1RrD4HxmfOCSd
 esSfNbQ2d5sA==
Date:   Mon, 15 Feb 2021 15:48:30 +0200
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
        Petr Mladek <pmladek@suse.com>,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        dri-devel@lists.freedesktop.org, hverkuil@xs4all.nl,
        laurent.pinchart@ideasonboard.com, mchehab@kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Joe Perches <joe@perches.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>
Subject: Re: [PATCH v7 3/3] drm: Switch to %p4cc format modifier
Message-ID: <20210215134830.GG3@paasikivi.fi.intel.com>
References: <20210215114030.11862-1-sakari.ailus@linux.intel.com>
 <20210215114030.11862-4-sakari.ailus@linux.intel.com>
 <YCp5+okZ2M0SLTY9@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YCp5+okZ2M0SLTY9@smile.fi.intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

Hi Andy,

On Mon, Feb 15, 2021 at 03:41:14PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 15, 2021 at 01:40:30PM +0200, Sakari Ailus wrote:
> > Switch DRM drivers from drm_get_format_name() to %p4cc. This gets rid of a
> > large number of temporary variables at the same time.
> 
> What a nice clean up!
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks!

> after addressing nit-picks below.
> 
> Side note (no need to implement esp. right now): it seems often it's coupled
> with modifier, would be nice to have them together that %p4ccM or so can do it
> in one go.

...

> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 03262472059c..5cf45aa6eedc 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -30,11 +30,6 @@
> >  #include <drm/drm_device.h>
> >  #include <drm/drm_fourcc.h>
> >  
> > -static char printable_char(int c)
> > -{
> > -	return isascii(c) && isprint(c) ? c : '?';
> > -}
> 
> If it goes as an ABI than your dot is incompatible with this and '?' should be
> used instead in the patch 1. And I bend towards suggested '?' rather than '.'.
> 
> Also it means that you probably would need different specifiers for full and
> short formats.

I thought of that, but then the resulting string would be indeed different
and comparing short and long formats would be harder. Remember this is for
debug prints. If the format is too long, then it should be made shorter,
but during the earlier review rounds people have expressed interest in
having this information there.

"?" can be expanded by the shell whereas "." is not. If DRM folks think we
should go back to "?" I'm fine with that. Also note that there's something
wrong with the fourcc code to begin with if it's got either "." or "?".

-- 
Regards,

Sakari Ailus