Note that the other patches from this series are fine as far as I am concerned. One general note: I always have difficulties with constructions like this: > + val = (bc->horz.win_count_calc & > + ISIF_HORZ_BC_WIN_COUNT_MASK) | > + ((!!bc->horz.base_win_sel_calc) << > + ISIF_HORZ_BC_WIN_SEL_SHIFT) | > + ((!!bc->horz.clamp_pix_limit) << > + ISIF_HORZ_BC_PIX_LIMIT_SHIFT) | > + ((bc->horz.win_h_sz_calc & > + ISIF_HORZ_BC_WIN_H_SIZE_MASK) << > + ISIF_HORZ_BC_WIN_H_SIZE_SHIFT) | > + ((bc->horz.win_v_sz_calc & > + ISIF_HORZ_BC_WIN_V_SIZE_MASK) << > + ISIF_HORZ_BC_WIN_V_SIZE_SHIFT); It's just about impossible for me to parse. And some of the patches in this series are full of such constructs. Unfortunately, I do not have a magic bullet solution. In some cases I suspect that a static inline function can help. In other cases it might help to split it up in smaller parts. For example: u32 tmp; val = bc->horz.win_count_calc & ISIF_HORZ_BC_WIN_COUNT_MASK; val |= !!bc->horz.base_win_sel_calc << ISIF_HORZ_BC_WIN_SEL_SHIFT; val |= !!bc->horz.clamp_pix_limit << ISIF_HORZ_BC_PIX_LIMIT_SHIFT; tmp = bc->horz.win_h_sz_calc & ISIF_HORZ_BC_WIN_H_SIZE_MASK; val |= tmp << ISIF_HORZ_BC_WIN_H_SIZE_SHIFT; tmp = bc->horz.win_v_sz_calc & ISIF_HORZ_BC_WIN_V_SIZE_MASK; val |= tmp << ISIF_HORZ_BC_WIN_V_SIZE_SHIFT; Of course, in this particular piece of code from the function isif_config_bclamp() I am also wondering why bc->horz.win_h_sz_calc and bc->horz.win_v_sz_calc need to be ANDed anyway. I would expect that to happen when these values are set. But I did not look at this in-depth, so I may well have missed some subtlety here. It would be interesting to know if people know of good ways of making awkward code like this more elegant (or at least less awkward). Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- 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
