Re: [PATCH] tpm_infineon: Support for new TPM 1.2 and PNPACPI

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

On Thursday 04 August 2005 4:31 am, Marcel Selhorst wrote:
> This patch includes support for the new Infineon Trusted Platform Module
> SLB 9635 TT 1.2 and does further include ACPI-support for both chip versions
> (SLD 9630 TT 1.1 and SLB9635 TT 1.2). Since the ioports and configuration
> registers are not correctly set on some machines, the configuration is now
> done via PNPACPI, which reads out the correct values out of the DSDT-table.
> Note that you have to have CONFIG_PNP, CONFIG_ACPI_BUS and CONFIG_PNPACPI
> enabled to run this driver (assuming that mainbaords including a TPM do have
> the need for ACPI anyway).

> +++ linux-new/drivers/char/tpm/tpm_infineon.c	2005-08-04 12:19:47.000000000 +0200
> ...
> +#include <acpi/acpi_bus.h>

You shouldn't need acpi_bus.h anymore, since you're using PNP.

> +/* These values will be filled after ACPI-call */

You're not using ACPI anymore.

> +static const struct pnp_device_id tpm_pnp_tbl[] = {
> +	/* Infineon TPMs */
> +	{"IFX0101", 0},
> +	{"IFX0102", 0},
> +	{"", 0}
> +};

I get the impression from other drivers (though I admit it's out
of my area), that in general, you should use MODULE_DEVICE_TABLE()
to export this table.

> +static int __devinit tpm_inf_acpi_probe(struct pnp_dev *dev,
> +					const struct pnp_device_id *dev_id)

Should be tpm_inf_pnp_probe() or similar.

> +	TPM_INF_ADDR = (pnp_port_start(dev, 0) & 0xff);

Thanks for using PNP and getting rid of the usage of
TPM_ADDR.  Now if we could just get the atml and nsc
folks to do the same thing, we could nuke both TPM_ADDR
and TPM_SUPERIO_ADDR.

>  static int __devinit tpm_inf_probe(struct pci_dev *pci_dev,
>  				   const struct pci_device_id *pci_id)
>  {
> @@ -353,64 +384,99 @@ static int __devinit tpm_inf_probe(struc
>  	int vendorid[2];
>  	int version[2];
>  	int productid[2];
> +	char chipname[20];
> 
>  	if (pci_enable_device(pci_dev))
>  		return -EIO;

Usually people just return what pci_enable_device() returned.

>  	dev_info(&pci_dev->dev, "LPC-bus found at 0x%x\n", pci_id->device);
> 
> +	/* read IO-ports from ACPI */
> +	pnp_register_driver(&tpm_inf_pnp);
> +	pnp_unregister_driver(&tpm_inf_pnp);

This is kind of a weird mixture of a PCI device & driver (the
LPC bus?) and a PNP device & driver (the TPM device).  Can there
be things other than the TPM on the LPC bus?  Should the LPC
part be split into a separate driver somehow?

I see "LPC" in the atml and nsc drivers as well -- is it
conceivable that the TPMs and LPCs could be mixed-and-matched
someday?  If so, you'd definitely want to split the LPC and
TPM drivers.

> +	switch ((productid[0] << 8) | productid[1]) {
> +	case 6:
> +		sprintf(chipname, " (SLD 9630 TT 1.1)");
> +		break;
> +	case 11:
> +		sprintf(chipname, " (SLB 9635 TT 1.2)");
> +		break;
> +	default:
> +		sprintf(chipname, " (unknown chip)");
> +		break;
> +	}
> +	chipname[19] = 0;

Just use "snprintf(chipname, sizeof(chipname), ...)", which null-
terminates the string for you.