Hi Tomi, On Mon, Feb 28, 2011 at 11:57 AM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote: > On Mon, 2011-02-28 at 00:11 -0600, K, Mythri P wrote: >> Hi Tomi, >> >> On Sun, Feb 27, 2011 at 3:47 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote: > >> >> + * HDMI interface DSS driver setting for TI's OMAP4 family of processor. >> >> + * Copyright (C) 2010-2011 Texas Instruments Incorporated - http://www.ti.com/ >> >> + * Authors: Yong Zhi >> > >> > Who is Yong Zhi and where's his signed-off-by? >> Yong zhi had written some of the functions in this file , which was in >> a library.He no longer works on the driver though . > > Ok. Generally there should be a signed-off-by from everybody who has > written the code. It's like "I have written this code and I have > permission to send it upstream" etc. > > His signed-off would be good for this too, but if the code is just based > on his work, and doesn't really resemble it anymore, perhaps you could > then say "Based on code by Yong Zhi", or similar. > sure i shall check with him to add his sign-off. >> >> + >> >> +static inline int hdmi_wait_for_bit_change(const struct hdmi_reg idx, >> >> + int b2, int b1, u32 val) >> >> +{ >> >> + u32 t = 0; >> >> + while (val != FLD_GET(hdmi_read_reg(idx), b2, b1)) { >> > >> > There's REG_GET macro used in other DSS files to read bits from the >> > registers. >> Hmm i see that in dsi.c REG_GET is defined to do the same, >> #define REG_GET(idx, start, end) \ >> FLD_GET(dsi_read_reg(idx), start, end) >> So should this be fine of should i define a new REG_GET ? > > REG_GET() uses the private dsi_read_reg() function, so it has to be > defined again in each interface file. So for HDMI, define REG_REG > similarly with hdmi_read_reg(). > But it is used in only one function here , so is it really needed :) ? >> >> + /* HACK : DDC needs time to stablize */ >> >> + mdelay(10); >> > >> > So what is the reason for this? It's a sleep that for unknown reason >> > make the code work? Or is there an idea why this is needed? >> This is the time it needs to stabilize DDC, else most of the time it >> reads a 0 or >> wrong shifted values , so i have prefixed with a HACK. > > Ok. I'm fine with the hack for now, but I'm just interested: Is the 10ms > just something that happens to work? This sleep is not discussed in any > documentation? Has this been discussed with HW people? yes i have checked with the silicon validation. > >> >> +static inline void print_omap_video_timings(struct omap_video_timings *timings) >> > >> > Why is this inline? >> Its only a print function , called many times by the driver. Do you >> see any reason not to make it inline? > > Well, generally you shouldn't use inline. I think that's the basic rule. > Compiler usually makes better choices than humans. I have used them for > xxx_read_reg() and xxx_write_reg(), although I'm not quite sure if even > that is needed. > > And in this case the function is quite long. I'm guessing here, but most > likely the code runs faster if it's _not_ inlined, because not inlining > reduces the size of the code. Also, print functions are on the slower > side anyway, so the overhead of calling this function is probably > negligible compared to the contents of the function. > ok i shall change. > Tomi > > > Thanks and regards, Mythri. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
