Re: [PATCH] video: simplefb: add mode parsing function

On 05/24/2013 01:30 AM, Alex Courbot wrote:

On 05/24/2013 01:27 AM, Stephen Warren wrote:

quoted

quoted

Stephen, please note that the "r5g6b5" mode initially supported
by the driver becomes "b5g6r5" with the new function. This is because
the least significant bits are defined first in the string - this
makes parsing much easier, notably for modes which do not fill whole
bytes (e.g. 15 bit modes). I hope this is not a problem - it's probably
better to do the change now while the driver is still new.

...

quoted

quoted

+    by a given color channel. Value channels are "r" (red), "g"
(green),
+    "b" (blue) and "a" (alpha).

I think you'll need "x" too, to represent any gaps/unused bits.

Are there such formats? 15-bit formats do exist but AFAIK they just drop
the MSB. Anyway, this can be done easily, so I won't argue. :)

Yes, I believe there are e.g. both a2r10g10b10 and b10g10r10a2 for
example, and it's quite common to replace a with x, especially for
scanout buffers. Google certainly has hits for that.

quoted

quoted

+    if (!params->format || IS_ERR(params->format)) {

That's just saying IS_ERR_OR_NULL(params->format). It'd be better to
pick one thing to represent errors; either NULL or an error-pointer. I
think having simplefb_parse_format() just return NULL on error would be
best; the caller can't really do anything useful with the information
anyway.

Weird - I actually removed that NULL check since the parse function can
only return a valid pointed an error code. Probably forgot to
format-patch again after that.

It seems more customary to let the function that fails decide the error
code and have it propagated by its caller (even though in the current
case there is no much variety in the returned error codes). If you see
no problem with it, I will stick to that way of doing.

Using just an error-return is probably fine. I was going to say that the
table lookup might propagate a NULL all the way through to that check,
and hence require both checks above, but I guess that case can't happen,
since if there is no table entry, then simplefb_parse_format() will
always be called, so that's fine.