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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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...)                          \
+       (!(f) ? -ENODEV : (((f)->pipeline_ops && (f)->pipeline_ops->op) ? \
+                           (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]

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux