RE: [PATCH v4 1/4] [media] s5p-mfc: Update MFCv5 driver for callback based architecture

[Arun Kumar K]

Hi Kamil,
Thank you for the review comments.
Will post v5 patches incorporating your comments.

Regards
Arun

------- Original Message -------
Sender : Kamil Debski<k.deb...@samsung.com>  Software Engineer/SPRC-Linux 
Platform (SSD)/Samsung Electronics
Date   : Aug 22, 2012 21:48 (GMT+05:30)
Title  : RE: [PATCH v4 1/4] [media] s5p-mfc: Update MFCv5 driver for callback
 based architecture

Hi Arun,

First of all - thank you for all the patches, they are getting better and
better
with every version.

Two things: 
1) A minor one - I suggest that the choice of the default pixel format should
be
done in a different manner. I suggest using a V4L2_PIX_FMT_* in the
DEF_SRC_FMT_DEC
define and then using find_format in s5p_mfc_dec_init. Same goes for encoding.

2) A major one - the ops mechanism could be improved. I see that there are
many functions that only call an ops. Such as:

> int s5p_mfc_alloc_dec_temp_buffers(struct s5p_mfc_ctx *ctx)
> {
>        return s5p_mfc_ops->s5p_mfc_alloc_dec_temp_buffers(ctx);
> }

I suggest dropping the s5p_mfc_ prefix in all the fields of s5p_mfc_hw_ops and
using a macro to call the ops. Like this:

+#define fimc_pipeline_call(f, op, p, args...)                          \r
+       (!(f) ? -ENODEV : (((f)->pipeline_ops && (f)->pipeline_ops->op) ? \r
+                           (f)->pipeline_ops->op((p), ##args) :
-ENOIOCTLCMD))
(from commit by Sylwester Nawrocki http://goo.gl/Q657j)

In addition, your code does not check whether the ops pointer is null and I
think that it should be done.

Best wishes,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

[snip the code]

<p>&nbsp;</p><p>&nbsp;</p>N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±™çbj)í…æèw*jg¬±¨¶‰šŽŠÝ¢j/�êäz¹Þ–Šà2ŠÞ™¨è­Ú&¢)ß¡«a¶Úþø®G«�éh®æj:+v‰¨Šwè†Ù¥