Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754770AbcDJNR1 (ORCPT ); Sun, 10 Apr 2016 09:17:27 -0400 Received: from mail-oi0-f44.google.com ([209.85.218.44]:36542 "EHLO mail-oi0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753598AbcDJNRW convert rfc822-to-8bit (ORCPT ); Sun, 10 Apr 2016 09:17:22 -0400 MIME-Version: 1.0 In-Reply-To: <1459452489-46827-1-git-send-email-aubrey.li@linux.intel.com> References: <1459452489-46827-1-git-send-email-aubrey.li@linux.intel.com> Date: Sun, 10 Apr 2016 16:17:20 +0300 Message-ID: Subject: Re: [PATCH] platform:x86 decouple telemetry driver from the optional IPC resources From: Andy Shevchenko To: Aubrey Li Cc: qipeng.zha@intel.com, "dvhart@infradead.org" , platform-driver-x86@vger.kernel.org, "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8513 Lines: 224 On Thu, Mar 31, 2016 at 10:28 PM, Aubrey Li wrote: > Currently the optional IPC resources prevent telemetry driver from > probing if these resources are not in ACPI table. This patch decouples > telemetry driver from these optional resources, so that telemetry driver > has dependency only on the necessary ACPI resources. Darren, I have comments as well. > > Signed-off-by: Aubrey Li > --- > drivers/platform/x86/intel_pmc_ipc.c | 48 +++++++++++++++----------------- > drivers/platform/x86/intel_punit_ipc.c | 48 +++++++++++++++++++++----------- > 2 files changed, 54 insertions(+), 42 deletions(-) > > diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c > index 092519e..29d9c02 100644 > --- a/drivers/platform/x86/intel_pmc_ipc.c > +++ b/drivers/platform/x86/intel_pmc_ipc.c > @@ -686,8 +686,8 @@ static int ipc_plat_get_res(struct platform_device *pdev) > ipcdev.acpi_io_size = size; > dev_info(&pdev->dev, "io res: %pR\n", res); > > - /* This is index 0 to cover BIOS data register */ > 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) { > @@ -697,55 +697,51 @@ static int ipc_plat_get_res(struct platform_device *pdev) > *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; > } > - /* This is index 1 to cover BIOS interface register */ > *++punit_res = *res; > dev_info(&pdev->dev, "punit BIOS interface res: %pR\n", res); > > + /* This is index 2 to cover ISP data register, optional */ All above looks like a commentary fixes (except an additional 'optional' word in one case). Can you do this separately? > res = platform_get_resource(pdev, IORESOURCE_MEM, > PLAT_RESOURCE_ISP_DATA_INDEX); > - if (!res) { > - dev_err(&pdev->dev, "Failed to get res of punit ISP data\n"); > - return -ENXIO; > + ++punit_res; > + if (res) { > + *punit_res = *res; > + dev_info(&pdev->dev, "punit ISP data res: %pR\n", res); Okay, what if you re-arrange this to some helper first int …_assign_res(*pdev, index, *punit_res) { struct resource res; res = platform_get_resource(pdev, …, index); if (!res) return -ERRNO; *punit_res = *res; dev_dbg(%pR); return 0; } In this patch you move to optional by dev_err -> dev_warn and use if (ret) dev_warn( "…skip optional resource…" ); instead of if (ret) { dev_err(); return ret; } > } > - /* This is index 2 to cover ISP data register */ > - *++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); > - if (!res) { > - dev_err(&pdev->dev, "Failed to get res of punit ISP iface\n"); > - return -ENXIO; > + ++punit_res; > + if (res) { > + *punit_res = *res; > + dev_info(&pdev->dev, "punit ISP interface res: %pR\n", res); > } > - /* This is index 3 to cover ISP interface register */ > - *++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); > - if (!res) { > - dev_err(&pdev->dev, "Failed to get res of punit GTD data\n"); > - return -ENXIO; > + ++punit_res; > + if (res) { > + *punit_res = *res; > + dev_info(&pdev->dev, "punit GTD data res: %pR\n", res); > } > - /* This is index 4 to cover GTD data register */ > - *++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); > - if (!res) { > - dev_err(&pdev->dev, "Failed to get res of punit GTD iface\n"); > - return -ENXIO; > + ++punit_res; > + if (res) { > + *punit_res = *res; > + dev_info(&pdev->dev, "punit GTD interface res: %pR\n", res); > } > - /* This is index 5 to cover GTD interface register */ > - *++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); > diff --git a/drivers/platform/x86/intel_punit_ipc.c b/drivers/platform/x86/intel_punit_ipc.c > index bd87540..a47a41f 100644 > --- a/drivers/platform/x86/intel_punit_ipc.c > +++ b/drivers/platform/x86/intel_punit_ipc.c > @@ -227,6 +227,11 @@ static int intel_punit_get_bars(struct platform_device *pdev) > struct resource *res; > void __iomem *addr; > > + /* > + * The following resources are required > + * - BIOS_IPC BASE_DATA > + * - BIOS_IPC BASE_IFACE > + */ > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > addr = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(addr)) > @@ -239,29 +244,40 @@ static int intel_punit_get_bars(struct platform_device *pdev) > return PTR_ERR(addr); > punit_ipcdev->base[BIOS_IPC][BASE_IFACE] = addr; > > + /* > + * The following resources are optional > + * - ISPDRIVER_IPC BASE_DATA > + * - ISPDRIVER_IPC BASE_IFACE > + * - GTDRIVER_IPC BASE_DATA > + * - GTDRIVER_IPC BASE_IFACE > + */ > res = platform_get_resource(pdev, IORESOURCE_MEM, 2); > - addr = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(addr)) > - return PTR_ERR(addr); > - punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr; > + if (res) { > + addr = devm_ioremap_resource(&pdev->dev, res); > + if (!IS_ERR(addr)) > + punit_ipcdev->base[ISPDRIVER_IPC][BASE_DATA] = addr; > + } And here, what about just replacing return to dev_warn()? > > res = platform_get_resource(pdev, IORESOURCE_MEM, 3); > - addr = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(addr)) > - return PTR_ERR(addr); > - punit_ipcdev->base[ISPDRIVER_IPC][BASE_IFACE] = addr; > + if (res) { > + addr = devm_ioremap_resource(&pdev->dev, res); > + if (!IS_ERR(addr)) > + punit_ipcdev->base[ISPDRIVER_IPC][BASE_IFACE] = addr; > + } > > res = platform_get_resource(pdev, IORESOURCE_MEM, 4); > - addr = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(addr)) > - return PTR_ERR(addr); > - punit_ipcdev->base[GTDRIVER_IPC][BASE_DATA] = addr; > + if (res) { > + addr = devm_ioremap_resource(&pdev->dev, res); > + if (!IS_ERR(addr)) > + punit_ipcdev->base[GTDRIVER_IPC][BASE_DATA] = addr; > + } > > res = platform_get_resource(pdev, IORESOURCE_MEM, 5); > - addr = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(addr)) > - return PTR_ERR(addr); > - punit_ipcdev->base[GTDRIVER_IPC][BASE_IFACE] = addr; > + if (res) { > + addr = devm_ioremap_resource(&pdev->dev, res); > + if (!IS_ERR(addr)) > + punit_ipcdev->base[GTDRIVER_IPC][BASE_IFACE] = addr; > + } > > return 0; > } > -- > 1.7.10.4 > -- With Best Regards, Andy Shevchenko