Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753510AbcDUVOA (ORCPT ); Thu, 21 Apr 2016 17:14:00 -0400 Received: from mail-oi0-f48.google.com ([209.85.218.48]:34861 "EHLO mail-oi0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752431AbcDUVN6 (ORCPT ); Thu, 21 Apr 2016 17:13:58 -0400 MIME-Version: 1.0 In-Reply-To: <20160415050553.GF3232@f23x64.localdomain> References: <1459452489-46827-1-git-send-email-aubrey.li@linux.intel.com> <570A5948.8000405@linux.intel.com> <20160415003236.GA3232@f23x64.localdomain> <57104F92.9060908@linux.intel.com> <20160415050553.GF3232@f23x64.localdomain> Date: Fri, 22 Apr 2016 00:13:56 +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 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2000 Lines: 49 On Fri, Apr 15, 2016 at 8:05 AM, Darren Hart wrote: >> >>>> + /* >> >>>> + * 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. >> > >> We discussed this. >> - For the necessary resources, if we obtain an error address, we should >> return immediately. >> - For the optional resources, we keep quiet if we don't get them, that >> is, not throwing a warning out. > > Andy, he's checking for "res" now too, which is a good extra check since > devm_ioremap_resource will issue a dev_err "invalid resource" if it's NULL, even > though in our case, that's expected for an optional resource. This adds the > extra nesting, and a dev_warn wouldn't be appropriate for an option resource. > > I'm happy to queue this to fixes at this point. Andy, are you OK with the > resolution here? Gmail hided this mail from me by some reason. I'm fine with the resolution. -- With Best Regards, Andy Shevchenko