RE: [RFC v1 3/6] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices
From: Chakravarty, Souvik K
Date: Wed Aug 02 2017 - 23:03:34 EST
> -----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