Re: [PATCH 5/8] OMAP4 : DSS2 : HDMI: HDMI driver addition in the DSS drivers interface

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

On Mon, 2011-02-28 at 00:30 -0600, K, Mythri P wrote:
> Hi Tomi,
> 
> On Mon, Feb 28, 2011 at 11:57 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> 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@ti.com> 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 :) ?

There are more than one FLD_GET(hdmi_read_reg()) patterns in hdmi.c,
which could be replaced with REG_FLD_GET(). But granted, there are still
not many of them now that I checked. Well, up to you if you want to add
that or not.

> >> >> +       /* 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.

Hmm. So, it's a HW feature or a HW bug? In either case, it's not a hack
then. If it's a normal HW feature, you just need to sleep there, simple
as that. If it's a HW bug, then this is a work-around, and it should be
mentioned.

Generally code like this should have a bit more explanation whether it's
a HW bug, HW feature, SW hack that magically makes it work, SW hack made
to get difficult thing done quickly and to be replaced later, etc. The
reader should understand what's going on here, and what can be done for
the code.

Also, I only now noticed that it's a mdelay, not msleep. 10ms mdelay
sounds very bad, don't do that. Use msleep. mdelay is basically a 10ms
busy loop.

 Tomi