Hi Alex
On Fri, 20 Apr 2012, Alex Gershgorin wrote:
[snip]
> > Signed-off-by: Alex Gershgorin <alexg@xxxxxxxxxxxxxx>
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
>
> > > 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.
>
> >> 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]
> > @@ -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);
>
> > > 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
