> Date: Sun, 14 Feb 2010 18:00:52 -0500
> From: Brad <[email protected]>
>
> On Sat, Feb 13, 2010 at 10:02:15PM +0500, Alexandr Shadchin wrote:
> > Hi!
> >
> > add smbus support VIA VT82C596A, VT82C596B and VT82C686A in viapm(4)
> >
> > Tested VIA VT82C596A.
> >
> > --
> > Alexandr Shadchin
>
> Here is a much simpler diff to add support for those chipsets. I also
> noticed the VT8231 chipset was missing from your diff.
>
> Tested by Alexandr as well as with a VT8237 chipset.
Brad, while I like your version a bit better than Alexandr's version,
I have some issues with it:
> Index: viapm.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/viapm.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 viapm.c
> --- viapm.c 26 Jun 2009 02:46:27 -0000 1.12
> +++ viapm.c 14 Feb 2010 22:52:35 -0000
> @@ -40,7 +40,9 @@
> */
>
> /* PCI configuration registers */
> -#define VIAPM_SMB_BASE 0xd0 /* SMBus base address */
> +#define VIAPM_SMB_BASE1 0x90 /* SMBus base address */
> +#define VIAPM_SMB_BASE2 0x80
What is VIAPM_SMB_BASE2 used for?
> +#define VIAPM_SMB_BASE3 0xd0
> #define VIAPM_SMB_HOSTC 0xd2 /* host configuration */
> #define VIAPM_SMB_HOSTC_HSTEN (1 << 0) /* enable host
> controller */
> #define VIAPM_SMB_HOSTC_INTEN (1 << 1) /* enable SCI/SMI */
> @@ -125,6 +127,8 @@ struct cfdriver viapm_cd = {
> };
>
> const struct pci_matchid viapm_ids[] = {
> + { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_CX700_ISA },
> + { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT8231_PWR },
> { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT8233_ISA },
> { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT8233A_ISA },
> { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT8235_ISA },
> @@ -132,7 +136,9 @@ const struct pci_matchid viapm_ids[] = {
> { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT8237A_ISA },
> { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT8237S_ISA },
> { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT8251_ISA },
> - { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_CX700_ISA },
> + { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT82C596 },
> + { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT82C596B_PM },
> + { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT82C686A_SMB },
> { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VX800_ISA },
> { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VX855_ISA }
> };
I don't like just blindly sorting these alphabetically. They were
quite deliberately order in a way such that older chipsets came first.
> @@ -167,7 +185,8 @@ viapm_attach(struct device *parent, stru
> }
>
> /* Read configuration */
> - conf = (iobase >> 16);
> + conf = pci_conf_read(pa->pa_pc, pa->pa_tag, hstcfg);
> + conf >>= 16;
> DPRINTF((": conf 0x%x", conf));
You're doing a config space read that isn't 4-byte aligned. That's verboten.
