Hans, 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 & > ISIF_HORZ_BC_WIN_COUNT_MASK; >val |= !!bc->horz.base_win_sel_calc << > ISIF_HORZ_BC_WIN_SEL_SHIFT; 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 & ISIF_HORZ_BC_WIN_COUNT_MASK) | ((!!bc->horz.base_win_sel_calc) << ISIF_HORZ_BC_WIN_SEL_SHIFT); val |= (!!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); val |= ((bc->horz.win_v_sz_calc & ISIF_HORZ_BC_WIN_V_SIZE_MASK) << ISIF_HORZ_BC_WIN_V_SIZE_SHIFT); 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 >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. Yes, isif_config_bclamp() set values in the register. > >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
