Re: V4L2: Add a v4l2-subdev (soc-camera) driver for OmniVision OV9640 sensor

[David Ellingsworth <david@identd.dyndns.org>]

On Mon, Sep 14, 2009 at 4:30 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Mon, 14 Sep 2009, Marek Vasut wrote:
>
>> Dne Po 14. září 2009 21:29:26 Guennadi Liakhovetski napsal(a):
>> > From: Marek Vasut <marek.vasut@gmail.com>
>> >
>> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > ---
>> >
>> > Marek, please confirm, that this version is ok. I'll push it upstream for
>> > 2.6.32 then.
>>
>> No, it's not OK. You removed the RGB part. Either enclose those parts into ifdef
>> OV9640_RGB_BUGGY or preserve it in some other way. Someone will certainly want
>> to re-add RGB parts later and will have to figure it out all over again.
>
> Ok, make a proposal, how you would like to see it. But - I do not want
> commented out code, including "#ifdef MACRO_THAT_DOESNT_GET_DEFINED." I
> think, I described it in sufficient detail, so that re-adding that code
> should not take longer than 10 minutes for anyone sufficiently familiar
> with the code. Referencing another driver also has an advantage, that if
> we switch to imagebus or any other API, you don't get stale commented out
> code, but you look up updated code in a functional driver. But I am open
> to your ideas / but no commented out code, please.
>

I don't know much about this driver, but here's my $0.02. Since the
RGB code has known issues and this is a new driver, it is probably
best to submit it without the code for RGB support. As is, even
without direct support from the driver, users can retrieve RGB data
from the camera using libv4l. Your argument is therefore more or less
mute. If someone were to want to add RGB support to the driver, I'm
sure they'd prefer to have a clean and functional driver to work with
rather than wading through a bunch of dead/broken code.

Regards,

David Ellingsworth