Re: [Linux-fbdev-devel] [PATCH 7/9] viafb: VIA Frame Buffer Device Driver

[Ondrej Zajicek]

On Wed, May 07, 2008 at 07:09:44PM +0800, JosephChan@xxxxxxxxxx wrote:
> +static int viafb_ioctl(struct fb_info *info, u_int cmd, u_long arg)
> +{
> +	switch (cmd) {
> +	case VIAFB_GET_CHIP_INFO:	/*struct chip_information chip_info ; */
> +	if (copy_to_user((void __user *)arg, &chip_info, sizeof(chip_info)))
> +		return -EFAULT;
> +		break;
> +	case VIAFB_GET_INFO_SIZE:
> +		return put_user(sizeof(viainfo), argp);
> +	case VIAFB_GET_INFO:
> +		return ioctl_get_viafb_info(arg);
...
> +	case VIAFB_GET_RESOLUTION:

Some of these ioctls looks like they duplicate standard fbdev ioctls.
What is a reason for that?

> +int get_mode_index(int hres, int vres, int flag)
> +{
> +	DEBUG_MSG(KERN_INFO "get_mode_index!\n");
> +
> +	if (flag == 0) {
> +		if (hres == 480 && vres == 640) {
> +			resMode = VIA_RES_480X640;
> +			mode = "480x640";
> +		} else if (hres == 640 && vres == 480) {
> +			resMode = VIA_RES_640X480;
> +			mode = "640x480";
> +		} else if (hres == 800 && vres == 480) {
> +			resMode = VIA_RES_800X480;
> +			mode = "800x480";
> +		} else if (hres == 800 && vres == 600) {
> +			resMode = VIA_RES_800X600;
> +			mode = "800x600";
> +		} else if (hres == 1024 && vres == 768) {
> +			resMode = VIA_RES_1024X768;
> +			mode = "1024x768";
...

This is particularly ugly function. Some loop through a table is perhaps
better than 250 lines of such if statements, isn't it?

> +		if (!strncmp(this_opt, "mode=", 5)) {
> +			mode = kmalloc(strlen(this_opt + 4), GFP_KERNEL);
> +			strcpy(mode, this_opt + 5);
> +		} else if (!strncmp(this_opt, "mode1=", 6)) {
> +			mode1 = kmalloc(strlen(this_opt + 5), GFP_KERNEL);
> +			strcpy(mode1, this_opt + 6);
> +		} else if (!strncmp(this_opt, "bpp=", 4)) {
> +			via_fb_bpp = simple_strtoul(this_opt + 4, NULL, 0);
> +		} else if (!strncmp(this_opt, "bpp1=", 5)) {
> +			via_fb_bpp1 = simple_strtoul(this_opt + 5, NULL, 0);
> +		} else if (!strncmp(this_opt, "refresh=", 8)) {
> +			refresh = simple_strtoul(this_opt + 8, NULL, 0);
> +		} else if (!strncmp(this_opt, "refresh1=", 9)) {
> +			refresh1 = simple_strtoul(this_opt + 9, NULL, 0);

Most drivers accept default mode specification like 1024x768-16@60
(see Documentation/fb/modedb.txt). Is there a reason to have separate
bpp= and refresh= ?

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@xxxxxxxxxxxxx)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/