[LinuxBIOS] [PATCH]Fintek F71805f for LinuxBIOSv3

Peter Stuge peter at stuge.se
Mon Oct 29 04:16:24 CET 2007


On Sun, Oct 28, 2007 at 08:49:04PM -0400, Corey Osgood wrote:
> +void f71805f_pnp_set_resources(struct device *dev);
> +void f71805f_pnp_set_resources(struct device *dev);

Twice?


> +static void pnp_enter_conf_state(struct device *dev);
> +static void pnp_exit_conf_state(struct device *dev);
> +
> +static void pnp_enter_conf_state(struct device *dev) 
> +{
> +	outb(0x87, dev->path.u.pnp.port);
> +}
> +
> +static void pnp_exit_conf_state(struct device *dev) 
> +{
> +	outb(0xaa, dev->path.u.pnp.port);
> +}

Are the declarations really needed when the definition is right after
them?


> +	switch (dev->path.u.pnp.device) {
> +	case F71805F_SP1: 
> +		res0 = find_resource(dev, PNP_IDX_IO0);
> +		//TODO: needed? fix or remove?
> +		//init_uart8250(res0->base, &conf->sp1);
> +		break;
> +		
> +	case F71805F_SP2:
> +		res1 = find_resource(dev, PNP_IDX_IO0);
> +		//init_uart8250(res0->base, &conf->sp2);
> +		break;

PNP_IDX_IO0 in both cases?
Plus the commented SP2 call uses res0.


> +static struct device_operations ops;
> +static struct pnp_info pnp_dev_info[] = {
> +	{ &ops, F71805F_SP1,  PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, },
> +	{ &ops, F71805F_SP2,  PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, },
> +	/* TODO: Everything else */
> +};

You could combine the declaration and definition of ops.


> +static struct device_operations ops = {
> +	.phase2_setup_scan_bus   = phase2_setup_scan_bus,
> +	.phase4_read_resources   = pnp_read_resources,
> +	.phase4_set_resources    = f71805f_pnp_set_resources,
> +	.phase4_enable_disable   = f71805f_pnp_enable_resources,
> +	.phase5_enable_resources = f71805f_pnp_enable,

Seems these two have been mixed up.


> +	.phase6_init             = f71805f_init,

Using ops it's of course very easy to have generic pnp functions that
do what most of the chips want/need. We want that. :)


//Peter




More information about the coreboot mailing list