[LinuxBIOS] [PATCH] v3: Intel 82371EB reworking, QEMU fixing

[Stefan Reinauer]

* Uwe Hermann <uwe at hermann-uwe.de> [070607 20:35]:
> Also, I tried to split up the ACPI/SMBus code into two functions,
> even though both devices have the same PCI ID. Is this supposed to
> work?

LinuxBIOS will only call one constructor per file.

Can't you call both functions from the same constructor?

> One issue needs to be resolved: Where/how do we set the config options
> from config.h? In the dts? For now they seem to default to 0 (off),
> thus the devices are not initialized (QEMU still boots for some reason).

See Ron's posting from these days.

Qemu needs no init. We do it for the sake of it and for the hope that it
will some time emulate enough of the hw. (Its improving)

> Signed-off-by: Uwe Hermann <uwe at hermann-uwe.de>
>  static void i82371eb_isa_init(struct device *dev)
>  {
>  	rtc_init(0);
> +
> +	/* TODO: Select full ISA (instead of EIO)? */
> +
> +	/* TODO: ISA DMA init? */
>  }
 
DMA init is there why dont you just call it?

> + // TODO: Rename to ide_init() if it's not 82371EB specific?

it is specific. So remove TODO

> +	if (conf->ide0_enable) {
> +		/* Enable/disable UDMA/33 operation (primary IDE interface). */
> +		reg8 = pci_read_config8(dev, UDMACTL);
> +		if (conf->ide0_drive0_udma33_enable) {
> +			reg8 |= PSDE0;
> +			printk(BIOS_INFO, "Primary IDE, drive 0: UDMA/33 on\n");
> +		} else {
> +			reg8 &= ~(PSDE0);
> +			printk(BIOS_INFO, "Primary IDE, drive 0: UDMA/33 off\n");
> +		}
> +		if (conf->ide0_drive1_udma33_enable) {
> +			reg8 |= PSDE1;
> +			printk(BIOS_INFO, "Primary IDE, drive 1: UDMA/33 on\n");
> +		} else {
> +			reg8 &= ~(PSDE1);
> +			printk(BIOS_INFO, "Primary IDE, drive 1: UDMA/33 off\n");
> +		}

Suggestion for code size: If you do 

printk(BIOS_INFO, "%s IDE, drive %d: UDMA/33 %s\n", "Primary", 1, "on");

Without any other magic, the resulting code will be a lot smaller, since
it only saves one main string, plus the words Primary/Secondary and
on/off instead of a full string every time. gcc is great sometimes.

> +	reg16 = pci_read_config16(dev, IDETIM_SEC);

printk(BIOS_INFO, "Secondary IDE interface %s\n",
                        conf->ide1_enable?"enabled":"disabled");

The IDE code is nice, but very wasteful.
  
> +	printk(BIOS_INFO, "SMBus controller enabled.\n");

This should be DEBUG.

> +	printk(BIOS_INFO, "Power management functions enabled\n");

Dito.

Let's make the code more silent when not _debugging_ it. output takes
time. Spent time makes us look bad.

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
      Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: info at coresystems.de  • http://www.coresystems.de/