Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753293AbbGAMUd (ORCPT ); Wed, 1 Jul 2015 08:20:33 -0400 Received: from mail-wg0-f41.google.com ([74.125.82.41]:36050 "EHLO mail-wg0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753669AbbGAMUX (ORCPT ); Wed, 1 Jul 2015 08:20:23 -0400 Date: Wed, 1 Jul 2015 13:20:18 +0100 From: Lee Jones To: Yi Zhang Cc: sameo@linux.intel.com, pebolle@tiscali.nl, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, zhouqiao@marvell.com Subject: Re: [PATCH V2 2/2] mfd: 88pm88x: initialize 88pm886/88pm880 base support Message-ID: <20150701122018.GJ3210@x1> References: <1434098601-3498-1-git-send-email-yizhang@marvell.com> <1434098601-3498-3-git-send-email-yizhang@marvell.com> <20150625083248.GT15013@x1> <20150626124928.GF32687@yizhang> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150626124928.GF32687@yizhang> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3723 Lines: 106 On Fri, 26 Jun 2015, Yi Zhang wrote: > On Thu, Jun 25, 2015 at 09:32:48AM +0100, Lee Jones wrote: > > On Fri, 12 Jun 2015, Yi Zhang wrote: > > > > > 88pm886 and 88pm880 are combo PMIC chip, which integrates > > > regulator, onkey, rtc, gpadc, charger, fuelgauge function; > > > > > > this patch add the basic support for them, adding related resource, such as > > > interrupt, preparing for the client-device driver > > > > > > Signed-off-by: Yi Zhang > > > --- > > > drivers/mfd/88pm880-table.c | 173 ++++++++++++ > > > drivers/mfd/88pm886-table.c | 173 ++++++++++++ > > > drivers/mfd/88pm88x-core.c | 584 ++++++++++++++++++++++++++++++++++++++++ > > > drivers/mfd/88pm88x-i2c.c | 167 ++++++++++++ > > > drivers/mfd/88pm88x-irq.c | 171 ++++++++++++ > > > drivers/mfd/88pm88x.h | 51 ++++ > > Thanks very much for your patience on such a long patch, it is my > fault, I will modify according to your precious comments and split > them in the next version, thanks very much; You are welcome. [...] /* Cut 100's of lines where you were agreeing with me */ In future, please cut all of the lines that you agree with me and only leave in the useful or contentious items. > > > + chip->ldo_page_addr = client->addr + 1; > > > + chip->power_page_addr = client->addr + 1; > > > + chip->gpadc_page_addr = client->addr + 2; > > > + chip->battery_page_addr = client->addr + 3; > > > + chip->buck_page_addr = client->addr + 4; > > > + chip->test_page_addr = client->addr + 7; > > > > I have no idea what's going on here, but it's almost certainly not > > correct. Instead of separating these out per device have a base > > address and create DEFINES for each of them. > > it is because this PMIC has several i2c address, each has different > function, the offset vesus the client->addr is fixed, the client->addr > is 0x30 from device tree, then the gpadc_page_addr is 0x32, that is > why I use client->addr + 2; > > you mean the DEFINES are better? I do mean that, yes. If you don't want supply nodes for each of the client devices (what is the reason for that?), then just supply the base address and use DEFINES to obtain the offsets. [...] /* Lots more 'yes's removed */ > > > + ret = mfd_add_devices(chip->dev, 0, common_cell_devs, > > > > What does 0 mean? > > I should use -1 here, though it _seems_ works.. Please use the pre-allocated platform DEFINEs for them. [...] > > > + for (;;) > > > + cpu_relax(); > > > > What's the point of this? > > I just would like to busy loop at the end of the power_off callback; Is that when you're meant to do? Why can't you just return? [...] > > > +#define CELL_DEV(_name, _r, _compatible, _id) { \ > > > + .name = _name, \ > > > + .of_compatible = _compatible, \ > > > + .num_resources = ARRAY_SIZE(_r), \ > > > + .resources = _r, \ > > > + .id = _id, \ > > > + } > > > > This is not required. > > > > If you feel the need for an MFD Cell macro, you probably have too many > > MFD cells and you have done something wrong.. > > Yes, I should use one MFD to cover regulators and one MFD to cover > LEDs, I will remember this lesson; thanks; That wasn't what I was saying. Just remove the MACRO and add the information into the cells like all the other MFD drivers do. [...] -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/