I remember there was a comment against an earlier patch that asks
for combining such statements since it makes the function appear
as big. Not sure who had made that comment. That is the reason you
find code like this in this patch. It was initially done with multiple
OR statements to construct the value to be written to the register as you 
stated below as 

>val = bc->horz.win_count_calc &
>val |= !!bc->horz.base_win_sel_calc <<

I have checked few other drivers such as bt819.c ir-kbd-i2c.c,
mt9m111.c etc, where similar statements are found, but they have used hardcoded 
values masks which makes it appears less complex. But I 
like to reduce magic numbers as much possible in the code.

I think what I can do is to  combine maximum of 2 such expressions in a 
statement which might make it less complex to read. Such as 

val = (bc->horz.win_count_calc &
        ((!!bc->horz.base_win_sel_calc) <<

val |= (!!bc->horz.clamp_pix_limit) <<
         ((bc->horz.win_h_sz_calc &
val |= ((bc->horz.win_v_sz_calc &

Also to make the line fits in 80 characters, I will consider reducing
the number of characters in #define names such as

val = (bc->horz.win_count_calc & HZ_BC_WIN_CNT_MASK) |
((!!bc->horz.base_win_sel_calc) << HZ_BC_WIN_SEL_SHIFT) |
(!!bc->horz.clamp_pix_limit) << HZ_BC_PIX_LIMIT_SHIFT);

Let me know if you don't agree.

>Of course, in this particular piece of code from the function
>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.

Yes, isif_config_bclamp() set values in the register.

>It would be interesting to know if people know of good ways of making
>code like this more elegant (or at least less awkward).
>       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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to