> -----Original Message----- > From: sathyanarayanan kuppuswamy > [mailto:sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx] > Sent: Thursday, August 3, 2017 3:29 AM > To: Chakravarty, Souvik K <souvik.k.chakravarty@xxxxxxxxx>; > x86@xxxxxxxxxx; mingo@xxxxxxxxxx; Zha, Qipeng > <qipeng.zha@xxxxxxxxx>; hpa@xxxxxxxxx; dvhart@xxxxxxxxxxxxx; > tglx@xxxxxxxxxxxxx; andy@xxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; > sathyaosid@xxxxxxxxx > Subject: Re: [RFC v1 3/6] platform/x86: intel_pmc_ipc: Use MFD framework > to create dependent devices > > > > On 08/01/2017 10:14 PM, Chakravarty, Souvik K wrote: > > > >> -----Original Message----- > >> From: platform-driver-x86-owner@xxxxxxxxxxxxxxx > >> [mailto:platform-driver- x86-owner@xxxxxxxxxxxxxxx] On Behalf Of > >> sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx > >> Sent: Tuesday, August 1, 2017 11:44 PM > >> To: x86@xxxxxxxxxx; mingo@xxxxxxxxxx; Zha, Qipeng > >> <qipeng.zha@xxxxxxxxx>; hpa@xxxxxxxxx; dvhart@xxxxxxxxxxxxx; > >> tglx@xxxxxxxxxxxxx; andy@xxxxxxxxxxxxx > >> Cc: linux-kernel@xxxxxxxxxxxxxxx; > >> platform-driver-x86@xxxxxxxxxxxxxxx; > >> sathyaosid@xxxxxxxxx; Kuppuswamy Sathyanarayanan > >> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> > >> Subject: [RFC v1 3/6] platform/x86: intel_pmc_ipc: Use MFD framework > >> to create dependent devices > >> > >> From: Kuppuswamy Sathyanarayanan > >> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> > >> > >> Currently, we have lot of repetitive code in dependent device > >> resource allocation and device creation handling code. This logic can > >> be improved if we use MFD framework for dependent device creation. > >> This patch adds this support. > >> > >> Signed-off-by: Kuppuswamy Sathyanarayanan > >> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> > >> --- > >> drivers/platform/x86/intel_pmc_ipc.c | 376 > >> +++++++++++++------------------- > >> --- > >> 1 file changed, 141 insertions(+), 235 deletions(-) > >> > >> diff --git a/drivers/platform/x86/intel_pmc_ipc.c > >> b/drivers/platform/x86/intel_pmc_ipc.c > >> index ad0416e..43533ec 100644 > >> --- a/drivers/platform/x86/intel_pmc_ipc.c > >> +++ b/drivers/platform/x86/intel_pmc_ipc.c > >> @@ -33,6 +33,7 @@ > >> #include <linux/suspend.h> > >> #include <linux/acpi.h> > >> #include <linux/io-64-nonatomic-lo-hi.h> > >> +#include <linux/mfd/core.h> > >> > >> #include <asm/intel_pmc_ipc.h> > >> > >> @@ -105,8 +106,6 @@ > >> #define TELEM_SSRAM_SIZE 240 > >> #define TELEM_PMC_SSRAM_OFFSET 0x1B00 > >> #define TELEM_PUNIT_SSRAM_OFFSET 0x1A00 > >> -#define TCO_PMC_OFFSET 0x8 > >> -#define TCO_PMC_SIZE 0x4 > >> > >> /* PMC register bit definitions */ > >> > >> @@ -115,6 +114,13 @@ > >> #define PMC_CFG_NO_REBOOT_EN (1 << 4) > >> #define PMC_CFG_NO_REBOOT_DIS (0 << 4) > >> > >> +enum { > >> + PMC_IPC_PUNIT_MFD_BLOCK, > >> + PMC_IPC_WATCHDOG_MFD_BLOCK, > >> + PMC_IPC_TELEMETRY_MFD_BLOCK, > >> + PMC_IPC_MAX_MFD_BLOCK > >> +}; > >> + > >> static struct intel_pmc_ipc_dev { > >> struct device *dev; > >> void __iomem *ipc_base; > >> @@ -123,25 +129,12 @@ static struct intel_pmc_ipc_dev { > >> int cmd; > >> struct completion cmd_complete; > >> > >> - /* The following PMC BARs share the same ACPI device with the IPC > >> */ > >> - resource_size_t acpi_io_base; > >> - int acpi_io_size; > >> - struct platform_device *tco_dev; > >> - > >> /* gcr */ > >> void __iomem *gcr_mem_base; > >> bool has_gcr_regs; > >> > >> - /* punit */ > >> - struct platform_device *punit_dev; > >> - > >> /* Telemetry */ > >> - resource_size_t telem_pmc_ssram_base; > >> - resource_size_t telem_punit_ssram_base; > >> - int telem_pmc_ssram_size; > >> - int telem_punit_ssram_size; > >> u8 telem_res_inval; > >> - struct platform_device *telemetry_dev; > >> } ipcdev; > >> > >> static char *ipc_err_sources[] = { > >> @@ -613,7 +606,7 @@ static const struct attribute_group > >> intel_ipc_group = { > >> .attrs = intel_ipc_attrs, > >> }; > >> > >> -static struct resource punit_res_array[] = { > >> +static struct resource punit_ipc_resources[] = { > >> /* Punit BIOS */ > >> { > >> .flags = IORESOURCE_MEM, > >> @@ -637,10 +630,7 @@ static struct resource punit_res_array[] = { > >> }, > >> }; > >> > >> -#define TCO_RESOURCE_ACPI_IO 0 > >> -#define TCO_RESOURCE_SMI_EN_IO 1 > >> -#define TCO_RESOURCE_GCR_MEM 2 > >> -static struct resource tco_res[] = { > >> +static struct resource watchdog_ipc_resources[] = { > >> /* ACPI - TCO */ > >> { > >> .flags = IORESOURCE_IO, > >> @@ -658,9 +648,7 @@ static struct itco_wdt_platform_data tco_info = > { > >> .update_no_reboot_bit = update_no_reboot_bit, }; > >> > >> -#define TELEMETRY_RESOURCE_PUNIT_SSRAM 0 > >> -#define TELEMETRY_RESOURCE_PMC_SSRAM 1 > >> -static struct resource telemetry_res[] = { > >> +static struct resource telemetry_ipc_resources[] = { > >> /*Telemetry*/ > >> { > >> .flags = IORESOURCE_MEM, > >> @@ -670,224 +658,152 @@ static struct resource telemetry_res[] = { > >> }, > >> }; > >> > >> -static int ipc_create_punit_device(void) -{ > >> - struct platform_device *pdev; > >> - const struct platform_device_info pdevinfo = { > >> - .parent = ipcdev.dev, > >> - .name = PUNIT_DEVICE_NAME, > >> - .id = -1, > >> - .res = punit_res_array, > >> - .num_res = ARRAY_SIZE(punit_res_array), > >> - }; > >> - > >> - pdev = platform_device_register_full(&pdevinfo); > >> - if (IS_ERR(pdev)) > >> - return PTR_ERR(pdev); > >> - > >> - ipcdev.punit_dev = pdev; > >> - > >> - return 0; > >> -} > >> - > >> -static int ipc_create_tco_device(void) -{ > >> - struct platform_device *pdev; > >> - struct resource *res; > >> - const struct platform_device_info pdevinfo = { > >> - .parent = ipcdev.dev, > >> - .name = TCO_DEVICE_NAME, > >> - .id = -1, > >> - .res = tco_res, > >> - .num_res = ARRAY_SIZE(tco_res), > >> - .data = &tco_info, > >> - .size_data = sizeof(tco_info), > >> - }; > >> - > >> - res = tco_res + TCO_RESOURCE_ACPI_IO; > >> - res->start = ipcdev.acpi_io_base + TCO_BASE_OFFSET; > >> - res->end = res->start + TCO_REGS_SIZE - 1; > >> - > >> - res = tco_res + TCO_RESOURCE_SMI_EN_IO; > >> - res->start = ipcdev.acpi_io_base + SMI_EN_OFFSET; > >> - res->end = res->start + SMI_EN_SIZE - 1; > >> - > >> - pdev = platform_device_register_full(&pdevinfo); > >> - if (IS_ERR(pdev)) > >> - return PTR_ERR(pdev); > >> - > >> - ipcdev.tco_dev = pdev; > >> - > >> - return 0; > >> -} > >> - > >> -static int ipc_create_telemetry_device(void) -{ > >> - struct platform_device *pdev; > >> - struct resource *res; > >> - const struct platform_device_info pdevinfo = { > >> - .parent = ipcdev.dev, > >> - .name = TELEMETRY_DEVICE_NAME, > >> - .id = -1, > >> - .res = telemetry_res, > >> - .num_res = ARRAY_SIZE(telemetry_res), > >> - }; > >> - > >> - res = telemetry_res + TELEMETRY_RESOURCE_PUNIT_SSRAM; > >> - res->start = ipcdev.telem_punit_ssram_base; > >> - res->end = res->start + ipcdev.telem_punit_ssram_size - 1; > >> - > >> - res = telemetry_res + TELEMETRY_RESOURCE_PMC_SSRAM; > >> - res->start = ipcdev.telem_pmc_ssram_base; > >> - res->end = res->start + ipcdev.telem_pmc_ssram_size - 1; > >> - > >> - pdev = platform_device_register_full(&pdevinfo); > >> - if (IS_ERR(pdev)) > >> - return PTR_ERR(pdev); > >> - > >> - ipcdev.telemetry_dev = pdev; > >> - > >> - return 0; > >> -} > >> - > >> -static int ipc_create_pmc_devices(void) > >> +static int ipc_create_pmc_devices(struct platform_device *pdev) > >> { > >> - int ret; > >> - > >> - /* If we have ACPI based watchdog use that instead */ > >> + u8 n = 0; > >> + struct mfd_cell *pmc_mfd_cells; > >> + > >> + pmc_mfd_cells = devm_kzalloc(&pdev->dev, > >> + (sizeof(*pmc_mfd_cells) * > >> PMC_IPC_MAX_MFD_BLOCK), > >> + GFP_KERNEL); > >> + if (!pmc_mfd_cells) > >> + return -ENOMEM; > >> + > >> + /* Create PUNIT IPC MFD cell */ > >> + pmc_mfd_cells[n].name = PUNIT_DEVICE_NAME; > >> + pmc_mfd_cells[n].id = -1; > >> + pmc_mfd_cells[n].num_resources = > >> ARRAY_SIZE(punit_ipc_resources); > >> + pmc_mfd_cells[n].resources = punit_ipc_resources; > >> + pmc_mfd_cells[n].ignore_resource_conflicts = 1; > >> + n++; > > Since it's not really a loop, cant the enum values of MFD block be used > instead of 'n'? Will be more readable/intuitive. > In this case, watchdog and telemetry device creation is optional and hence > we can't use fixed index when creating mfd-cells. If one of these devices is > not available then the number of valid-mfd-cells and the index number of > individual device cell in pmc_mfd_cells array will change. Agreed. I read in a hurry. > > > >> + > >> + /* If we have ACPI based watchdog use that instead, othewise > >> create > >> + * a MFD cell for iTCO watchdog*/ > >> if (!acpi_has_watchdog()) { > >> - ret = ipc_create_tco_device(); > >> - if (ret) { > >> - dev_err(ipcdev.dev, "Failed to add tco platform > >> device\n"); > >> - return ret; > >> - } > >> - } > >> - > >> - ret = ipc_create_punit_device(); > >> - if (ret) { > >> - dev_err(ipcdev.dev, "Failed to add punit platform > >> device\n"); > >> - platform_device_unregister(ipcdev.tco_dev); > >> + pmc_mfd_cells[n].name = TCO_DEVICE_NAME; > >> + pmc_mfd_cells[n].id = -1; > >> + pmc_mfd_cells[n].platform_data = &tco_info; > >> + pmc_mfd_cells[n].pdata_size = sizeof(tco_info); > >> + pmc_mfd_cells[n].num_resources = > >> + ARRAY_SIZE(watchdog_ipc_resources); > >> + pmc_mfd_cells[n].resources = watchdog_ipc_resources; > >> + pmc_mfd_cells[n].ignore_resource_conflicts = 1; > >> + n++; > > Same as above. > Same as above. > > > >> } > >> > >> if (!ipcdev.telem_res_inval) { > >> - ret = ipc_create_telemetry_device(); > >> - if (ret) > >> - dev_warn(ipcdev.dev, > >> - "Failed to add telemetry platform > >> device\n"); > >> + pmc_mfd_cells[n].name = TELEMETRY_DEVICE_NAME; > >> + pmc_mfd_cells[n].id = -1; > >> + pmc_mfd_cells[n].num_resources = > >> + ARRAY_SIZE(telemetry_ipc_resources); > >> + pmc_mfd_cells[n].resources = telemetry_ipc_resources; > >> + pmc_mfd_cells[n].ignore_resource_conflicts = 1; > >> + n++; > > Same as above. > Same as above. > >> } > >> > >> - return ret; > >> + return devm_mfd_add_devices(&pdev->dev, -1, pmc_mfd_cells, n, > >> NULL, > >> + 0, NULL); > >> } > >> > >> static int ipc_plat_get_res(struct platform_device *pdev) { > >> - struct resource *res, *punit_res; > >> + struct resource *res; > >> void __iomem *addr; > >> - int size; > >> + int mindex, pindex = 0; > >> > >> + /* Get iTCO watchdog resources */ > >> res = platform_get_resource(pdev, IORESOURCE_IO, > >> PLAT_RESOURCE_ACPI_IO_INDEX); > >> if (!res) { > >> dev_err(&pdev->dev, "Failed to get io resource\n"); > >> return -ENXIO; > >> } > >> - size = resource_size(res); > >> - ipcdev.acpi_io_base = res->start; > >> - ipcdev.acpi_io_size = size; > >> - dev_info(&pdev->dev, "io res: %pR\n", res); > >> - > >> - punit_res = punit_res_array; > >> - /* This is index 0 to cover BIOS data register */ > >> - res = platform_get_resource(pdev, IORESOURCE_MEM, > >> - PLAT_RESOURCE_BIOS_DATA_INDEX); > >> - if (!res) { > >> - dev_err(&pdev->dev, "Failed to get res of punit BIOS > >> data\n"); > >> - return -ENXIO; > >> - } > >> - *punit_res = *res; > >> - dev_info(&pdev->dev, "punit BIOS data res: %pR\n", res); > >> - > >> - /* This is index 1 to cover BIOS interface register */ > >> - res = platform_get_resource(pdev, IORESOURCE_MEM, > >> - PLAT_RESOURCE_BIOS_IFACE_INDEX); > >> - if (!res) { > >> - dev_err(&pdev->dev, "Failed to get res of punit BIOS > >> iface\n"); > >> - return -ENXIO; > >> - } > >> - *++punit_res = *res; > >> - dev_info(&pdev->dev, "punit BIOS interface res: %pR\n", res); > >> - > >> - /* This is index 2 to cover ISP data register, optional */ > >> - res = platform_get_resource(pdev, IORESOURCE_MEM, > >> - PLAT_RESOURCE_ISP_DATA_INDEX); > >> - ++punit_res; > >> - if (res) { > >> - *punit_res = *res; > >> - dev_info(&pdev->dev, "punit ISP data res: %pR\n", res); > >> - } > >> > >> - /* This is index 3 to cover ISP interface register, optional */ > >> - res = platform_get_resource(pdev, IORESOURCE_MEM, > >> - PLAT_RESOURCE_ISP_IFACE_INDEX); > >> - ++punit_res; > >> - if (res) { > >> - *punit_res = *res; > >> - dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res); > >> - } > >> - > >> - /* This is index 4 to cover GTD data register, optional */ > >> - res = platform_get_resource(pdev, IORESOURCE_MEM, > >> - PLAT_RESOURCE_GTD_DATA_INDEX); > >> - ++punit_res; > >> - if (res) { > >> - *punit_res = *res; > >> - dev_info(&pdev->dev, "punit GTD data res: %pR\n", res); > >> - } > >> - > >> - /* This is index 5 to cover GTD interface register, optional */ > >> - res = platform_get_resource(pdev, IORESOURCE_MEM, > >> - PLAT_RESOURCE_GTD_IFACE_INDEX); > >> - ++punit_res; > >> - if (res) { > >> - *punit_res = *res; > >> - dev_info(&pdev->dev, "punit GTD interface res: %pR\n", > >> res); > >> - } > >> - > >> - res = platform_get_resource(pdev, IORESOURCE_MEM, > >> - PLAT_RESOURCE_IPC_INDEX); > >> - if (!res) { > >> - dev_err(&pdev->dev, "Failed to get ipc resource\n"); > >> - return -ENXIO; > >> - } > >> - res->end = (res->start + PLAT_RESOURCE_IPC_SIZE + > >> - PLAT_RESOURCE_GCR_SIZE - 1); > >> - > >> - addr = devm_ioremap_resource(&pdev->dev, res); > >> - if (IS_ERR(addr)) { > >> - dev_err(&pdev->dev, > >> - "PMC I/O memory remapping failed\n"); > >> - return PTR_ERR(addr); > >> - } > >> - > >> - ipcdev.ipc_base = addr; > >> - > >> - ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET; > >> - dev_info(&pdev->dev, "ipc res: %pR\n", res); > >> - > >> - ipcdev.telem_res_inval = 0; > >> - res = platform_get_resource(pdev, IORESOURCE_MEM, > >> - PLAT_RESOURCE_TELEM_SSRAM_INDEX); > >> - if (!res) { > >> - dev_err(&pdev->dev, "Failed to get telemetry ssram > >> resource\n"); > >> - ipcdev.telem_res_inval = 1; > >> - } else { > >> - ipcdev.telem_punit_ssram_base = res->start + > >> - > >> TELEM_PUNIT_SSRAM_OFFSET; > >> - ipcdev.telem_punit_ssram_size = TELEM_SSRAM_SIZE; > >> - ipcdev.telem_pmc_ssram_base = res->start + > >> - > >> TELEM_PMC_SSRAM_OFFSET; > >> - ipcdev.telem_pmc_ssram_size = TELEM_SSRAM_SIZE; > >> - dev_info(&pdev->dev, "telemetry ssram res: %pR\n", res); > >> + watchdog_ipc_resources[0].start = res->start + TCO_BASE_OFFSET; > >> + watchdog_ipc_resources[0].end = res->start + > >> + TCO_BASE_OFFSET + TCO_REGS_SIZE - 1; > >> + watchdog_ipc_resources[1].start = res->start + SMI_EN_OFFSET; > >> + watchdog_ipc_resources[1].end = res->start + > >> + SMI_EN_OFFSET + SMI_EN_SIZE - 1; > > The 0/1 magic may be replaced with MACROS here and elsewhere. > I think I tried it first and then later removed it because the length of few > lines go over and 80. > > > >> + > >> + dev_info(&pdev->dev, "watchdog res 0: %pR\n", > >> + &watchdog_ipc_resources[0]); > >> + dev_info(&pdev->dev, "watchdog res 1: %pR\n", > >> + &watchdog_ipc_resources[1]); > >> + > >> + for (mindex = 0; mindex <= PLAT_RESOURCE_GTD_IFACE_INDEX; > >> mindex++) { > > Here we are relying on the fact that the GTD_IFACE_INDEX is at offset 7. > It's however not by design but a side-effect of how the BIOS places the > resources. > > So perhaps not a good idea to use it, since GTD_IFACE is not meant to be > a terminator/max delimiter. > We can add a new macro like "#define PLAT_RESOURCE_MAX_INDEX 8" > and use it in this loop. > > But, In future, If BIOS changes its resource order or removes certain > resources then it needs corresponding changes to this driver. And when > they fix that issue, they have to make sure the MAX_INDEX value is > appropriately modified. > > So even using separate macro doesn't completely isolates it from BIOS > resource changes. Well you are right as far as changing the MACRO value is. But using a MAX_INDEX is intuitive and changing the MAX_INDEX value is also intuitive. OTOH if you maintain the GTD_IFACE, you have to change the loop delimiter to a different IFACE's (e.g. ISP_DATA) every time the BIOS changes and this seems to be a bit strange, esp. when only the index max value has changed. Isn't the above case exactly not what a MAX macro is designed to do? > > > >> + > >> + res = platform_get_resource(pdev, IORESOURCE_MEM, > >> mindex); > >> + > >> + switch (mindex) { > >> + /* Get PUNIT resources */ > >> + case PLAT_RESOURCE_BIOS_DATA_INDEX: > >> + case PLAT_RESOURCE_BIOS_IFACE_INDEX: > >> + /* BIOS resources are required, so return error if not > >> + * available */ > >> + if (!res) { > >> + dev_err(&pdev->dev, > >> + "Failed to get punit mem resource > >> %d\n", > >> + pindex); > >> + return -ENXIO; > >> + } > >> + case PLAT_RESOURCE_ISP_DATA_INDEX: > >> + case PLAT_RESOURCE_ISP_IFACE_INDEX: > >> + case PLAT_RESOURCE_GTD_DATA_INDEX: > >> + case PLAT_RESOURCE_GTD_IFACE_INDEX: > >> + /* if not valid resource, skip the rest of steps */ > >> + if (!res) { > >> + pindex++; > >> + continue; > >> + } > >> + memcpy(&punit_ipc_resources[pindex], res, > >> + sizeof(*res)); > >> + dev_info(&pdev->dev, "PUNIT memory res: %pR\n", > >> + &punit_ipc_resources[pindex]); > >> + pindex++; > >> + break; > >> + /* Get Telemetry resources */ > >> + case PLAT_RESOURCE_TELEM_SSRAM_INDEX: > >> + if (!res) { > >> + dev_warn(&pdev->dev, > >> + "Failed to get telemtry sram res\n"); > >> + ipcdev.telem_res_inval = 1; > >> + continue; > >> + } > >> + telemetry_ipc_resources[0].start = res->start + > >> + TELEM_PUNIT_SSRAM_OFFSET; > >> + telemetry_ipc_resources[0].end = res->start + > >> + TELEM_PUNIT_SSRAM_OFFSET + > >> TELEM_SSRAM_SIZE - 1; > >> + telemetry_ipc_resources[1].start = res->start + > >> + TELEM_PMC_SSRAM_OFFSET; > >> + telemetry_ipc_resources[1].end = res->start + > >> + TELEM_PMC_SSRAM_OFFSET + > >> TELEM_SSRAM_SIZE - 1; > >> + > >> + dev_info(&pdev->dev, "telemetry punit ssram res: > >> %pR\n", > >> + &telemetry_ipc_resources[0]); > >> + dev_info(&pdev->dev, "telemetry pmc ssram res: > >> %pR\n", > >> + &telemetry_ipc_resources[1]); > >> + break; > >> + /* Get IPC resources */ > >> + case PLAT_RESOURCE_IPC_INDEX: > >> + if (!res) { > >> + dev_err(&pdev->dev, > >> + "Failed to get IPC resources\n"); > >> + return -ENXIO; > >> + } > >> + res->end = (res->start + PLAT_RESOURCE_IPC_SIZE + > >> + PLAT_RESOURCE_GCR_SIZE - 1); > >> + addr = devm_ioremap_resource(&pdev->dev, res); > >> + if (IS_ERR(addr)) { > >> + dev_err(&pdev->dev, > >> + "PMC I/O memory remapping > >> failed\n"); > >> + return PTR_ERR(addr); > >> + } > >> + ipcdev.ipc_base = addr; > >> + ipcdev.gcr_mem_base = addr + > >> PLAT_RESOURCE_GCR_OFFSET; > >> + dev_info(&pdev->dev, "PMC IPC resource %pR\n", > >> res); > >> + break; > >> + }; > >> } > >> > >> return 0; > >> @@ -943,7 +859,7 @@ static int ipc_plat_probe(struct platform_device > >> *pdev) > >> return ret; > >> } > >> > >> - ret = ipc_create_pmc_devices(); > >> + ret = ipc_create_pmc_devices(pdev); > >> if (ret) { > >> dev_err(&pdev->dev, "Failed to create pmc devices\n"); > >> return ret; > >> @@ -952,36 +868,26 @@ static int ipc_plat_probe(struct > >> platform_device > >> *pdev) > >> if (devm_request_irq(&pdev->dev, ipcdev.irq, ioc, > IRQF_NO_SUSPEND, > >> "intel_pmc_ipc", &ipcdev)) { > >> dev_err(&pdev->dev, "Failed to request irq\n"); > >> - ret = -EBUSY; > >> - goto unregister_devices; > >> + return -EBUSY; > >> } > >> > >> ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group); > >> if (ret) { > >> dev_err(&pdev->dev, "Failed to create sysfs group %d\n", > >> ret); > >> - goto unregister_devices; > >> + return ret; > >> } > >> > >> ipcdev.has_gcr_regs = true; > >> > >> return 0; > >> - > >> -unregister_devices: > >> - platform_device_unregister(ipcdev.tco_dev); > >> - platform_device_unregister(ipcdev.punit_dev); > >> - platform_device_unregister(ipcdev.telemetry_dev); > >> - > >> - return ret; > >> } > >> > >> static int ipc_plat_remove(struct platform_device *pdev) { > >> sysfs_remove_group(&pdev->dev.kobj, &intel_ipc_group); > >> - platform_device_unregister(ipcdev.tco_dev); > >> - platform_device_unregister(ipcdev.punit_dev); > >> - platform_device_unregister(ipcdev.telemetry_dev); > >> ipcdev.dev = NULL; > >> + > >> return 0; > >> } > >> > >> -- > >> 2.7.4 > > > > -- > Sathyanarayanan Kuppuswamy > Linux kernel developer
