Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: KjlCnnBbhacVC+Qu+49V5qs1h8bUOGYC2gJzjS3jZOmIzynutONt/3J/kquBAvp6Zyf7zcJNgL
 VzBno0pFAfbw==
IronPort-SDR: gdOzEhnqEgsoR8sAYDzfV05CATH4fzVWWsS+k0boW8O/yu3Xfi3sJxeG235iqhxdcOZT0BRZiL
 Wb4kADP/oc4Q==
Date:   Thu, 2 Apr 2020 10:32:20 +0300
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Hans Verkuil <hverkuil@xs4all.nl>, Petr Mladek <pmladek@suse.com>,
        linux-media@vger.kernel.org,
        Dave Stevenson <dave.stevenson@raspberrypi.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        laurent.pinchart@ideasonboard.com, mchehab@kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH 1/1] lib/vsprintf: Add support for printing V4L2 and DRM
 fourccs
Message-ID: <20200402073220.GB1522@kekkonen.localdomain>
References: <20200401140522.966-1-sakari.ailus@linux.intel.com>
 <080ddddf-717e-61dc-8522-fbdbe52da94a@xs4all.nl>
 <20200401151332.GU1922688@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200401151332.GU1922688@smile.fi.intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Andy,

Thanks for the review.

On Wed, Apr 01, 2020 at 06:13:32PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 01, 2020 at 04:13:51PM +0200, Hans Verkuil wrote:
> > On 4/1/20 4:05 PM, Sakari Ailus wrote:
> > > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > > the same implementation can be used.
> 
> %p4cc ?

Sounds good. Numbers have special handling but AFAIR only right after %
sign, so this should be possible.

> 
> > > +	char ch[2] = { 0 };
> > 
> > This can just be '{ };'
> 
> The latter is GCC extension, while above is C standard. Former is slightly
> better I think. Though see below.
> 
> > > +	unsigned int i;
> > > +
> > > +	if (check_pointer(&buf, end, fourcc, spec))
> > > +		return buf;
> > > +
> > > +	switch (fmt[1]) {
> > > +	case 'f':
> 
> > > +		for (i = 0; i < sizeof(*fourcc); i++) {
> > > +			ch[0] = *fourcc >> (i << 3);
> > 
> > You need to AND with 0x7f, otherwise a big endian fourcc (bit 31 is set)
> > will look wrong. Also, each character is standard 7 bit ascii, bit 7 isn't
> > used except to indicate a BE variant.
> 
> Why not to do it once by a flag and do reset it once?
> 
> 	u32 tmp = *fourcc;
> 	bool be4cc = tmp & BIT(31);
> 
> 	tmp &= BIT(31);

I had two extra temporary variables in a version I didn't send but I
figured they could be removed. :-)

> 
> On top of that, as promised above, why not simple do it in a simpler way, i.e.
> using standard idiom:
> 
> 	for (i = 0; i < sizeof(*fourcc); i++) {
> 		if (buf < end)
> 			*buf = tmp >> (i * 8);
> 		buf++;
> 	}
> ?

I guess that's at least more efficient, and comparing buf to end is
trivial. I'll do that in v2.

> 
> > > +			buf = string(buf, end, ch, spec);
> > > +		}
> > > +
> > > +		if (*fourcc & BIT(31))
> > > +			buf = string(buf, end, "-BE", spec);
> 
> Another possibility
> 
> 	u8 ch[8];
> 
> 	if (*fourcc & BIT(31)) {
> 		put_unaligned_be32(tmp, &ch[0]);
> 		strcpy(&ch[4], "-BE");
> 	} else {
> 		put_unaligned_le32(tmp, &ch[0]);
> 		strcpy(&ch[4], "-LE");
> 	}
> 	return string(buf, end, &ch[0], spec);

I think I prefer the loop. I figured you can only call string once,
otherwise field width handling will be broken. Let's see.

> 
> > > +		return buf;
> > > +	default:
> > > +		return error_string(buf, end, "(%pp?)", spec);
> > > +	}
> > > +}
> 

-- 
Kind regards,

Sakari Ailus