Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753393AbcDUVKh (ORCPT ); Thu, 21 Apr 2016 17:10:37 -0400 Received: from mail-oi0-f50.google.com ([209.85.218.50]:35269 "EHLO mail-oi0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751658AbcDUVKf convert rfc822-to-8bit (ORCPT ); Thu, 21 Apr 2016 17:10:35 -0400 MIME-Version: 1.0 In-Reply-To: <20160415003236.GA3232@f23x64.localdomain> References: <1459452489-46827-1-git-send-email-aubrey.li@linux.intel.com> <570A5948.8000405@linux.intel.com> <20160415003236.GA3232@f23x64.localdomain> Date: Fri, 22 Apr 2016 00:10:34 +0300 Message-ID: Subject: Re: [PATCH] platform:x86 decouple telemetry driver from the optional IPC resources From: Andy Shevchenko To: Darren Hart Cc: "Li, Aubrey" , qipeng.zha@intel.com, 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: 6090 Lines: 154 On Fri, Apr 15, 2016 at 3:32 AM, Darren Hart wrote: > On Sun, Apr 10, 2016 at 09:46:48PM +0800, Li, Aubrey wrote: >> 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. >> >> --- 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. >> > > This is typically necessary as you would not want the comment fixes above to be > backed out if the functional changes below were found to be buggy and reverted. > This is why we encourage small functional changes. It protects against > inadvertent reverts and facilitates review. > > That said, these comment changes continue below in a way that makes it a bit > difficult to isolate them out, so I do not particularly object. > > That said, everyone should understand that Andy is part of the > platform-driver-x86 maintainer team so please respect his comments as such. > >> > >> > >> >> 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? > > In both arguments, we need to identify the WHY. > > I imagine Andy is trying to reduce the copy and paste potential for error as > well as error introduction in future patches. There are... 7 or so cases with > near identical usage, that's a compelling argument for a refactor such as the > helper Andy suggests. Correct, But it might be done in a separate patch I suppose. > Aubrey, you said you don't like it. Why is that? Will it not save enough lines > of code to be worth it? Are you concerned about revalidating the change? > > In my opinion, a refactor is a good suggestion, but I would be OK with this > patch as it is and a refactor to follow. I hesitate to do this when the refactor > is really critical as it may not happen, but in this case, it doesn't seem > absolutely necessary. > >> >> > 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; >> > } >> >> @@ -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. > > Why is that? Will the driver fail to provide any functionality? Or could it be > the other IFACEs could still be of some use? > > This one does need a justification. -- With Best Regards, Andy Shevchenko