Hi Kamil,
On Fri, May 9, 2014 at 1:31 AM, Kamil Debski <k.debski@xxxxxxxxxxx> wrote:
Hi Arun,
I think that this driver is getting too complicated now.
First there are separate files for MFC versions, but in addition there are
many
IF_MFCVx in there.
The intention of this patch is to actually get rid of IF_MFCVx
conditionals wherever possible.
I am curious how many additional lines it would take to
add s5p_mfc_cmd_v8.* and s5p_mfc_opr_v8.*.
I get the point that this approach may result in less lines added, but
having a callback specific for version use register pointers specific for
another version makes the code look unreadable and difficult to maintain.
Could you please give an example of how this reduces readability?
I personally feel this patch makes things much more readable (see below).
On the other hand, if we continued without the current method, we
would have to sprinkle
IF_MFCVx macros all around actual functions/operations, instead of
just containing this
to the regs structure, and the only difference in each path would be
register name defines.
I don't feel this would be a better direction to be honest.
Compare, new, after this patch:
+ WRITEL(y_addr, mfc_regs->e_source_first_plane_addr);
+ WRITEL(c_addr, mfc_regs->e_source_second_plane_addr);
vs previously, before this patch:
- if (IS_MFCV7(dev)) {
- WRITEL(y_addr, S5P_FIMV_E_SOURCE_FIRST_ADDR_V7);
- WRITEL(c_addr, S5P_FIMV_E_SOURCE_SECOND_ADDR_V7);
- } else {
- WRITEL(y_addr, S5P_FIMV_E_SOURCE_LUMA_ADDR_V6);
- WRITEL(c_addr, S5P_FIMV_E_SOURCE_CHROMA_ADDR_V6);
- }
And of course adding V8 more would make it even worse with yet another
else if case.
Please give your opinion on another way to add support for v8.
s5p_mfc_cmd_v8.* and s5p_mfc_opr_v8.* ?
If we add v7 and v8 files, a majority of their code will look like this:
s5p_mfc_opr_v6.c:
(...)
void foo_v6(args)
{
foofun(REGISTER_A_V6);
barfun(REGISTER_B_V6);
}
(...)
s5p_mfc_opr_v7.c:
(...)
void foo_v7(args)
{
foofun(REGISTER_A_V7);
barfun(REGISTER_B_V7);
}
(...)
s5p_mfc_opr_v8.c:
(...)
void foo_v8(args)
{
foofun(REGISTER_A_V8);
barfun(REGISTER_B_V8);
}
(...)
I'm not sure this is less error prone and less code...
