> >> + * Copyright (C) Nicolas VIVIEN
> >
> > It would be interesting to have Nicolas SOB as well, if possible.
>
> I don't think he ever knows about this version of the driver. I got his GPL
> driver, cleaned up -- coding style, v4l1 and v4l2 ioctl conversion to v4l2
> functions, some bug fixes and so on... If you still want him to sign this
> of, I'll try my best to catch him but can't guarantee any results.
It would be nice. I can accept it without his ack, but it would be
better to have it, if possible.
>
> >> +
> >> +#ifndef CONFIG_STK11XX_DEBUG_STREAM
> >> +#define CONFIG_STK11XX_DEBUG_STREAM 0
> >> +#endif
> >> +
> >> +#if CONFIG_STK11XX_DEBUG_STREAM
> >
> > I would instead use:
> > #ifdef CONFIG_STK11XX_DEBUG_STREAM
>
> Hmm, no, I would rather get rid of CONFIG_ thing, it may make things
> unclear, beacuse there is (will be) no option in Kconfig for this, because
> this is the most verbose option for the driver mainly used for algorithms
> debugging.
Seems ok to me.
> > We don't do format conversions in kernel. Instead, you should return a
> > proper Bayer Fourcc format (like V4L2_PIX_FMT_SBGGR8).
>
>
> Ok, there is a debate about this, I will do the changes after some decision
> will be made.
As you wish.
> > Please use instead the load_firmware routines. It is not a good idea to
> > have firmware inside the kernel. Also, this might rise some legal issues
> > due to licensing models.
>
> Markus wrote:
> <cite>
> Jiri, are you allowed to include that microcode, did you get any
> information about this from the manufacturer which could allow the
> inclusion?
> The sequences are rather small not putting it into extra firmware
> files would make life much easier for some users, on the other side if
> it raises legal issues Mauro's right with loading it from a file
> </cite>
>
> This seems to be a reverse engineered driver, I think, all those values are
> intercepted, so there are no licensing issues.
Are those a code, or just another internal driver configuration (for
example, maybe some register initialization inside the sensors)? We
should take care to avoid adding material here that can be later
complained.
> >
> > Instead of using all those write, you should consider creating a table
> > of values and use something like:
> > stk11xx_write_regs(dev, table1);
>
> There is a problem with this approach. There are reads every 3-5 writes and
> this can grow into many small tables.
Maybe you can do this then just for the bigger tables.
> > You may also consider writing a separate c file for stk1135. Having a
> > large .c file is not very nice. The better is to split the code into a
> > few parts.
>
> I don't like many files for one driver and finding little pieces of code
> in each file separately -- 1125 + 1235 will be small pieces. Not considering
> the static functions and warning about unused code. But it's up to you, it's
> your subtree, make a decision.
Your driver have about 3600 lines. We target to keep newer files with a
maximum of about 1000 lines on the same file (unfortunately, some
drivers are bigger than that), separating the driver into logical
pieces. I think it would be interesting to split it into two or tree
files.
>
> >> +static void *stk11xx_rvmalloc(unsigned long size)
> >
> > Another rvmalloc implementation? You should consider using the one
> > already at kernel.
>
> What's the name, I can't find it?
There are some rvmalloc on cpia, cpia2, em28xx, ...
What the current drivers are doing is to replace it to vmalloc_32:
#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,15)
if ((buff = rvmalloc(dev->num_frames * imagesize))) {
#else
if ((buff = vmalloc_32(dev->num_frames * imagesize))) {
#endif
>
> The rest of comments has been applied, thanks,
You're welcome.
--
Cheers,
Mauro
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
