Em 03-08-2010 12:16, Pawel Osciak escreveu:
> Hi Mauro,
>
> please pull the s5p-fimc driver with your yesterday's comments addressed.
>
>
> The following changes since commit c57fd88318988f17731e446fe1d8498f506fdd44:
>
> V4L/DVB: uvcvideo: Add support for Manta MM-353 Plako (2010-07-05 19:47:16
> -0300)
>
> are available in the git repository at:
> git://git.infradead.org/users/kmpark/linux-2.6-samsung v4l/s5p-fimc-v4
>
> Sylwester Nawrocki (1):
> v4l: Add driver for Samsung S5P SoC video postprocessor
>
> Documentation/DocBook/v4l/pixfmt-packed-rgb.xml | 78 ++
> drivers/media/video/Kconfig | 9 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/s5p-fimc/Makefile | 3 +
> drivers/media/video/s5p-fimc/fimc-core.c | 1570
> +++++++++++++++++++++++
> drivers/media/video/s5p-fimc/fimc-core.h | 465 +++++++
> drivers/media/video/s5p-fimc/fimc-reg.c | 527 ++++++++
> drivers/media/video/s5p-fimc/regs-fimc.h | 293 +++++
> include/linux/videodev2.h | 1 +
> 9 files changed, 2947 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/s5p-fimc/Makefile
> create mode 100644 drivers/media/video/s5p-fimc/fimc-core.c
> create mode 100644 drivers/media/video/s5p-fimc/fimc-core.h
> create mode 100644 drivers/media/video/s5p-fimc/fimc-reg.c
> create mode 100644 drivers/media/video/s5p-fimc/regs-fimc.h
I still found a few minor issues:
+/* Nothing done in job_abort. */
+static void fimc_job_abort(void *priv) {}
This callback violates CodingStyle. It would be better if coded as:
static void fimc_job_abort(void *priv) {
/* Nothing done in job_abort. */
}
To look as a real function.
+struct videobuf_queue_ops fimc_qops = {
+ .buf_setup = fimc_buf_setup,
+ .buf_prepare = fimc_buf_prepare,
+ .buf_queue = fimc_buf_queue,
+ .buf_release = fimc_buf_release,
+};
This is also static.
+#define ctx_m2m_get_frame(frame, ctx, type) do { \
+ if (V4L2_BUF_TYPE_VIDEO_OUTPUT == (type)) { \
+ frame = &(ctx)->s_frame; \
+ } else if (V4L2_BUF_TYPE_VIDEO_CAPTURE == (type)) { \
+ frame = &(ctx)->d_frame; \
+ } else { \
+ v4l2_err(&(ctx)->fimc_dev->m2m.v4l2_dev,\
+ "Wrong buffer/video queue type (%d)\n", type); \
+ return -EINVAL; \
+ } \
+} while (0)
>From Documentation/CodingStyle:
1) macros that affect control flow:
#define FOO(x) \
do { \
if (blah(x) < 0) \
return -EBUGGERED; \
} while(0)
is a _very_ bad idea. It looks like a function call but exits the
"calling"
function; don't break the internal parsers of those who will read the
code.
This kind of macros obfuscate the logic. Instead, store the error condition on
a temporary
var and return its value to the macro caller, letting it to return on an error,
or code it
as a "static inline" function.
I'll be pulling the patch on my tree, but I expect patches fixing those pointed
issues soon.
Cheers,
Mauro.
--
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
