Re: [PATCH 11/13] viafb: viafbdev.c, viafbdev.h

[Jiri Slaby <jirislaby@gmail.com>]

On 06/30/2008 09:46 AM, JosephChan@via.com.tw wrote:
> Initialization, remove and some other important functions of viafb.
> 
> Signed-off-by: Joseph Chan <josephchan@via.com.tw>
> 
> diff -Nur a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
> --- a/drivers/video/via/viafbdev.c	1970-01-01 08:00:00.000000000 +0800
> +++ b/drivers/video/via/viafbdev.c	2008-06-30 08:53:33.000000000 +0800
> @@ -0,0 +1,2659 @@
[...]
> +#ifdef MODULE
> +#include <linux/module.h>
> +#endif

don't ifdef it

> +#define _MASTER_FILE
[...]
> +static void get_panel_max_scal_size(struct _panel_size_pos_info
> +	*p_max_size)
> +{
> +	switch (p_max_size->device_type) {
> +	case DVI_Device:
> +		p_max_size->x = p_max_size->y = 0;
> +		break;
> +	default:
> +		p_max_size->x = p_max_size->y = 0;
> +	}
> +}
> +static void get_panel_max_scal_pos(struct _panel_size_pos_info *p_para)
> +{
> +	switch (p_para->device_type) {
> +	case DVI_Device:
> +		p_para->x = p_para->y = 0;
> +		break;
> +	default:
> +		p_para->x = p_para->y = 0;
> +	}
> +}
> +
> +static void get_panel_scal_pos(struct _panel_size_pos_info *p_para)
> +{
> +	switch (p_para->device_type) {
> +	case DVI_Device:
> +		p_para->x = p_para->y = 0;
> +		break;
> +	default:
> +		p_para->x = p_para->y = 0;
> +	}
> +}
> +static void get_panel_scal_size(struct _panel_size_pos_info *p_para)
> +{
> +	switch (p_para->device_type) {
> +	case DVI_Device:
> +		p_para->x = p_para->y = 0;
> +		break;
> +	default:
> +		p_para->x = p_para->y = 0;
> +	}

why all the swicthes?

> +}
> +
> +static void set_panel_scal_pos(struct _panel_size_pos_info *p_para)
> +{
> +	switch (p_para->device_type) {
> +	case DVI_Device:
> +		break;
> +	default:
> +		;
> +	}
> +}
> +
> +static void set_panel_scal_size(struct _panel_size_pos_info *p_para)
> +{
> +	switch (p_para->device_type) {
> +	case DVI_Device:
> +		break;
> +	default:
> +		;
> +	}

Sorry?

> +}
[...]
> +static int viafb_get_cmap_len(struct fb_var_screeninfo *var)

inline candidate?

> +{
> +	DEBUG_MSG(KERN_INFO "viafb_get_cmap_len!\n");
> +
> +	return (var->bits_per_pixel == 8) ? 256 : 16;
> +}
[...]
> +static int viafb_pan_display(struct fb_var_screeninfo *var,
> +	struct fb_info *info)
> +{
> +	unsigned int offset = 0;

do not init it.

> +
> +	DEBUG_MSG(KERN_INFO "viafb_pan_display!\n");
> +
> +	offset = (var->xoffset + (var->yoffset * var->xres)) *
> +	    var->bits_per_pixel / 16;
> +
> +	DEBUG_MSG(KERN_INFO "\nviafb_pan_display,offset =%d ", offset);
> +
> +	viafb_write_reg_mask(0x48, 0x3d4, ((offset >> 24) & 0x3), 0x3);
> +	viafb_write_reg_mask(0x34, 0x3d4, ((offset >> 16) & 0xff), 0xff);
> +	viafb_write_reg_mask(0x0c, 0x3d4, ((offset >> 8) & 0xff), 0xff);
> +	viafb_write_reg_mask(0x0d, 0x3d4, (offset & 0xff), 0xff);
> +
> +	return 0;
> +}
[...]
> +static int viafb_ioctl(struct fb_info *info, u_int cmd, u_long arg)
> +{

I haven't measured this, but how big are these variables?
4*256 + plenty of non-pointers seems like stack-unfriendly code.


> +	struct viafb_ioctl_info viainfo;
> +	struct viafb_ioctl_mode viamode;
> +	struct viafb_ioctl_samm viasamm;
> +	struct viafb_driver_version driver_version;
> +	struct fb_var_screeninfo sec_var;
> +	struct _panel_size_pos_info panel_pos_size_para;
> +	u32 state_info = 0;
> +	u32 viafb_gamma_table[256];
> +	char driver_name[10] = "viafb\0";

the nul-termination is implicit

> +
> +	u32 __user *argp = (u32 __user *) arg;
> +	u32 gpu32 = 0, ss;
> +	u32 video_dev_info = 0;
> +	struct viafb_ioctl_setting viafb_setting;
> +	struct device_t active_dev;

you can use "= { }" instead of memsets

> +	ss = sizeof(active_dev);
> +	memset(&active_dev, 0, ss);
> +	memset(&viafb_setting, 0, sizeof(viafb_setting));
> +
> +	DEBUG_MSG(KERN_INFO "viafb_ioctl: 0x%X !!\n", cmd);
> +

This should be unlocked_ioctl ready, I suppose.

> +	switch (cmd) {
> +	case VIAFB_GET_CHIP_INFO:	/*struct chip_information chip_info ; */
> +	if (copy_to_user((void __user *)arg, viaparinfo->chip_info,

some bad alignment, use argp

> +		sizeof(struct chip_information)))
> +		return -EFAULT;
> +		break;
> +	case VIAFB_GET_INFO_SIZE:
> +		return put_user(sizeof(viainfo), argp);
> +	case VIAFB_GET_INFO:
> +		return viafb_ioctl_get_viafb_info(arg);
> +	case VIAFB_HOTPLUG:
> +		return put_user((u32)

cast unneeded

> +				viafb_ioctl_hotplug(info->var.xres,
> +					      info->var.yres,
> +					      info->var.bits_per_pixel), argp);
> +		break;

break unneeded

> +	case VIAFB_SET_HOTPLUG_FLAG:
> +		if (copy_from_user(&gpu32, argp, sizeof(gpu32)))
> +			return -EFAULT;
> +		via_fb_hotplug = (gpu32) ? 1 : 0;
> +		break;
> +	case VIAFB_GET_RESOLUTION:
> +		viamode.xres = (u32) via_fb_hotplug_Xres;
> +		viamode.yres = (u32) via_fb_hotplug_Yres;
> +		viamode.refresh = (u32) via_fb_hotplug_refresh;
> +		viamode.bpp = (u32) via_fb_hotplug_bpp;
> +		if (viafb_SAMM_ON == 1) {
> +			viamode.xres_sec = viafb_second_xres;
> +			viamode.yres_sec = viafb_second_yres;
> +			viamode.virtual_xres_sec = viafb_second_virtual_xres;
> +			viamode.virtual_yres_sec = viafb_second_virtual_yres;
> +			viamode.refresh_sec = viafb_refresh1;
> +			viamode.bpp_sec = via_fb_bpp1;
> +		} else {
> +			viamode.xres_sec = 0;
> +			viamode.yres_sec = 0;
> +			viamode.virtual_xres_sec = 0;
> +			viamode.virtual_yres_sec = 0;
> +			viamode.refresh_sec = 0;
> +			viamode.bpp_sec = 0;
> +		}
> +		if (copy_to_user((void __user *)arg,

argp

> +			&viamode, sizeof(viamode)))
> +			return -EFAULT;
> +		break;
> +	case VIAFB_GET_SAMM_INFO:
> +		viasamm.samm_status = viafb_SAMM_ON;
> +
> +		if (viafb_SAMM_ON == 1) {
> +			if (viafb_dual_fb) {
> +				viasamm.size_prim = viaparinfo->fbmem_free;
> +				viasamm.size_sec = viaparinfo1->fbmem_free;
> +			} else {
> +				if (viafb_second_size) {
> +					viasamm.size_prim =
> +					    viaparinfo->fbmem_free -
> +					    viafb_second_size * 1024 * 1024;
> +					viasamm.size_sec =
> +					    viafb_second_size * 1024 * 1024;
> +				} else {
> +					viasamm.size_prim =
> +					    viaparinfo->fbmem_free >> 1;
> +					viasamm.size_sec =
> +					    (viaparinfo->fbmem_free >> 1);
> +				}
> +			}
> +			viasamm.mem_base = viaparinfo->fbmem;
> +			viasamm.offset_sec = viafb_second_offset;
> +		} else {
> +			viasamm.size_prim =
> +			    viaparinfo->memsize - viaparinfo->fbmem_used;
> +			viasamm.size_sec = 0;
> +			viasamm.mem_base = viaparinfo->fbmem;
> +			viasamm.offset_sec = 0;
> +		}
> +
> +		if (copy_to_user((void __user *)arg,

...

> +			&viasamm, sizeof(viasamm)))
> +			return -EFAULT;
> +
> +		break;
> +	case VIAFB_TURN_ON_OUTPUT_DEVICE:
> +		if (copy_from_user(&gpu32, argp, sizeof(gpu32)))
> +			return -EFAULT;
> +		if (gpu32 & CRT_Device)
> +			viafb_crt_enable();
> +		if (gpu32 & DVI_Device)
> +			viafb_dvi_enable();
> +		if (gpu32 & LCD_Device)
> +			viafb_lcd_enable();
> +		break;
> +	case VIAFB_TURN_OFF_OUTPUT_DEVICE:
> +		if (copy_from_user(&gpu32, argp, sizeof(gpu32)))
> +			return -EFAULT;
> +		if (gpu32 & CRT_Device)
> +			viafb_crt_disable();
> +		if (gpu32 & DVI_Device)
> +			viafb_dvi_disable();
> +		if (gpu32 & LCD_Device)
> +			viafb_lcd_disable();
> +		break;
> +	case VIAFB_SET_DEVICE:
> +		if (copy_from_user(&active_dev, (void *)argp, ss))

sparse unfriendly cast

> +			return -EFAULT;
> +		viafb_set_device(active_dev);
> +		viafb_set_par(info);
> +		break;
> +	case VIAFB_GET_DEVICE:
> +		active_dev.crt = viafb_CRT_ON;
> +		active_dev.dvi = viafb_DVI_ON;
> +		active_dev.lcd = viafb_LCD_ON;
> +		active_dev.samm = viafb_SAMM_ON;
> +		active_dev.primary_dev = viafb_primary_dev;
> +
> +		active_dev.lcd_dsp_cent = viafb_lcd_dsp_method;
> +		active_dev.lcd_panel_id = viafb_lcd_panel_id;
> +		active_dev.lcd_mode = viafb_lcd_mode;
> +
> +		active_dev.xres = via_fb_hotplug_Xres;
> +		active_dev.yres = via_fb_hotplug_Yres;
> +
> +		active_dev.xres1 = viafb_second_xres;
> +		active_dev.yres1 = viafb_second_yres;
> +
> +		active_dev.bpp = via_fb_bpp;
> +		active_dev.bpp1 = via_fb_bpp1;
> +		active_dev.refresh = viafb_refresh;
> +		active_dev.refresh1 = viafb_refresh1;
> +
> +		active_dev.epia_dvi = viafb_platform_epia_dvi;
> +		active_dev.lcd_dual_edge = viafb_device_lcd_dualedge;
> +		active_dev.bus_width = via_bus_width;
> +
> +		if (copy_to_user((void __user *)arg, &active_dev, ss))

jsut argp

> +			return -EFAULT;
> +		break;
> +
> +	case VIAFB_GET_DRIVER_VERSION:
> +		driver_version.iMajorNum = VERSION_MAJOR;
> +		driver_version.iKernelNum = VERSION_KERNEL;
> +		driver_version.iOSNum = VERSION_OS;
> +		driver_version.iMinorNum = VERSION_MINOR;
> +
> +		if (copy_to_user
> +		    ((void __user *)arg, &driver_version,

... and so on

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
[...]
> +static u8 is_duoview(void)
> +{
> +	if (0 == viafb_SAMM_ON) {
> +		if (viafb_LCD_ON + viafb_LCD2_ON +
> +			viafb_DVI_ON + viafb_CRT_ON == 2)
> +			return TRUE;
> +		return FALSE;
> +	} else {
> +		return FALSE;
> +	}

use already defined true and false.

> +}
[...]
> +static int parse_port(char *opt_str, int *output_interface)
> +{
> +	if (!strncmp(opt_str, "DVP0", 4))
> +		*output_interface = INTERFACE_DVP0;
> +	else if (!strncmp(opt_str, "DVP1", 4))
> +		*output_interface = INTERFACE_DVP1;
> +	else if (!strncmp(opt_str, "DFP_HIGHLOW", 11))
> +		*output_interface = INTERFACE_DFP;
> +	else if (!strncmp(opt_str, "DFP_HIGH", 8))
> +		*output_interface = INTERFACE_DFP_HIGH;
> +	else if (!strncmp(opt_str, "DFP_LOW", 8))

7?

> +		*output_interface = INTERFACE_DFP_LOW;
> +	else
> +		*output_interface = INTERFACE_NONE;
> +	return 0;
> +}
[...]
> +static int __devinit via_pci_probe(void)
> +{
> +
> +	/*unsigned char revision; */
> +	unsigned int default_xres, default_yres;
> +	char *tmpc, *tmpm;
> +	char *tmpc_sec, *tmpm_sec;
> +	int vmode_index;
> +    u32 tmds_length, lvds_length, crt_length, chip_length, viafb_par_length;
> +
> +	DEBUG_MSG(KERN_INFO "VIAFB PCI Probe!!\n");
> +
> +#define BYTES_PER_LONG (BITS_PER_LONG/8)
> +#define PADDING(A) (BYTES_PER_LONG - (sizeof(A) % BYTES_PER_LONG))

Is this ALIGN() reimplementation?

> +	viafb_par_length = sizeof(struct viafb_par) + PADDING(struct viafb_par);
> +	tmds_length = sizeof(struct tmds_setting_information) +
> +		PADDING(struct tmds_setting_information);
> +	lvds_length = sizeof(struct lvds_setting_information) +
> +		PADDING(struct lvds_setting_information);
> +	crt_length = sizeof(struct crt_setting_information) +
> +		PADDING(struct crt_setting_information);
> +	chip_length = sizeof(struct chip_information) +
> +		PADDING(struct chip_information);
> +#undef PADDING
> +#undef BYTES_PER_LONG
[...]
> +static void __devexit via_pci_remove(void)
> +{
> +	DEBUG_MSG(KERN_INFO "via_pci_remove!\n");
> +	fb_dealloc_cmap(&viafbinfo->cmap);
> +	unregister_framebuffer(viafbinfo);
> +	if (viafb_dual_fb)
> +		unregister_framebuffer(viafbinfo1);
> +	iounmap((void *)viaparinfo->fbmem_virt);

I suppose io_virt is unmapped somewhere else?

> +
> +	framebuffer_release(viafbinfo);
> +	if (viafb_dual_fb)
> +		framebuffer_release(viafbinfo1);
> +
> +	viafb_remove_proc(viaparinfo->proc_entry);
> +}
> +
> +int __init viafb_init(void)

static

> +{
> +	DEBUG_MSG(KERN_INFO "viafb_init!\n");

Is this useful?

> +	printk(KERN_INFO
> +       "VIA Graphics Intergration Chipset framebuffer %d.%d initializing\n",
> +	       VERSION_MAJOR, VERSION_MINOR);
> +#ifndef MODULE
> +	char *option = NULL;

mixing decl and code. put this code into { }

> +	if (fb_get_options("viafb", &option))
> +		return -ENODEV;
> +	viafb_setup(option);
> +#endif
> +	return via_pci_probe();
> +}
> +
> +void __exit viafb_exit(void)
> +{
> +	DEBUG_MSG(KERN_INFO "viafb_exit!\n");
> +	if (timer_on) {
> +		del_timer(&timer_for3D);

del_timer_sync. You don't need to test for timer_on, do you?

> +		timer_on = 0;
> +	}
> +	via_pci_remove();
> +}
> +
> +int __init viafb_setup(char *options)

static

> +{
> +	char *this_opt;
> +	DEBUG_MSG(KERN_INFO "viafb_setup!\n");
> +
> +	if (!options || !*options)
> +		return 0;
> +
> +	while ((this_opt = strsep(&options, ",")) != NULL) {
> +		if (!*this_opt)
> +			continue;
> +
> +		if (!strncmp(this_opt, "viafb_mode=", 11)) {
> +			viafb_mode = kmalloc(strlen(this_opt + 5), GFP_KERNEL);
> +			strcpy(viafb_mode, this_opt + 11);

kstrdup + checks for NULL and so further...

> +		} else if (!strncmp(this_opt, "viafb_mode1=", 12)) {
> +			viafb_mode1 = kmalloc(strlen(this_opt + 11),
> +				GFP_KERNEL);
> +			strcpy(viafb_mode1, this_opt + 12);
> +		} else if (!strncmp(this_opt, "via_fb_bpp=", 11))
> +			strict_strtoul(this_opt + 11, 0,
> +				(unsigned long *)&via_fb_bpp);