Date: Thu, 9 Jul 2015 19:52:43 +0800
From: Yi Zhang <yizhang@marvell.com>
To: Lee Jones <lee.jones@linaro.org>
CC: <sameo@linux.intel.com>, <pebolle@tiscali.nl>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2 2/2] mfd: 88pm88x: initialize 88pm886/88pm880 base
 support
Message-ID: <20150709115243.GA23453@yizhang>
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>
 <20150701122018.GJ3210@x1>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20150701122018.GJ3210@x1>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4349
Lines: 120

On Wed, Jul 01, 2015 at 01:20:18PM +0100, Lee Jones wrote:
> 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 <yizhang@marvell.com>
> > > > ---
> > > >  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.

  Thanks, I need to pay attention to the etiquette, reading such a long
  mail is absolutly not a good experience, my fault, sorry;
> 
> > > > +	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.

  I got your point.
> 
> [...] /* 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.
  
  Clear now;
> 
> [...]
> 
> > > > +	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?

  Just return is a good idea, my intention is trying to invoke the
  user's attention that the power off fails..
> 
> [...]
> 
> > > > +#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.

  Got your point, I will remove them and clean up the regulator part;
  thanks;
> 
> [...]
> 
> -- 
> 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/