Re: [PATCH v12 3/6] fbmon: add videomode helpers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thursday 22 November 2012 09:53:42 Sascha Hauer wrote:
> On Thu, Nov 22, 2012 at 09:50:10AM +0100, Laurent Pinchart wrote:
> > On Thursday 22 November 2012 07:20:00 Sascha Hauer wrote:
> > > On Wed, Nov 21, 2012 at 11:02:44PM +0100, Laurent Pinchart wrote:
> > > > Hi Steffen,
> > > > 
> > > > > +
> > > > > +	htotal = vm->hactive + vm->hfront_porch + vm->hback_porch +
> > > > > +		 vm->hsync_len;
> > > > > +	vtotal = vm->vactive + vm->vfront_porch + vm->vback_porch +
> > > > > +		 vm->vsync_len;
> > > > > +	fbmode->refresh = (vm->pixelclock * 1000) / (htotal * vtotal);
> > > > 
> > > > This will fail if vm->pixelclock >= ((1 << 32) / 1000). The easiest
> > > > solution is probably to use 64-bit computation.
> > > 
> > > You have displays with a pixelclock > 4GHz?
> > 
> > vm->pixelclock is expressed in Hz. vm->pixelclock * 1000 will thus
> > overflow if the clock frequency is >= ~4.3 MHz. I have displays with a
> > clock frequency higher than that :-)
> 
> If vm->pixelclock is in Hz, then the * 1000 above is wrong.

My bad, I though refresh was expressed in mHz. So yes, the above computation 
is wrong.

BTW it seems that the refreshrate field in struct videomode isn't used. It 
should then be removed.

I've just realized that the struct videomode fields are not documented. 
kerneldoc in include/linux/videomode.h would be a good addition.

-- 
Regards,

Laurent Pinchart