RE: [PATCH v1] ARM: i.mx: mx3fb: add overlay support

Hi Alex

On Fri, 20 Apr 2012, Alex Gershgorin wrote:

[snip]

quoted

Signed-off-by: Alex Gershgorin <redacted>
Signed-off-by: Guennadi Liakhovetski <redacted>

quoted

quoted

Thanks for the credit (;-)), but no, putting my Sob after yours means,
that I took your patch and forwarded it on to the next maintainer, which
is clearly not the case here:-) The original i.MX31 framebuffer overlay
code from my old patches also clearly wasn't written by me, since I didn't
have a chance to test it. So, if you like, you can try to trace back
original authors of that code and ask them, how they want to be credited
here,

I would like to thank all the authors of original code.
unfortunately I can't thank for each one of you separately by name, i hope
that you understand and accept it.

quoted

quoted

 otherwise just mentioning, that this work is based on some earlier
patch series "i.MX31: dmaengine and framebuffer drivers" from 2008 by ...
should be enough.

This option is more suitable, I just correct the description of the patch,
and leave your signature (if you have any objections?) since 2008 patch version.

Well, if you wish so...:-) To me it looks like a new patch from you, 
that's just vaguely based on my previous patch, that was copying some 
previous work, so, my contribution to this code isn't huge;-) But if you 
insist - you can keep my Sob, but at least put it above yours.

[snip]

quoted

@@ -1333,8 +1534,8 @@ static int init_fb_chan(struct mx3fb_data *mx3fb, struct idmac_channel *ichan)
      ichan->client = mx3fb;
      irq = ichan->eof_irq;

-     if (ichan->dma_chan.chan_id != IDMAC_SDC_0)
-             return -EINVAL;
+     switch (ichan->dma_chan.chan_id) {
+     case IDMAC_SDC_0:

      fbi = mx3fb_init_fbinfo(dev, &mx3fb_ops);

quoted

quoted

I would bite the bullet and indent this case block...

This makes a clear separation between the framebuffer and overlay
channels during initializing, but if you have any ideas welcome, please
send, I could do a test on my hardware :-)

Sorry, I didn't mean any functional change, just a pure formatting issue: 
you put a "switch-case" statement above, but didn't add an indentation 
level to the following code. While reducing the patch size by avoiding 
unnecessary changes is good, I think, following the coding style and 
improving readability are more important arguments here, so, I would go 
and do that "unnecessary" change and indent the code.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/