Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753439AbcDJNqx (ORCPT ); Sun, 10 Apr 2016 09:46:53 -0400 Received: from mga03.intel.com ([134.134.136.65]:40451 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753026AbcDJNqv (ORCPT ); Sun, 10 Apr 2016 09:46:51 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,462,1455004800"; d="scan'208";a="781928185" Subject: Re: [PATCH] platform:x86 decouple telemetry driver from the optional IPC resources To: Andy Shevchenko References: <1459452489-46827-1-git-send-email-aubrey.li@linux.intel.com> Cc: qipeng.zha@intel.com, "dvhart@infradead.org" , platform-driver-x86@vger.kernel.org, "linux-kernel@vger.kernel.org" From: "Li, Aubrey" Message-ID: <570A5948.8000405@linux.intel.com> Date: Sun, 10 Apr 2016 21:46:48 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: 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: 7528 Lines: 192 On 2016/4/10 21:17, Andy Shevchenko wrote: > 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? I don't think it's necessary. > > >> 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 > Thanks for the idea, but I don't like a helper here, did you see anything harmful of the current implementation? > 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()? I don't think we need to continue the subsequent ops if an error address returns. Thanks, -Aubrey