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. > >> + > >> +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(). > >> + /* 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? > >> +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. Tomi -- 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
