From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753406AbaIALsZ (ORCPT ); Mon, 1 Sep 2014 07:48:25 -0400 Received: from mga09.intel.com ([134.134.136.24]:41108 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753193AbaIALsY (ORCPT ); Mon, 1 Sep 2014 07:48:24 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,442,1406617200"; d="scan'208";a="566554578" Message-ID: <1409572101.30155.59.camel@linux.intel.com> Subject: Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code From: Andy Shevchenko To: Lee Jones Cc: Bjorn Helgaas , linux-kernel@vger.kernel.org, Samuel Ortiz , Chang Rebecca Swee Fun Date: Mon, 01 Sep 2014 14:48:21 +0300 In-Reply-To: <20140901113805.GO7374@lee--X1> References: <1408705096-31286-1-git-send-email-andriy.shevchenko@linux.intel.com> <1408705096-31286-2-git-send-email-andriy.shevchenko@linux.intel.com> <20140901091317.GG7374@lee--X1> <1409566903.30155.47.camel@linux.intel.com> <20140901113805.GO7374@lee--X1> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.5-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2014-09-01 at 12:38 +0100, Lee Jones wrote: > On Mon, 01 Sep 2014, Andy Shevchenko wrote: > > > On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote: > > > On Fri, 22 Aug 2014, Andy Shevchenko wrote: > > > > > > > This patch refactors the driver to use helper functions instead of > > > > copy'n'pasted pieces of code. > > > > > > > > Tested-by: Chang Rebecca Swee Fun > > > > Signed-off-by: Andy Shevchenko > > > > --- > > > > drivers/mfd/lpc_sch.c | 146 ++++++++++++++++++++++---------------------------- > > > > 1 file changed, 64 insertions(+), 82 deletions(-) > > > > > > > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c > > > > index 4ee7550..0f01ef0 100644 > > > > --- a/drivers/mfd/lpc_sch.c > > > > +++ b/drivers/mfd/lpc_sch.c > > > > @@ -40,41 +40,6 @@ > > > > > > [...] > > > > Thanks for review, my answers below. > > > > > > > > > -static int lpc_sch_probe(struct pci_dev *dev, > > > > - const struct pci_device_id *id) > > > > +static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name, > > > > + struct resource *res, int size) > > > > { > > > > unsigned int base_addr_cfg; > > > > unsigned short base_addr; > > > > - int i, cells = 0; > > > > - int ret; > > > > > > > > - pci_read_config_dword(dev, SMBASE, &base_addr_cfg); > > > > + pci_read_config_dword(pdev, where, &base_addr_cfg); > > > > base_addr = 0; > > > > if (!(base_addr_cfg & (1 << 31))) > > > > - dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n"); > > > > + dev_warn(&pdev->dev, "Decode of the %s I/O range disabled\n", > > > > + name); > > > > else > > > > base_addr = (unsigned short)base_addr_cfg; > > > > > > > > if (base_addr == 0) { > > > > - dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n"); > > > > - } else { > > > > - lpc_sch_cells[cells++] = isch_smbus_cell; > > > > - smbus_sch_resource.start = base_addr; > > > > - smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1; > > > > + dev_warn(&pdev->dev, "I/O space for %s uninitialized\n", name); > > > > + return -ENODEV; > > > > > > If you're going to return an error, you need to use dev_err() above. > > > > Okay. > > > > > > } > > > > > > > > - pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg); > > > > - base_addr = 0; > > > > - if (!(base_addr_cfg & (1 << 31))) > > > > - dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n"); > > > > + res->start = base_addr; > > > > + res->end = base_addr + size - 1; > > > > + res->flags = IORESOURCE_IO; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int lpc_sch_populate_cell(struct pci_dev *pdev, int where, > > > > + const char *name, int size, int id, > > > > + struct mfd_cell *cell) > > > > +{ > > > > + struct resource *res; > > > > + int ret; > > > > + > > > > + res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL); > > > > + if (!res) > > > > + return -ENOMEM; > > > > + > > > > + ret = lpc_sch_get_io(pdev, where, name, res, size); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + memset(cell, 0, sizeof(*cell)); > > > > + > > > > + cell->name = name; > > > > + cell->resources = res; > > > > + cell->num_resources = 1; > > > > + cell->ignore_resource_conflicts = true; > > > > + cell->id = id; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int lpc_sch_probe(struct pci_dev *dev, > > > > + const struct pci_device_id *id) > > > > +{ > > > > + struct mfd_cell lpc_sch_cells[3]; > > > > + int size, cells = 0; > > > > + int ret; > > > > + > > > > + ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE, > > > > + id->device, &lpc_sch_cells[cells]); > > > > + if (!ret) > > > > + cells++; > > > > > > You're masking errors here. You need to return on error. > > > > No, I don't. It's a local error, I just need to understand if the HW has > > or hasn't the IP part we're looking for. > > Yes, you do. -ENOMEM is not a local error, it needs to be returned. > > > I could, let's say, return true or false, if you prefer, with the above > > meaning. > > I don't see this very often, which makes me wonder if it's the right > thing to do at all; however, if you're going to this then please > return a local state, instead of a Linux Error Code. Okay, what about 1 — IP is present and we have no error, 0 — no ip, no error, -err — error that should be returned, would it work for you? > #define LCP_SKIP_RESOURCE /* Seems resonable? */ -- Andy Shevchenko Intel Finland Oy