Re: [PATCH v2] OMAPDSS: HDMI: Add M2 divider while calculating clkout

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

[-- Attachment #1: Type: text/plain, Size: 3012 bytes --]

On Wed, 2012-02-15 at 14:55 +0530, K, Mythri P wrote:
> Hi,
> 
> On Wed, Feb 15, 2012 at 1:11 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Wed, 2012-02-15 at 11:20 +0530, mythripk@ti.com wrote:
> >> From: Mythri P K <mythripk@ti.com>
> >>
> >> Add M2 divider in the equation to calculate regm and regmf.
> >> Formula for calculating:
> >> Output clock on digital core domain:
> >>       CLKOUT = (M / (N+1))*CLKINP*(1/M2)
> >> Internal oscillator output clock on internal LDO domain:
> >>       CLKDCOLDO = (M / (N+1))*CLKINP
> >> The current code when allows variable M2 values as input
> >> ignores using M2 divider values in calculation of regm and regmf.
> >> so fix it by using M2 in calculation although the default value for
> >> M2 is 1.
> >>
> >> Signed-off-by: Mythri P K <mythripk@ti.com>
> >> ---
> >>  drivers/video/omap2/dss/hdmi.c |   16 ++++++++--------
> >>  1 files changed, 8 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> >> index 92a6679..9185630 100644
> >> --- a/drivers/video/omap2/dss/hdmi.c
> >> +++ b/drivers/video/omap2/dss/hdmi.c
> >> @@ -256,24 +256,24 @@ static void hdmi_compute_pll(struct omap_dss_device *dssdev, int phy,
> >>
> >>       refclk = clkin / pi->regn;
> >>
> >> -     /*
> >> -      * multiplier is pixel_clk/ref_clk
> >> -      * Multiplying by 100 to avoid fractional part removal
> >> -      */
> >> -     pi->regm = (phy * 100 / (refclk)) / 100;
> >> -
> >>       if (dssdev->clocks.hdmi.regm2 == 0)
> >>               pi->regm2 = HDMI_DEFAULT_REGM2;
> >>       else
> >>               pi->regm2 = dssdev->clocks.hdmi.regm2;
> >>
> >>       /*
> >> +      * multiplier is pixel_clk/ref_clk
> >> +      * Multiplying by 100 to avoid fractional part removal
> >> +      */
> >> +     pi->regm = (phy * 100 * pi->regm2 / (refclk)) / 100;
> >
> > No need for parenthesis around refclk.
> Well this is just a copy of old code will change.

The multiplication and division with 100 is actually extra also, they
don't bring any precision here.

> >
> >> +
> >> +     /*
> >>        * fractional multiplier is remainder of the difference between
> >>        * multiplier and actual phy(required pixel clock thus should be
> >>        * multiplied by 2^18(262144) divided by the reference clock
> >>        */
> >> -     mf = (phy - pi->regm * refclk) * 262144;
> >> -     pi->regmf = mf / (refclk);
> >> +     mf = (phy - pi->regm / pi->regm2 * refclk) * 262144;
> >
> > What kind of values does regm have? If the regm2 is something else than
> > 1, and regm is relatively small value, it's quite easy to lose precision
> > there. Would it be better to have:
> Normally regm values are much higher compared to regm2 , regm2 is
> normally 1 or 2.
> also given that regm2 is a multiplier factor in calculating regm there
> should not be any precision factor what do you think ?

Ok, I think it's fine.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]