On Mon, Nov 30, 2015 at 12:27:12PM -0700, Jason Gunthorpe wrote:
> The TPM core has long assumed that every device has a driver attached,
> however b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are
> called unconditionally") breaks that assumption.Maybe it's worth to point out that b8b2c7d845d5 didn't break it on purpose and is fixed accordingly. Still the assumption isn't valid, but works in practise. > Rework the TPM setup to create a platform device with resources and > then allow the driver core to naturally bind and probe it through the > normal mechanisms. All this structure is needed anyhow to enable TPM > for OF environments. > > Finally, since the entire flow is changing convert the init/exit to use > the modern ifdef-less coding style when possible > > Reported-by: "Wilck, Martin" <[email protected]> > Signed-off-by: Jason Gunthorpe <[email protected]> > --- > drivers/char/tpm/tpm_tis.c | 161 > ++++++++++++++++++++++++++++----------------- > 1 file changed, 101 insertions(+), 60 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index 0a2d94f3d679..0e5c282aa37e 100644 > --- a/drivers/char/tpm/tpm_tis.c > +++ b/drivers/char/tpm/tpm_tis.c > @@ -60,8 +60,6 @@ enum tis_int_flags { > }; > > enum tis_defaults { > - TIS_MEM_BASE = 0xFED40000, > - TIS_MEM_LEN = 0x5000, > TIS_SHORT_TIMEOUT = 750, /* ms */ > TIS_LONG_TIMEOUT = 2000, /* 2 sec */ > }; > @@ -72,12 +70,6 @@ struct tpm_info { > int irq; > }; > > -static struct tpm_info tis_default_info = { > - .start = TIS_MEM_BASE, > - .len = TIS_MEM_LEN, > - .irq = 0, > -}; > - > /* Some timeout values are needed before it is known whether the chip is > * TPM 1.0 or TPM 2.0. > */ > @@ -847,7 +839,6 @@ out_err: > return rc; > } > > -#ifdef CONFIG_PM_SLEEP > static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) > { > u32 intmask; > @@ -889,11 +880,9 @@ static int tpm_tis_resume(struct device *dev) > > return 0; > } > -#endif > > static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume); > > -#ifdef CONFIG_PNP > static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, > const struct pnp_device_id *pnp_id) > { > @@ -908,14 +897,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, > else > tpm_info.irq = -1; > > -#ifdef CONFIG_ACPI > if (pnp_acpi_device(pnp_dev)) { > if (is_itpm(pnp_acpi_device(pnp_dev))) > itpm = true; > > - acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle; > + acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev); > } > -#endif > > return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle); > } > @@ -956,7 +943,6 @@ static struct pnp_driver tis_pnp_driver = { > module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id, > sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444); > MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to > probe"); > -#endif > > #ifdef CONFIG_ACPI > static int tpm_check_resource(struct acpi_resource *ares, void *data) > @@ -1029,80 +1015,135 @@ static struct acpi_driver tis_acpi_driver = { > }; > #endif > > +static struct platform_device *force_pdev; > + > +static int tpm_tis_plat_probe(struct platform_device *pdev) > +{ > + struct tpm_info tpm_info = {}; > + struct resource *res; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (res == NULL) { indention problems here. > + dev_err(&pdev->dev, "no memory resource defined\n"); > + return -ENODEV; > + } > + tpm_info.start = res->start; > + tpm_info.len = resource_size(res); > + > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + if (res) > + tpm_info.irq = res->start; > + else { If one branch of an if/else has braces, all of them should. > + if (pdev == force_pdev) > + tpm_info.irq = -1; > + else > + /* When forcing auto probe the IRQ */ > + tpm_info.irq = 0; > + } ah, so 0 means autoprobe and -1 means invalid. Hmm. > + > + return tpm_tis_init(&pdev->dev, &tpm_info, NULL); > +} > + > +static int tpm_tis_plat_remove(struct platform_device *pdev) > +{ > + struct tpm_chip *chip = dev_get_drvdata(&pdev->dev); > + > + tpm_chip_unregister(chip); > + tpm_tis_remove(chip); > + > + return 0; > +} > + > static struct platform_driver tis_drv = { > + .probe = tpm_tis_plat_probe, > + .remove = tpm_tis_plat_remove, > .driver = { > .name = "tpm_tis", > .pm = &tpm_tis_pm, > }, > }; > > -static struct platform_device *pdev; > - > static bool force; > +#ifdef CONFIG_X86 > module_param(force, bool, 0444); > MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry"); > +#endif Is this added ifdef intended to be in this commit? > +static int force_device(void) > +{ > + struct platform_device *pdev; > + static const struct resource x86_resources[] ={ > + { > + .start = 0xFED40000, > + .end = 0xFED44FFF, > + .flags = IORESOURCE_MEM, > + }, > + }; > + > + if (!force) > + return 0; > + > + /* The driver core will match the name tpm_tis of the device to > + * the tpm_tis platform driver and complete the setup via > + * tpm_tis_plat_probe > + */ > + pdev = platform_device_register_simple("tpm_tis", -1, x86_resources, > + ARRAY_SIZE(x86_resources)); > + if (IS_ERR(pdev)) > + return PTR_ERR(pdev); > + force_pdev = pdev; > + > + return 0; > +} > + > static int __init init_tis(void) > { > int rc; > -#ifdef CONFIG_PNP > - if (!force) { > + > + rc = force_device(); > + if (rc) > + goto out1; > + > + if (IS_ENABLED(CONFIG_PNP)) { > rc = pnp_register_driver(&tis_pnp_driver); > if (rc) > - return rc; > + goto out2; > } > -#endif > + > #ifdef CONFIG_ACPI > - if (!force) { > - rc = acpi_bus_register_driver(&tis_acpi_driver); > - if (rc) { > -#ifdef CONFIG_PNP > - pnp_unregister_driver(&tis_pnp_driver); > -#endif > - return rc; > - } > - } > + rc = acpi_bus_register_driver(&tis_acpi_driver); > + if (rc) > + goto out3; > #endif > - if (!force) > - return 0; > > rc = platform_driver_register(&tis_drv); > - if (rc < 0) > - return rc; > - pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0); > - if (IS_ERR(pdev)) { > - rc = PTR_ERR(pdev); > - goto err_dev; > - } > - rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL); > if (rc) > - goto err_init; > + goto out4; > + > return 0; > -err_init: > - platform_device_unregister(pdev); > -err_dev: > - platform_driver_unregister(&tis_drv); > +out4: > +#ifdef CONFIG_ACPI > + acpi_bus_unregister_driver(&tis_acpi_driver); > +out3: > +#endif > + pnp_unregister_driver(&tis_pnp_driver); > +out2: > + platform_device_unregister(force_pdev); > +out1: Might be a matter of taste, but having nicer names for the error labels makes review easier. For example I would have called "out3" "err_register_acpi" instead. Then you can easily verify that it's placed right in the error path being directly after acpi_bus_unregister_driver. Also all kind of strange things happen if you later need to add a label between out2 and out3. drivers/scsi/hpsa.c for example used "clean2_5" in a similar situation. The alternative is to renumber the label makeing patch review still harder. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
