On Wed, Dec 16, 2009 at 13:11:57, Hans Verkuil wrote:
> On Wednesday 16 December 2009 00:37:52 Karicheri, Muralidharan wrote:
> > 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 &
> > >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.
> Personally I have mixed feelings about the use for symbolic names for things
> like this. The problem is that they do not help me understanding the code.
> The names tend to be long, leading to these broken up lines, and if I want
> to know how the bits are distributed in the value I continuously have to
> do the look up in the header containing these defines.
> Frankly, I have a similar problem with using symbolic names for registers.
> Every time I need to look up a register in the datasheet I first need to
> look up the register number the register name maps to. All datasheets seem
> to be organized around the register addresses and there rarely is a datasheet
> that has an index of symbolic names.
> Using hard-coded numbers together with a well written comment tends to be much
> more readable in my opinion. I don't really think there is anything magic 
> about
> these numbers: these are exactly the numbers that I need to know whenever I
> have to deal with the datasheet. Having to go through a layer of obfuscation
> is just plain irritating to me.
> However, I seem to be a minority when it comes to this and I've given up
> arguing for this...
> Note that all this assumes that the datasheet is publicly available. If it
> is a reversed engineered driver or based on NDA datasheets only, then the
> symbolic names may be all there is to make you understand what is going on.


> That seems overkill. I actually think it can be improved a lot by visually
> grouping the lines:
>                      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);
> Now I can at least see the various values that are ORed.

I liked this piece of code from drivers/mtd/nand/s3c2410.c. Serves as
a good template to do this sort of thing.

#define S3C2410_NFCONF_TACLS(x)    ((x)<<8)
#define S3C2410_NFCONF_TWRPH0(x)   ((x)<<4)
#define S3C2410_NFCONF_TWRPH1(x)   ((x)<<0)

[Okay, spaces around '<<', please :)]


        if (plat != NULL) {
                tacls = s3c_nand_calc_rate(plat->tacls, clkrate, tacls_max);
                twrph0 = s3c_nand_calc_rate(plat->twrph0, clkrate, 8);
                twrph1 = s3c_nand_calc_rate(plat->twrph1, clkrate, 8);


                mask = (S3C2410_NFCONF_TACLS(3) |
                        S3C2410_NFCONF_TWRPH0(7) |
                set = S3C2410_NFCONF_EN;
                set |= S3C2410_NFCONF_TACLS(tacls - 1);
                set |= S3C2410_NFCONF_TWRPH0(twrph0 - 1);
                set |= S3C2410_NFCONF_TWRPH1(twrph1 - 1);


        cfg = readl(info->regs + S3C2410_NFCONF);
        cfg &= ~mask;
        cfg |= set;
        writel(cfg, info->regs + S3C2410_NFCONF);

And Murali said:

> >Huh? That does not explain why apparently bc->horz.win_h_sz_calc can be
> >larger
> because the values come from the user and since we can't use the enum
> for the types, I have to make sure the value is within range. Other way
> to do is to check the value in the validate() function. I am inclined to
> do the validation so that the & statements with masks can be removed while 
> setting it in
> the register.

Agree fully here. Either a separate validate function or
an if check before using the values. Results with masking
or without masking are both unpredictable.


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