On Monday 29 March 2010 08:40:50 Hans Verkuil wrote:
> Hi Xiaolin,
>
> On Sunday 28 March 2010 16:42:30 Zhang, Xiaolin wrote:
> > From 1c18c41be33246e4b766d0e95e28a72dded87475 Mon Sep 17 00:00:00 2001
> > From: Xiaolin Zhang <[email protected]>
> > Date: Sun, 28 Mar 2010 21:31:24 +0800
> > Subject: [PATCH 2/10] This patch is second part of intel moorestown isp
> > driver and c files collection which is v4l2 implementation.
> >
>
> ....
>
> > +struct videobuf_dma_contig_memory {
> > + u32 magic;
> > + void *vaddr;
> > + dma_addr_t dma_handle;
> > + unsigned long size;
> > + int is_userptr;
> > +};
> > +
> > +#define MAGIC_DC_MEM 0x0733ac61
> > +#define MAGIC_CHECK(is, should)
> > \
> > + if (unlikely((is) != (should))) {
> > \
> > + pr_err("magic mismatch: %x expected %x\n", (is), (should));
> > \
> > + BUG();
> > \
> > + }
>
> I will do a more in-depth review in a few days.
I realize that I never got round to this. You have to remind me if you don't
see a follow-up within a week! These things tend to get buried otherwise.
I will try to review this this weekend. Let me know if you prefer me to wait
for the next patch series incorporating the comments already made by others.
Regards,
Hans
> However, I did notice that
> you added your own dma_contig implementation. What were the reasons for doing
> this? I've CC-ed Pawel since he will be interested in this as well.
>
> Another question that came up is: what is 'marvin'? It's clearly a codename,
> but a codename for what? This should be documented at the top of some source
> or header. Apologies if it is already documented, I didn't read everything
> yet.
>
> A final point I noticed: don't cast away a function result:
>
> (void)ci_isp_set_bp_detection(NULL);
>
> No need for (void). The gcc compiler won't warn about this unless the function
> is annotated with __must_check__.
>
> Regards,
>
> Hans
>
>
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
