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

On Thu, Nov 22, 2012 at 10:07:07AM +0100, Laurent Pinchart wrote:

On Thursday 22 November 2012 09:53:42 Sascha Hauer wrote:

quoted

On Thu, Nov 22, 2012 at 09:50:10AM +0100, Laurent Pinchart wrote:

quoted

On Thursday 22 November 2012 07:20:00 Sascha Hauer wrote:

quoted

On Wed, Nov 21, 2012 at 11:02:44PM +0100, Laurent Pinchart wrote:

quoted

Hi Steffen,

quoted

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

Okay. I will fix that with the next version...

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

...and remove this field.

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

Regards,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |