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
